Re: [Qemu-devel] [PATCH RFC qom-next for-next v2 6/6] pci: Move VMSTATE_MSIX() into vmstate_pci_device

[Michael S. Tsirkin]

On Wed, Apr 16, 2014 at 04:22:32PM +0200, Andreas Färber wrote:
> Am 02.09.2013 13:31, schrieb Michael S. Tsirkin:
> > On Mon, Jul 29, 2013 at 02:27:01AM +0200, Andreas Färber wrote:
> >> Use it conditional on msix_present() and drop msix_{save,load}() calls
> >> following pci_device_{save,load}().
> >>
> >> This reorders the msix_save() and msix_unuse_all_vectors() calls for
> >> virtio-pci, but they seem independent of each other.
> > 
> > No, that's a bug. msix_unuse_all_vectors clears pending state
> > if any, it will lose state if called before load.
> 
> Michael, you were just saying no here, now MegaSAS is forgetting to add
> appropriate VMState. How do you envision solving that bug? Can we move
> msix_unuse_all_vectors() to common code or something?


I agree it's a good idea to find a way to fix this.
I'll look into this, thanks  for the reminder.

> FTR, Stefan had requested on IRC that vmxnet state not be changed
> incompatibly. What we discussed there was to register the legacy savevm
> handler only for reading incoming state (NULL for writing new state);
> but I am no longer sure that could work due to new VMSTATE_PCI().
> 
> Dmitry, why is vmxnet using such a non-standard way of registering
> VMState for MSI-X, and can you please help to fix that for the benefit
> of all PCI devices?
> 
> Thanks,
> Andreas
> 
> >>
> >> Signed-off-by: Andreas Färber <address@hidden>
> >> ---
> >>  hw/misc/ivshmem.c      |  7 ++-----
> >>  hw/net/vmxnet3.c       | 27 +++------------------------
> >>  hw/pci/pci.c           |  8 ++++++++
> >>  hw/usb/hcd-xhci.c      |  1 -
> >>  hw/virtio/virtio-pci.c | 19 +++++++++++--------
> >>  include/hw/pci/msix.h  |  7 ++++---
> >>  6 files changed, 28 insertions(+), 41 deletions(-)
> >>
> >> diff --git a/hw/misc/ivshmem.c b/hw/misc/ivshmem.c
> >> index 4a74856..de997cd 100644
> >> --- a/hw/misc/ivshmem.c
> >> +++ b/hw/misc/ivshmem.c
> >> @@ -599,9 +599,7 @@ static void ivshmem_save(QEMUFile* f, void *opaque)
> >>      IVSHMEM_DPRINTF("ivshmem_save\n");
> >>      pci_device_save(pci_dev, f);
> >>  
> >> -    if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> >> -        msix_save(pci_dev, f);
> >> -    } else {
> >> +    if (!ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> >>          qemu_put_be32(f, proxy->intrstatus);
> >>          qemu_put_be32(f, proxy->intrmask);
> >>      }
> >> @@ -631,8 +629,7 @@ static int ivshmem_load(QEMUFile* f, void *opaque, int 
> >> version_id)
> >>      }
> >>  
> >>      if (ivshmem_has_feature(proxy, IVSHMEM_MSI)) {
> >> -        msix_load(pci_dev, f);
> >> -  ivshmem_use_msix(proxy);
> >> +        ivshmem_use_msix(proxy);
> >>      } else {
> >>          proxy->intrstatus = qemu_get_be32(f);
> >>          proxy->intrmask = qemu_get_be32(f);
> >> diff --git a/hw/net/vmxnet3.c b/hw/net/vmxnet3.c
> >> index 3bad83c..471ca24 100644
> >> --- a/hw/net/vmxnet3.c
> >> +++ b/hw/net/vmxnet3.c
> >> @@ -2031,21 +2031,6 @@ vmxnet3_cleanup_msi(VMXNET3State *s)
> >>      }
> >>  }
> >>  
> >> -static void
> >> -vmxnet3_msix_save(QEMUFile *f, void *opaque)
> >> -{
> >> -    PCIDevice *d = PCI_DEVICE(opaque);
> >> -    msix_save(d, f);
> >> -}
> >> -
> >> -static int
> >> -vmxnet3_msix_load(QEMUFile *f, void *opaque, int version_id)
> >> -{
> >> -    PCIDevice *d = PCI_DEVICE(opaque);
> >> -    msix_load(d, f);
> >> -    return 0;
> >> -}
> >> -
> >>  static const MemoryRegionOps b0_ops = {
> >>      .read = vmxnet3_io_bar0_read,
> >>      .write = vmxnet3_io_bar0_write,
> >> @@ -2103,9 +2088,6 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev)
> >>  
> >>      vmxnet3_net_init(s);
> >>  
> >> -    register_savevm(dev, "vmxnet3-msix", -1, 1,
> >> -                    vmxnet3_msix_save, vmxnet3_msix_load, s);
> >> -
> >>      add_boot_device_path(s->conf.bootindex, dev, "/address@hidden");
> >>  
> >>      return 0;
> >> @@ -2114,13 +2096,10 @@ static int vmxnet3_pci_init(PCIDevice *pci_dev)
> >>  
> >>  static void vmxnet3_pci_uninit(PCIDevice *pci_dev)
> >>  {
> >> -    DeviceState *dev = DEVICE(pci_dev);
> >>      VMXNET3State *s = VMXNET3(pci_dev);
> >>  
> >>      VMW_CBPRN("Starting uninit...");
> >>  
> >> -    unregister_savevm(dev, "vmxnet3-msix", s);
> >> -
> >>      vmxnet3_net_uninit(s);
> >>  
> >>      vmxnet3_cleanup_msix(s);
> >> @@ -2372,9 +2351,9 @@ const VMStateInfo int_state_info = {
> >>  
> >>  static const VMStateDescription vmstate_vmxnet3 = {
> >>      .name = "vmxnet3",
> >> -    .version_id = 1,
> >> -    .minimum_version_id = 1,
> >> -    .minimum_version_id_old = 1,
> >> +    .version_id = 2,
> >> +    .minimum_version_id = 2,
> >> +    .minimum_version_id_old = 2,
> >>      .pre_save = vmxnet3_pre_save,
> >>      .post_load = vmxnet3_post_load,
> >>      .fields      = (VMStateField[]) {
> >> diff --git a/hw/pci/pci.c b/hw/pci/pci.c
> >> index b790d98..bd6078b 100644
> >> --- a/hw/pci/pci.c
> >> +++ b/hw/pci/pci.c
> >> @@ -481,6 +481,13 @@ static bool pci_device_aer_needed(void *opaque, int 
> >> version_id)
> >>      return pci_is_express(s) && s->exp.aer_log.log != NULL;
> >>  }
> >>  
> >> +static bool pci_device_msix_needed(void *opaque, int version_id)
> >> +{
> >> +    PCIDevice *s = opaque;
> >> +
> >> +    return msix_present(s);
> >> +}
> >> +
> >>  const VMStateDescription vmstate_pci_device = {
> >>      .name = "PCIDevice",
> >>      .version_id = 2,
> >> @@ -499,6 +506,7 @@ const VMStateDescription vmstate_pci_device = {
> >>          VMSTATE_BUFFER_UNSAFE_INFO(irq_state, PCIDevice, 2,
> >>                                     vmstate_info_pci_irq_state,
> >>                                     PCI_NUM_PINS * sizeof(int32_t)),
> >> +        VMSTATE_MSIX_TEST(pci_device_msix_needed),
> >>          VMSTATE_STRUCT_TEST(exp.aer_log, PCIDevice, 
> >> pci_device_aer_needed, 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 167b58d..ca7b3cd 100644
> >> --- a/hw/usb/hcd-xhci.c
> >> +++ b/hw/usb/hcd-xhci.c
> >> @@ -3545,7 +3545,6 @@ static const VMStateDescription vmstate_xhci = {
> >>      .post_load = usb_xhci_post_load,
> >>      .fields = (VMStateField[]) {
> >>          VMSTATE_PCI_DEVICE(),
> >> -        VMSTATE_MSIX(parent_obj, XHCIState),
> >>  
> >>          VMSTATE_STRUCT_VARRAY_UINT32(ports, XHCIState, numports, 1,
> >>                                       vmstate_xhci_port, XHCIPort),
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index c38cfd1..8e2789d 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -121,10 +121,12 @@ static void virtio_pci_notify(DeviceState *d, 
> >> uint16_t vector)
> >>  static void virtio_pci_save_config(DeviceState *d, QEMUFile *f)
> >>  {
> >>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >> -    pci_device_save(&proxy->pci_dev, f);
> >> -    msix_save(&proxy->pci_dev, f);
> >> -    if (msix_present(&proxy->pci_dev))
> >> +    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> >> +
> >> +    pci_device_save(pci_dev, f);
> >> +    if (msix_present(pci_dev)) {
> >>          qemu_put_be16(f, proxy->vdev->config_vector);
> >> +    }
> >>  }
> >>  
> >>  static void virtio_pci_save_queue(DeviceState *d, int n, QEMUFile *f)
> >> @@ -137,20 +139,21 @@ static void virtio_pci_save_queue(DeviceState *d, 
> >> int n, QEMUFile *f)
> >>  static int virtio_pci_load_config(DeviceState *d, QEMUFile *f)
> >>  {
> >>      VirtIOPCIProxy *proxy = to_virtio_pci_proxy(d);
> >> +    PCIDevice *pci_dev = PCI_DEVICE(proxy);
> >>      int ret;
> >> -    ret = pci_device_load(&proxy->pci_dev, f);
> >> +
> >> +    ret = pci_device_load(pci_dev, f);
> >>      if (ret) {
> >>          return ret;
> >>      }
> >> -    msix_unuse_all_vectors(&proxy->pci_dev);
> >> -    msix_load(&proxy->pci_dev, f);
> >> -    if (msix_present(&proxy->pci_dev)) {
> >> +    msix_unuse_all_vectors(pci_dev);
> >> +    if (msix_present(pci_dev)) {
> >>          qemu_get_be16s(f, &proxy->vdev->config_vector);
> >>      } else {
> >>          proxy->vdev->config_vector = VIRTIO_NO_VECTOR;
> >>      }
> >>      if (proxy->vdev->config_vector != VIRTIO_NO_VECTOR) {
> >> -        return msix_vector_use(&proxy->pci_dev, 
> >> proxy->vdev->config_vector);
> >> +        return msix_vector_use(pci_dev, proxy->vdev->config_vector);
> >>      }
> >>      return 0;
> >>  }
> >> diff --git a/include/hw/pci/msix.h b/include/hw/pci/msix.h
> >> index 954d82b..b1b8874 100644
> >> --- a/include/hw/pci/msix.h
> >> +++ b/include/hw/pci/msix.h
> >> @@ -46,12 +46,13 @@ void msix_unset_vector_notifiers(PCIDevice *dev);
> >>  
> >>  extern const VMStateDescription vmstate_msix;
> >>  
> >> -#define VMSTATE_MSIX(_field, _state) {                               \
> >> -    .name       = (stringify(_field)),                               \
> >> +#define VMSTATE_MSIX_TEST(_test) {                                   \
> >> +    .name       = "MSI-X",                                           \
> >>      .size       = sizeof(PCIDevice),                                 \
> >>      .vmsd       = &vmstate_msix,                                     \
> >>      .flags      = VMS_STRUCT,                                        \
> >> -    .offset     = vmstate_offset_value(_state, _field, PCIDevice),   \
> >> +    .offset     = 0,                                                 \
> >> +    .field_exists = (_test),                                         \
> >>  }
> >>  
> >>  #endif
> >> -- 
> >> 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