Re: [PATCH v27 03/17] vfio: Add save and load functions for VFIO PCI devices

[Kirti Wankhede <kwankhede@nvidia.com>]

On 10/22/2020 7:36 PM, Alex Williamson wrote:
> On Thu, 22 Oct 2020 16:41:53 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> Added functions to save and restore PCI device specific data,
>> specifically config space of PCI device.
>>
>> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
>> Reviewed-by: Neo Jia <cjia@nvidia.com>
>> ---
>>   hw/vfio/pci.c                 | 48 +++++++++++++++++++++++++++++++++++++++++++
>>   include/hw/vfio/vfio-common.h |  2 ++
>>   2 files changed, 50 insertions(+)
>>
>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
>> index bffd5bfe3b78..1036a5332772 100644
>> --- a/hw/vfio/pci.c
>> +++ b/hw/vfio/pci.c
>> @@ -41,6 +41,7 @@
>>   #include "trace.h"
>>   #include "qapi/error.h"
>>   #include "migration/blocker.h"
>> +#include "migration/qemu-file.h"
>>   
>>   #define TYPE_VFIO_PCI_NOHOTPLUG "vfio-pci-nohotplug"
>>   
>> @@ -2401,11 +2402,58 @@ static Object *vfio_pci_get_object(VFIODevice *vbasedev)
>>       return OBJECT(vdev);
>>   }
>>   
>> +static bool vfio_msix_enabled(void *opaque, int version_id)
>> +{
>> +    PCIDevice *pdev = opaque;
>> +
>> +    return msix_enabled(pdev);
> 
> Why msix_enabled() rather than msix_present()?  It seems that even if
> MSI-X is not enabled at the point in time where this is called, there's
> still emulated state in the vector table.  For example if the guest has
> written the vectors but has not yet enabled the capability at the point
> where we start a migration, this test might cause the guest on the
> target to enable MSI-X with uninitialized data in the vector table.
> 

You're correct. Changing it to check if present.

>> +}
>> +
>> +const VMStateDescription vmstate_vfio_pci_config = {
>> +    .name = "VFIOPCIDevice",
>> +    .version_id = 1,
>> +    .minimum_version_id = 1,
>> +    .fields = (VMStateField[]) {
>> +        VMSTATE_PCI_DEVICE(pdev, VFIOPCIDevice),
>> +        VMSTATE_MSIX_TEST(pdev, VFIOPCIDevice, vfio_msix_enabled),
> 
> MSI (not-X) state is entirely in config space, so doesn't need a
> separate field, correct?
> 

Yes.

> Otherwise this looks quite a bit cleaner than previous version, I hope
> VMState experts can confirm this is sufficiently extensible within the
> migration framework.  Thanks,
> 

Thanks,
Kirti

> Alex
> 
>> +        VMSTATE_END_OF_LIST()
>> +    }
>> +};
>> +
>> +static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
>> +{
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +
>> +    vmstate_save_state(f, &vmstate_vfio_pci_config, vdev, NULL);
>> +}
>> +
>> +static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
>> +{
>> +    VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice, vbasedev);
>> +    PCIDevice *pdev = &vdev->pdev;
>> +    int ret;
>> +
>> +    ret = vmstate_load_state(f, &vmstate_vfio_pci_config, vdev, 1);
>> +    if (ret) {
>> +        return ret;
>> +    }
>> +
>> +    if (msi_enabled(pdev)) {
>> +        vfio_msi_enable(vdev);
>> +    } else if (msix_enabled(pdev)) {
>> +        vfio_msix_enable(vdev);
>> +    }
>> +
>> +    return ret;
>> +}
>> +
>>   static VFIODeviceOps vfio_pci_ops = {
>>       .vfio_compute_needs_reset = vfio_pci_compute_needs_reset,
>>       .vfio_hot_reset_multi = vfio_pci_hot_reset_multi,
>>       .vfio_eoi = vfio_intx_eoi,
>>       .vfio_get_object = vfio_pci_get_object,
>> +    .vfio_save_config = vfio_pci_save_config,
>> +    .vfio_load_config = vfio_pci_load_config,
>>   };
>>   
>>   int vfio_populate_vga(VFIOPCIDevice *vdev, Error **errp)
>> diff --git a/include/hw/vfio/vfio-common.h b/include/hw/vfio/vfio-common.h
>> index fe99c36a693a..ba6169cd926e 100644
>> --- a/include/hw/vfio/vfio-common.h
>> +++ b/include/hw/vfio/vfio-common.h
>> @@ -120,6 +120,8 @@ struct VFIODeviceOps {
>>       int (*vfio_hot_reset_multi)(VFIODevice *vdev);
>>       void (*vfio_eoi)(VFIODevice *vdev);
>>       Object *(*vfio_get_object)(VFIODevice *vdev);
>> +    void (*vfio_save_config)(VFIODevice *vdev, QEMUFile *f);
>> +    int (*vfio_load_config)(VFIODevice *vdev, QEMUFile *f);
>>   };
>>   
>>   typedef struct VFIOGroup {
>