Re: [Qemu-devel] [RFC] PCI/migration merge vmstate_pci_device and vmstate_pcie_device

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Dec 16, 2016 at 09:35:28AM +0000, Dr. David Alan Gilbert wrote:
> * Michael S. Tsirkin (mst@redhat.com) wrote:
> > On Wed, Dec 14, 2016 at 07:58:29PM +0000, Dr. David Alan Gilbert (git) wrote:
> > > From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
> > > 
> > > The vmstate_pci_device and vmstate_pcie_devices differ
> > > just in the size of one buffer; combine the two using a _TEST
> > > macro.
> > > 
> > > I think this is safe as long as everywhere which currently
> > > uses either of these two uses the right type.
> > > 
> > > One thing that concerns me is that some places use pci_device_load/save
> > > which does some irq mangling, but others just use the VMSTATE_PCI_DEVICE
> > > macro - how are they getting the same irq mangling?
> > > 
> > > This passes a smoke test migrate of:
> > > ./x86_64-softmmu/qemu-system-x86_64 -M pc,accel=kvm -m 1024
> > > ./littlefed20.img -device e1000e -device virtio-net -device
> > > e1000 -device virtio-rng -device megasas -device megasas-gen2 -device
> > > ioh3420 -device nec-usb-xhci
> > > 
> > > to an unmodified qemu.
> > > 
> > > Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> > 
> > Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > feel free to merge through migration tree.
> 
> Thanks!
> What about my related question about the difference between pci_device_load/save
> with respect to the irq mangling it does?
> 
> Dave

Do you mean pci_update_irq_status?

All this saving/clearing is for compatibility with
old code which didn't set/clear it properly.
I would not be too surprised if that compatibility
just got broken at some point ...


> > > ---
> > >  hw/net/e1000e.c                    |  2 +-
> > >  hw/net/vmxnet3.c                   |  2 +-
> > >  hw/pci-bridge/ioh3420.c            |  2 +-
> > >  hw/pci-bridge/xio3130_downstream.c |  2 +-
> > >  hw/pci-bridge/xio3130_upstream.c   |  2 +-
> > >  hw/pci/pci.c                       | 41 +++++++++++++++++---------------------
> > >  hw/scsi/megasas.c                  |  2 +-
> > >  hw/scsi/vmw_pvscsi.c               |  2 +-
> > >  hw/usb/hcd-xhci.c                  |  2 +-
> > >  include/hw/pci/pcie.h              | 10 ----------
> > >  10 files changed, 26 insertions(+), 41 deletions(-)
> > > 
> > > diff --git a/hw/net/e1000e.c b/hw/net/e1000e.c
> > > index 4994e1c..463ac9b 100644
> > > --- a/hw/net/e1000e.c
> > > +++ b/hw/net/e1000e.c
> > > @@ -592,7 +592,7 @@ static const VMStateDescription e1000e_vmstate = {
> > >      .pre_save = e1000e_pre_save,
> > >      .post_load = e1000e_post_load,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj, E1000EState),
> > > +        VMSTATE_PCI_DEVICE(parent_obj, E1000EState),
> > >          VMSTATE_MSIX(parent_obj, E1000EState),
> > >  
> > >          VMSTATE_UINT32(ioaddr, E1000EState),
> > > diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> > > index bbb898b..86cded9 100644
> > > --- a/hw/net/vmxnet3.c
> > > +++ b/hw/net/vmxnet3.c
> > > @@ -2538,7 +2538,7 @@ static const VMStateDescription vmstate_vmxnet3_pcie_device = {
> > >      .minimum_version_id = 1,
> > >      .needed = vmxnet3_vmstate_need_pcie_device,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj, VMXNET3State),
> > > +        VMSTATE_PCI_DEVICE(parent_obj, VMXNET3State),
> > >          VMSTATE_END_OF_LIST()
> > >      }
> > >  };
> > > diff --git a/hw/pci-bridge/ioh3420.c b/hw/pci-bridge/ioh3420.c
> > > index c8b5ac4..98114e1 100644
> > > --- a/hw/pci-bridge/ioh3420.c
> > > +++ b/hw/pci-bridge/ioh3420.c
> > > @@ -178,7 +178,7 @@ static const VMStateDescription vmstate_ioh3420 = {
> > >      .minimum_version_id = 1,
> > >      .post_load = pcie_cap_slot_post_load,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> > > +        VMSTATE_PCI_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> > >          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 cef6e13..4c54301 100644
> > > --- a/hw/pci-bridge/xio3130_downstream.c
> > > +++ b/hw/pci-bridge/xio3130_downstream.c
> > > @@ -164,7 +164,7 @@ static const VMStateDescription vmstate_xio3130_downstream = {
> > >      .minimum_version_id = 1,
> > >      .post_load = pcie_cap_slot_post_load,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> > > +        VMSTATE_PCI_DEVICE(parent_obj.parent_obj.parent_obj, PCIESlot),
> > >          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 4ad0440..45dbd1c 100644
> > > --- a/hw/pci-bridge/xio3130_upstream.c
> > > +++ b/hw/pci-bridge/xio3130_upstream.c
> > > @@ -136,7 +136,7 @@ static const VMStateDescription vmstate_xio3130_upstream = {
> > >      .version_id = 1,
> > >      .minimum_version_id = 1,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj.parent_obj, PCIEPort),
> > > +        VMSTATE_PCI_DEVICE(parent_obj.parent_obj, PCIEPort),
> > >          VMSTATE_STRUCT(parent_obj.parent_obj.exp.aer_log, PCIEPort, 0,
> > >                         vmstate_pcie_aer_log, PCIEAERLog),
> > >          VMSTATE_END_OF_LIST()
> > > diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> > > index 16df188..8e4226b 100644
> > > --- a/hw/pci/pci.c
> > > +++ b/hw/pci/pci.c
> > > @@ -542,30 +542,29 @@ static VMStateInfo vmstate_info_pci_irq_state = {
> > >      .put  = put_pci_irq_state,
> > >  };
> > >  
> > > +static bool migrate_is_pcie(void *opaque, int version_id)
> > > +{
> > > +    return pci_is_express((PCIDevice *)opaque);
> > > +}
> > > +
> > > +static bool migrate_is_not_pcie(void *opaque, int version_id)
> > > +{
> > > +    return !pci_is_express((PCIDevice *)opaque);
> > > +}
> > > +
> > >  const VMStateDescription vmstate_pci_device = {
> > >      .name = "PCIDevice",
> > >      .version_id = 2,
> > >      .minimum_version_id = 1,
> > >      .fields = (VMStateField[]) {
> > >          VMSTATE_INT32_POSITIVE_LE(version_id, PCIDevice),
> > > -        VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0,
> > > -                                   vmstate_info_pci_config,
> > > +        VMSTATE_BUFFER_UNSAFE_INFO_TEST(config, PCIDevice,
> > > +                                   migrate_is_not_pcie,
> > > +                                   0, vmstate_info_pci_config,
> > >                                     PCI_CONFIG_SPACE_SIZE),
> > > -        VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
> > > -				   vmstate_info_pci_irq_state,
> > > -				   PCI_NUM_PINS * sizeof(int32_t)),
> > > -        VMSTATE_END_OF_LIST()
> > > -    }
> > > -};
> > > -
> > > -const VMStateDescription vmstate_pcie_device = {
> > > -    .name = "PCIEDevice",
> > > -    .version_id = 2,
> > > -    .minimum_version_id = 1,
> > > -    .fields = (VMStateField[]) {
> > > -        VMSTATE_INT32_POSITIVE_LE(version_id, PCIDevice),
> > > -        VMSTATE_BUFFER_UNSAFE_INFO(config, PCIDevice, 0,
> > > -                                   vmstate_info_pci_config,
> > > +        VMSTATE_BUFFER_UNSAFE_INFO_TEST(config, PCIDevice,
> > > +                                   migrate_is_pcie,
> > > +                                   0, vmstate_info_pci_config,
> > >                                     PCIE_CONFIG_SPACE_SIZE),
> > >          VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
> > >  				   vmstate_info_pci_irq_state,
> > > @@ -574,10 +573,6 @@ const VMStateDescription vmstate_pcie_device = {
> > >      }
> > >  };
> > >  
> > > -static inline const VMStateDescription *pci_get_vmstate(PCIDevice *s)
> > > -{
> > > -    return pci_is_express(s) ? &vmstate_pcie_device : &vmstate_pci_device;
> > > -}
> > >  
> > >  void pci_device_save(PCIDevice *s, QEMUFile *f)
> > >  {
> > > @@ -586,7 +581,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> > >       * This makes us compatible with old devices
> > >       * which never set or clear this bit. */
> > >      s->config[PCI_STATUS] &= ~PCI_STATUS_INTERRUPT;
> > > -    vmstate_save_state(f, pci_get_vmstate(s), s, NULL);
> > > +    vmstate_save_state(f, &vmstate_pci_device, s, NULL);
> > >      /* Restore the interrupt status bit. */
> > >      pci_update_irq_status(s);
> > >  }
> > > @@ -594,7 +589,7 @@ void pci_device_save(PCIDevice *s, QEMUFile *f)
> > >  int pci_device_load(PCIDevice *s, QEMUFile *f)
> > >  {
> > >      int ret;
> > > -    ret = vmstate_load_state(f, pci_get_vmstate(s), s, s->version_id);
> > > +    ret = vmstate_load_state(f, &vmstate_pci_device, s, s->version_id);
> > >      /* Restore the interrupt status bit. */
> > >      pci_update_irq_status(s);
> > >      return ret;
> > > diff --git a/hw/scsi/megasas.c b/hw/scsi/megasas.c
> > > index 67fc1e7..adc0c4b 100644
> > > --- a/hw/scsi/megasas.c
> > > +++ b/hw/scsi/megasas.c
> > > @@ -2288,7 +2288,7 @@ static const VMStateDescription vmstate_megasas_gen2 = {
> > >      .minimum_version_id = 0,
> > >      .minimum_version_id_old = 0,
> > >      .fields      = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj, MegasasState),
> > > +        VMSTATE_PCI_DEVICE(parent_obj, MegasasState),
> > >          VMSTATE_MSIX(parent_obj, MegasasState),
> > >  
> > >          VMSTATE_INT32(fw_state, MegasasState),
> > > diff --git a/hw/scsi/vmw_pvscsi.c b/hw/scsi/vmw_pvscsi.c
> > > index a5ce7de..7557546 100644
> > > --- a/hw/scsi/vmw_pvscsi.c
> > > +++ b/hw/scsi/vmw_pvscsi.c
> > > @@ -1207,7 +1207,7 @@ static const VMStateDescription vmstate_pvscsi_pcie_device = {
> > >      .name = "pvscsi/pcie",
> > >      .needed = pvscsi_vmstate_need_pcie_device,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj, PVSCSIState),
> > > +        VMSTATE_PCI_DEVICE(parent_obj, PVSCSIState),
> > >          VMSTATE_END_OF_LIST()
> > >      }
> > >  };
> > > diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> > > index 4acf0c6..e0b5169 100644
> > > --- a/hw/usb/hcd-xhci.c
> > > +++ b/hw/usb/hcd-xhci.c
> > > @@ -3894,7 +3894,7 @@ static const VMStateDescription vmstate_xhci = {
> > >      .version_id = 1,
> > >      .post_load = usb_xhci_post_load,
> > >      .fields = (VMStateField[]) {
> > > -        VMSTATE_PCIE_DEVICE(parent_obj, XHCIState),
> > > +        VMSTATE_PCI_DEVICE(parent_obj, XHCIState),
> > >          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 056d25e..894262e 100644
> > > --- a/include/hw/pci/pcie.h
> > > +++ b/include/hw/pci/pcie.h
> > > @@ -121,16 +121,6 @@ void pcie_add_capability(PCIDevice *dev,
> > >  void pcie_ari_init(PCIDevice *dev, uint16_t offset, uint16_t nextfn);
> > >  void pcie_dev_ser_num_init(PCIDevice *dev, uint16_t offset, uint64_t ser_num);
> > >  
> > > -extern const VMStateDescription vmstate_pcie_device;
> > > -
> > > -#define VMSTATE_PCIE_DEVICE(_field, _state) {                        \
> > > -    .name       = (stringify(_field)),                               \
> > > -    .size       = sizeof(PCIDevice),                                 \
> > > -    .vmsd       = &vmstate_pcie_device,                              \
> > > -    .flags      = VMS_STRUCT,                                        \
> > > -    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
> > > -}
> > > -
> > >  void pcie_cap_slot_hotplug_cb(HotplugHandler *hotplug_dev, DeviceState *dev,
> > >                                Error **errp);
> > >  void pcie_cap_slot_hot_unplug_request_cb(HotplugHandler *hotplug_dev,
> > > -- 
> > > 2.9.3
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK