Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort and PCIESlot into abstract QOM types

[Michael S. Tsirkin]

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" <address@hidden> writes:
> > 
> >> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
> >>> Signed-off-by: Andreas Färber <address@hidden>
> >>> ---
> >>>  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