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

["Andreas Färber" <afaerber@suse.de>]

Am 22.07.2013 22:29, schrieb Anthony Liguori:
> "Michael S. Tsirkin" <mst@redhat.com> writes:
> 
>> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas Färber wrote:
>>> Signed-off-by: Andreas Färber <afaerber@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

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg