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

[Kirti Wankhede]

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 {