On Apr 16, 2014, at 17:22 PM, Andreas Färber <afaer...@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 <afaer...@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
