Date: Tue, 23 Jul 2013 10:07:55 +0300
From: "Michael S. Tsirkin" <mst@redhat.com>
Message-ID: <20130723070755.GB11106@redhat.com>
References: <1374415744-6675-1-git-send-email-afaerber@suse.de>
	<1374415744-6675-5-git-send-email-afaerber@suse.de>
	<20130721202607.GA15187@redhat.com> <87y58y49tx.fsf@codemonkey.ws>
	<51ED9E71.3060809@suse.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <51ED9E71.3060809@suse.de>
Content-Transfer-Encoding: quoted-printable
Subject: Re: [Qemu-devel] [PATCH RFC qom-next 4/4] pcie_port: Turn PCIEPort
 and PCIESlot into abstract QOM types
To: Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>
Cc: qemu-devel@nongnu.org, Anthony Liguori <anthony@codemonkey.ws>

On Mon, Jul 22, 2013 at 11:04:49PM +0200, Andreas F=E4rber wrote:
> Am 22.07.2013 22:29, schrieb Anthony Liguori:
> > "Michael S. Tsirkin" <mst@redhat.com> writes:
> >=20
> >> On Sun, Jul 21, 2013 at 04:09:04PM +0200, Andreas F=E4rber wrote:
> >>> Signed-off-by: Andreas F=E4rber <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)
> >>> =20
> >>>  static int ioh3420_initfn(PCIDevice *d)
> >>>  {
> >>> -    PCIBridge *br =3D PCI_BRIDGE(d);
> >>> -    PCIEPort *p =3D DO_UPCAST(PCIEPort, br, br);
> >>> -    PCIESlot *s =3D DO_UPCAST(PCIESlot, port, p);
> >>> +    PCIEPort *p =3D PCIE_PORT(d);
> >>> +    PCIESlot *s =3D PCIE_SLOT(d);
> >>>      int rc;
> >>> =20
> >>>      rc =3D pci_bridge_initfn(d, TYPE_PCIE_BUS);
> >>> @@ -148,9 +147,7 @@ err_bridge:
> >>> =20
> >>>  static void ioh3420_exitfn(PCIDevice *d)
> >>>  {
> >>> -    PCIBridge *br =3D PCI_BRIDGE(d);
> >>> -    PCIEPort *p =3D DO_UPCAST(PCIEPort, br, br);
> >>> -    PCIESlot *s =3D DO_UPCAST(PCIESlot, port, p);
> >>> +    PCIESlot *s =3D PCIE_SLOT(d);
> >>> =20
> >>>      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);
> >>> =20
> >>> -    return DO_UPCAST(PCIESlot, port, DO_UPCAST(PCIEPort, br, br));
> >>> +    return PCIE_SLOT(d);
> >>>  }
> >>> =20
> >>>  static const VMStateDescription vmstate_ioh3420 =3D {
> >>> @@ -190,19 +187,19 @@ static const VMStateDescription vmstate_ioh34=
20 =3D {
> >>>      .minimum_version_id_old =3D 1,
> >>>      .post_load =3D pcie_cap_slot_post_load,
> >>>      .fields =3D (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()
> >>>      }
> >>>  };
> >>> =20
> >>>  static Property ioh3420_properties[] =3D {
> >>> -    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.
> >=20
> > What cast?
> >=20
> > VMstate takes a void *.
> >=20
> > One an object is cast to a void *, it's just as much as PCIESlot as i=
t
> > is a PCIEPort.
> >=20
> > That said, for consistency, I think having everything be relatively t=
o
> > *one* type for a Property list is pretty helpful.
> >=20
> > Expecting someone to know the type hierarchy by heart such that this
> > doesn't look like a bug is too much IMHO.
>=20
> I'm updating the patch to that effect for VMState. But I notice the rea=
l
> fix for qdev properties would be to move the PCIEPort property to the
> new PCIEPort type, so that all derived types inherit it. :)
>=20
> 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?
>=20
> Regards,
> Andreas

The real fix is savevm/loadvm taking into account
the class hierarchy.


> --=20
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 N=FCrnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imend=F6rffer; HRB 16746 AG N=FCrn=
berg