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 03:29:46PM -0500, Anthony Liguori wrote:
> "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.
> 
> Regards,
> 
> Anthony Liguori

Exactly.