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