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

[Andreas Färber]

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 <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