From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53590) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VGSXV-0003OQ-J6 for qemu-devel@nongnu.org; Mon, 02 Sep 2013 07:43:30 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VGSXP-00020F-KH for qemu-devel@nongnu.org; Mon, 02 Sep 2013 07:43:25 -0400 Received: from mx1.redhat.com ([209.132.183.28]:6802) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VGSXP-0001zx-Ce for qemu-devel@nongnu.org; Mon, 02 Sep 2013 07:43:19 -0400 Date: Mon, 2 Sep 2013 14:45:19 +0300 From: "Michael S. Tsirkin" Message-ID: <20130902114519.GE20911@redhat.com> References: <1375057621-19961-1-git-send-email-afaerber@suse.de> <1375057621-19961-3-git-send-email-afaerber@suse.de> <20130902113628.GB20911@redhat.com> <522478B1.4030407@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: <522478B1.4030407@suse.de> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH qom-next for-next v2 2/6] pci: Simplify VMSTATE_PCIE_DEVICE() macro List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Andreas =?iso-8859-1?Q?F=E4rber?= Cc: qemu-devel@nongnu.org, Anthony Liguori , Gerd Hoffmann On Mon, Sep 02, 2013 at 01:38:25PM +0200, Andreas F=E4rber wrote: > Am 02.09.2013 13:36, schrieb Michael S. Tsirkin: > > On Mon, Jul 29, 2013 at 02:26:57AM +0200, Andreas F=E4rber wrote: > >> Drop the arguments to avoid QOM refactorings causing more churn. > >> > >> Signed-off-by: Andreas F=E4rber > >=20 > > We discussed and discarded something very similar. >=20 > Anthony requested that I don't mix different types for the offsets in > VMSTATE_*() macros. Exactly. The solution is *not* not to put any types there. > >=20 > > This only works correctly if you put it in a struct which has the pci > > device at offset 0, and there's no either runtime or compile-time che= ck > > to verify it's the correct type. >=20 > As Anthony already said, QOM depends on the offset being 0, casts will > fail otherwise. So there are runtime checks. I don't see any runtime casts in vmstate. That's exactly the issue. This is a bad API. vmstate isn't limited to QOM objects, if you make a macro without arguments it should either make sense in any vmstate, make compilation fail if misused, or at least quickly make migration fail if misused. Not corrupt random memory. > >=20 > > So please find some other way to do this. >=20 > I did post an RFC, but that is significantly more invasive, since it > affects not only PCI and needs a lot of review before we can move > forward with it. >=20 > Andreas That one looks ok. > > Or go back to give fields sane names instead > > of "parent_obj". > >=20 > >=20 > >> --- > >> hw/pci-bridge/ioh3420.c | 2 +- > >> hw/pci-bridge/xio3130_downstream.c | 2 +- > >> hw/pci-bridge/xio3130_upstream.c | 2 +- > >> hw/usb/hcd-xhci.c | 2 +- > >> include/hw/pci/pcie.h | 6 +++--- > >> 5 files changed, 7 insertions(+), 7 deletions(-) > >> > >> diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c > >> index e07c7e8..0657354 100644 > >> --- a/hw/pci-bridge/ioh3420.c > >> +++ b/hw/pci-bridge/ioh3420.c > >> @@ -187,7 +187,7 @@ static const VMStateDescription vmstate_ioh3420 = =3D { > >> .minimum_version_id_old =3D 1, > >> .post_load =3D pcie_cap_slot_post_load, > >> .fields =3D (VMStateField[]) { > >> - VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIES= lot), > >> + VMSTATE_PCIE_DEVICE(), > >> VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log= , > >> PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLo= g), > >> VMSTATE_END_OF_LIST() > >> diff --git a/hw/pci-bridge/xio3130_downstream.c b/hw/pci-bridge/xio3= 130_downstream.c > >> index 2c84b1a..9022949 100644 > >> --- a/hw/pci-bridge/xio3130_downstream.c > >> +++ b/hw/pci-bridge/xio3130_downstream.c > >> @@ -154,7 +154,7 @@ static const VMStateDescription vmstate_xio3130_= downstream =3D { > >> .minimum_version_id_old =3D 1, > >> .post_load =3D pcie_cap_slot_post_load, > >> .fields =3D (VMStateField[]) { > >> - VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIES= lot), > >> + VMSTATE_PCIE_DEVICE(), > >> VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log= , > >> PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLo= g), > >> VMSTATE_END_OF_LIST() > >> diff --git a/hw/pci-bridge/xio3130_upstream.c b/hw/pci-bridge/xio313= 0_upstream.c > >> index 82add15..046b790 100644 > >> --- a/hw/pci-bridge/xio3130_upstream.c > >> +++ b/hw/pci-bridge/xio3130_upstream.c > >> @@ -133,7 +133,7 @@ static const VMStateDescription vmstate_xio3130_= upstream =3D { > >> .minimum_version_id =3D 1, > >> .minimum_version_id_old =3D 1, > >> .fields =3D (VMStateField[]) { > >> - VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort), > >> + VMSTATE_PCIE_DEVICE(), > >> VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort,= 0, > >> vmstate_pcie_aer_log, PCIEAERLog), > >> VMSTATE_END_OF_LIST() > >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c > >> index 9ba3e3e..a22861f 100644 > >> --- a/hw/usb/hcd-xhci.c > >> +++ b/hw/usb/hcd-xhci.c > >> @@ -3544,7 +3544,7 @@ static const VMStateDescription vmstate_xhci =3D= { > >> .version_id =3D 1, > >> .post_load =3D usb_xhci_post_load, > >> .fields =3D (VMStateField[]) { > >> - VMSTATE_PCIE_DEVICE(parent_obj, XHCIState), > >> + VMSTATE_PCIE_DEVICE(), > >> VMSTATE_MSIX(parent_obj, XHCIState), > >> =20 > >> VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1, > >> diff --git a/include/hw/pci/pcie.h b/include/hw/pci/pcie.h > >> index c010007..fc4ebd3 100644 > >> --- a/include/hw/pci/pcie.h > >> +++ b/include/hw/pci/pcie.h > >> @@ -132,12 +132,12 @@ void pcie_ari_init(PCIDevice *dev, uint16_t of= fset, uint16_t nextfn); > >> =20 > >> extern const VMStateDescription vmstate_pcie_device; > >> =20 > >> -#define VMSTATE_PCIE_DEVICE(_field, _state) { = \ > >> - .name =3D (stringify(_field)), = \ > >> +#define VMSTATE_PCIE_DEVICE() { = \ > >> + .name =3D "parent_obj", = \ > >> .size =3D sizeof(PCIDevice), = \ > >> .vmsd =3D &vmstate_pcie_device, = \ > >> .flags =3D VMS_STRUCT, = \ > >> - .offset =3D vmstate_offset_value(_state, _field, PCIDevice)= , \ > >> + .offset =3D 0, = \ > >> } > >> =20 > >> #endif /* QEMU_PCIE_H */ > >> --=20 > >> 1.8.1.4 >=20 >=20 > --=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