On Mon, Sep 02, 2013 at 01:38:25PM +0200, Andreas Färber wrote:
> Am 02.09.2013 13:36, schrieb Michael S. Tsirkin:
> > On Mon, Jul 29, 2013 at 02:26:57AM +0200, Andreas Färber wrote:
> >> Drop the arguments to avoid QOM refactorings causing more churn.
> >>
> >> Signed-off-by: Andreas Färber <afaer...@suse.de>
> >
> > We discussed and discarded something very similar.
>
> 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.
> >
> > 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 check
> > to verify it's the correct type.
>
> 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.
> >
> > So please find some other way to do this.
>
> 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.
>
> Andreas
That one looks ok.
> > Or go back to give fields sane names instead
> > of "parent_obj".
> >
> >
> >> ---
> >> 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 = {
> >> .minimum_version_id_old = 1,
> >> .post_load = pcie_cap_slot_post_load,
> >> .fields = (VMStateField[]) {
> >> - VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> >> + VMSTATE_PCIE_DEVICE(),
> >> VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
> >> PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
> >> VMSTATE_END_OF_LIST()
> >> diff --git a/hw/pci-bridge/xio3130_downstream.c
> >> b/hw/pci-bridge/xio3130_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 = {
> >> .minimum_version_id_old = 1,
> >> .post_load = pcie_cap_slot_post_load,
> >> .fields = (VMStateField[]) {
> >> - VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> >> + VMSTATE_PCIE_DEVICE(),
> >> VMSTATE_STRUCT(parent_obj.parent_obj.parent_obj.exp.aer_log,
> >> PCIESlot, 0, vmstate_pcie_aer_log, PCIEAERLog),
> >> VMSTATE_END_OF_LIST()
> >> diff --git a/hw/pci-bridge/xio3130_upstream.c
> >> b/hw/pci-bridge/xio3130_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 = {
> >> .minimum_version_id = 1,
> >> .minimum_version_id_old = 1,
> >> .fields = (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 = {
> >> .version_id = 1,
> >> .post_load = usb_xhci_post_load,
> >> .fields = (VMStateField[]) {
> >> - VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
> >> + VMSTATE_PCIE_DEVICE(),
> >> VMSTATE_MSIX(parent_obj, XHCIState),
> >>
> >> 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 offset,
> >> uint16_t nextfn);
> >>
> >> extern const VMStateDescription vmstate_pcie_device;
> >>
> >> -#define VMSTATE_PCIE_DEVICE(_field, _state) { \
> >> - .name = (stringify(_field)), \
> >> +#define VMSTATE_PCIE_DEVICE() { \
> >> + .name = "parent_obj", \
> >> .size = sizeof(PCIDevice), \
> >> .vmsd = &vmstate_pcie_device, \
> >> .flags = VMS_STRUCT, \
> >> - .offset = vmstate_offset_value(_state, _field, PCIDevice), \
> >> + .offset = 0, \
> >> }
> >>
> >> #endif /* QEMU_PCIE_H */
> >> --
> >> 1.8.1.4
>
>
> --
> SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
> GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg
