diff mbox

[RFC,qom-next,for-next,v2,6/6] pci: Move VMSTATE_MSIX() into vmstate_pci_device

Message ID 1375057621-19961-7-git-send-email-afaerber@suse.de
State New
Headers show

Commit Message

Andreas Färber July 29, 2013, 12:27 a.m. UTC
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.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 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(-)

Comments

Michael S. Tsirkin Sept. 2, 2013, 11:31 a.m. UTC | #1
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.

> 
> Signed-off-by: Andreas Färber <afaerber@suse.de>
> ---
>  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, "/ethernet-phy@0");
>  
>      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
Andreas Färber April 16, 2014, 2:22 p.m. UTC | #2
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?

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 <afaerber@suse.de>
>> ---
>>  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, "/ethernet-phy@0");
>>  
>>      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
Michael S. Tsirkin April 16, 2014, 6:57 p.m. UTC | #3
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 <afaerber@suse.de>
> >> ---
> >>  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, "/ethernet-phy@0");
> >>  
> >>      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
Dmitry Fleytman April 25, 2014, 12:18 p.m. UTC | #4
On Apr 16, 2014, at 17:22 PM, Andreas Färber <afaerber@suse.de> 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?
> 
> 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?

It was a log time ago so I don’t really remember. Probably got the template from some other device’s code.
Sure, I’ll put this to my queue.


> 
> Thanks,
> Andreas
> 
>>> 
>>> Signed-off-by: Andreas Färber <afaerber@suse.de>
>>> ---
>>> 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, "/ethernet-phy@0");
>>> 
>>>     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
diff mbox

Patch

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, "/ethernet-phy@0");
 
     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