On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas Färber wrote:
> Am 22.07.2013 22:29, schrieb Anthony Liguori:
> > "Michael S. Tsirkin" <m...@redhat.com> writes:
> >
> >> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
> >>> Signed-off-by: Andreas Färber <afaer...@suse.de>
> >>> ---
> >>> hw/pci-bridge/ioh3420.c | 23 ++++++++++-------------
> >>> hw/pci-bridge/xio3130_downstream.c | 23 ++++++++++-------------
> >>> hw/pci-bridge/xio3130_upstream.c | 15 +++++++--------
> >>> hw/pci/pcie_port.c | 22 ++++++++++++++++++++++
> >>> include/hw/pci/pcie_port.h | 14 ++++++++++++--
> >>> 5 files changed, 61 insertions(+), 36 deletions(-)
> >>>
> >>> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> >>> index 728f658..83db054 100644
> >>> --- a/hw/pci-bridge/ioh3420.c
> >>> +++ b/hw/pci-bridge/ioh3420.c
> >>> @@ -92,9 +92,8 @@ static void ioh3420_reset(DeviceState *qdev)
> >>>
> >>> static int ioh3420_initfn(PCIDevice *d)
> >>> {
> >>> - PCIBridge *br = PCI_BRIDGE(d);
> >>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >>> + PCIEPort *p = PCIE_PORT(d);
> >>> + PCIESlot *s = PCIE_SLOT(d);
> >>> int rc;
> >>>
> >>> rc = pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >>> @@ -148,9 +147,7 @@ err_bridge:
> >>>
> >>> static void ioh3420_exitfn(PCIDevice *d)
> >>> {
> >>> - PCIBridge *br = PCI_BRIDGE(d);
> >>> - PCIEPort *p = DO_UPCAST(PCIEPort, br, br);
> >>> - PCIESlot *s = DO_UPCAST(PCIESlot, port, p);
> >>> + PCIESlot *s = PCIE_SLOT(d);
> >>>
> >>> pcie_aer_exit(d);
> >>> pcie_chassis_del_slot(s);
> >>> @@ -180,7 +177,7 @@ PCIESlot *ioh3420_init(PCIBus *bus, int devfn, bool
> >>> multifunction,
> >>> qdev_prop_set_uint16(qdev, "slot", slot);
> >>> qdev_init_nofail(qdev);
> >>>
> >>> - return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
> >>> + return PCIE_SLOT(d);
> >>> }
> >>>
> >>> static const VMStateDescription vmstate_ioh3420 = {
> >>> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh3420 = {
> >>> .minimum_version_id_old = 1,
> >>> .post_load = pcie_cap_slot_post_load,
> >>> .fields = (VMStateField[]) {
> >>> - VMSTATE_PCIE_DEVICE(port.br.parent_obj, PCIESlot),
> >>> - VMSTATE_STRUCT(port.br.parent_obj.exp.aer_log, PCIESlot, 0,
> >>> + VMSTATE_PCIE_DEVICE(parent_obj, PCIBridge),
> >>> + VMSTATE_STRUCT(exp.aer_log, PCIDevice, 0,
> >>> vmstate_pcie_aer_log, PCIEAERLog),
> >>> VMSTATE_END_OF_LIST()
> >>> }
> >>> };
> >>>
> >>> static Property ioh3420_properties[] = {
> >>> - DEFINE_PROP_UINT8("port", PCIESlot, port.port, 0),
> >>> + DEFINE_PROP_UINT8("port", PCIEPort, port, 0),
> >>> DEFINE_PROP_UINT8("chassis", PCIESlot, chassis, 0),
> >>> DEFINE_PROP_UINT16("slot", PCIESlot, slot, 0),
> >>> - DEFINE_PROP_UINT16("aer_log_max", PCIESlot,
> >>> - port.br.parent_obj.exp.aer_log.log_max,
> >>> + DEFINE_PROP_UINT16("aer_log_max", PCIDevice,
> >>> + exp.aer_log.log_max,
> >>> PCIE_AER_LOG_MAX_DEFAULT),
> >>> DEFINE_PROP_END_OF_LIST(),
> >>> };
> >>
> >>
> >> This looks scary. This does a cast to different types
> >> without any checks at all.
> >
> > What cast?
> >
> > VMstate takes a void *.
> >
> > One an object is cast to a void *, it's just as much as PCIESlot as it
> > is a PCIEPort.
> >
> > That said, for consistency, I think having everything be relatively to
> > *one* type for a Property list is pretty helpful.
> >
> > Expecting someone to know the type hierarchy by heart such that this
> > doesn't look like a bug is too much IMHO.
>
> I'm updating the patch to that effect for VMState. But I notice the real
> fix for qdev properties would be to move the PCIEPort property to the
> new PCIEPort type, so that all derived types inherit it. :)
>
> For VMState I believe the real follow-up fix would be mst defining a
> central macro VMSTATE_PCI_DEVICE_AER_LOG() operating on PCIDevice.
> Why is that separate from VMSTATE_PCI_DEVICE() or VMSTATE_PCIE_DEVICE()
> in the first place?
>
> Regards,
> Andreas
The real fix is savevm/loadvm taking into account
the class hierarchy.
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
