[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH QEMU v25 03/17] vfio: Add save and load functions for VFIO PC
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [PATCH QEMU v25 03/17] vfio: Add save and load functions for VFIO PCI devices |
Date: |
Fri, 26 Jun 2020 13:16:13 +0100 |
User-agent: |
Mutt/1.14.3 (2020-06-14) |
* Alex Williamson (alex.williamson@redhat.com) wrote:
> On Wed, 24 Jun 2020 19:59:39 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>
> > On 6/23/2020 1:58 AM, Alex Williamson wrote:
> > > On Sun, 21 Jun 2020 01:51:12 +0530
> > > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > >
> > >> These functions save and restore PCI device specific data - config
> > >> space of PCI device.
> > >> Tested save and restore with MSI and MSIX type.
> > >>
> > >> Signed-off-by: Kirti Wankhede <kwankhede@nvidia.com>
> > >> Reviewed-by: Neo Jia <cjia@nvidia.com>
> > >> ---
> > >> hw/vfio/pci.c | 95
> > >> +++++++++++++++++++++++++++++++++++++++++++
> > >> include/hw/vfio/vfio-common.h | 2 +
> > >> 2 files changed, 97 insertions(+)
> > >>
> > >> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c
> > >> index 27f8872db2b1..5ba340aee1d4 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 "vfio-pci"
> > >> #define PCI_VFIO(obj) OBJECT_CHECK(VFIOPCIDevice, obj,
> > >> TYPE_VFIO_PCI)
> > >> @@ -2407,11 +2408,105 @@ static Object *vfio_pci_get_object(VFIODevice
> > >> *vbasedev)
> > >> return OBJECT(vdev);
> > >> }
> > >>
> > >> +static void vfio_pci_save_config(VFIODevice *vbasedev, QEMUFile *f)
> > >> +{
> > >> + VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice,
> > >> vbasedev);
> > >> + PCIDevice *pdev = &vdev->pdev;
> > >> +
> > >> + qemu_put_buffer(f, vdev->emulated_config_bits, vdev->config_size);
> > >> + qemu_put_buffer(f, vdev->pdev.wmask, vdev->config_size);
> > >> + pci_device_save(pdev, f);
> > >> +
> > >> + qemu_put_be32(f, vdev->interrupt);
> > >> + if (vdev->interrupt == VFIO_INT_MSIX) {
> > >> + msix_save(pdev, f);
> > >
> > > msix_save() checks msix_present() so shouldn't we include this
> > > unconditionally? Can't there also be state in the vector table
> > > regardless of whether we're currently running in MSI-X mode?
> > >
> > >> + }
> > >> +}
> > >> +
> > >> +static int vfio_pci_load_config(VFIODevice *vbasedev, QEMUFile *f)
> > >> +{
> > >> + VFIOPCIDevice *vdev = container_of(vbasedev, VFIOPCIDevice,
> > >> vbasedev);
> > >> + PCIDevice *pdev = &vdev->pdev;
> > >> + uint32_t interrupt_type;
> > >> + uint16_t pci_cmd;
> > >> + int i, ret;
> > >> +
> > >> + qemu_get_buffer(f, vdev->emulated_config_bits, vdev->config_size);
> > >> + qemu_get_buffer(f, vdev->pdev.wmask, vdev->config_size);
> > >
> > > This doesn't seem safe, why is it ok to indiscriminately copy these
> > > arrays that are configured via support or masking of various device
> > > features from the source to the target?
> > >
> >
> > Ideally, software state at host should be restrored at destination -
> > this is the attempt to do that.
>
> Or is it the case that both source and target should initialize these
> and come up with the same result and they should be used for
> validation, not just overwriting the target with the source?
Is the request to have something similar to get_pci_config_device's
check where it compares the configs and c/w/w1c masks (see
hw/pci/pci.c:520 ish) - we get errors like:
Bad config data: i=0x.... read: ... device: ... cmask...
this is pretty good at spotting things where the source and destination
device are configured differently, but to allow other dynamic
configuration values to be passed through OK.
Dave
> > > I think this still fails basic feature support negotiation. For
> > > instance, Intel IGD assignment modifies emulated_config_bits and wmask
> > > to allow the VM BIOS to allocate fake stolen memory for the GPU and
> > > store this value in config space. This support can be controlled via a
> > > QEMU build-time option, therefore the feature support on the target can
> > > be different from the source. If this sort of feature set doesn't
> > > match between source and target, I think we'd want to abort the
> > > migration, but we don't have any provisions for that here (a physical
> > > IGD device is obviously just an example as it doesn't support migration
> > > currently).
> > >
> >
> > Then is it ok not to include vdev->pdev.wmask? If yes, I'll remove it.
> > But we need vdev->emulated_config_bits to be restored.
>
> It's not clear why we need emulated_config_bits copied or how we'd
> handle the example I set forth above. The existence of emulation
> provided by QEMU is also emulation state.
>
>
> > >> +
> > >> + ret = pci_device_load(pdev, f);
> > >> + if (ret) {
> > >> + return ret;
> > >> + }
> > >> +
> > >> + /* retore pci bar configuration */
> > >> + pci_cmd = pci_default_read_config(pdev, PCI_COMMAND, 2);
> > >> + vfio_pci_write_config(pdev, PCI_COMMAND,
> > >> + pci_cmd & (!(PCI_COMMAND_IO |
> > >> PCI_COMMAND_MEMORY)), 2);
> > >
> > > s/!/~/? Extra parenthesis too
> > >
> > >> + for (i = 0; i < PCI_ROM_SLOT; i++) {
> > >> + uint32_t bar = pci_default_read_config(pdev,
> > >> + PCI_BASE_ADDRESS_0 + i *
> > >> 4, 4);
> > >> +
> > >> + vfio_pci_write_config(pdev, PCI_BASE_ADDRESS_0 + i * 4, bar, 4);
> > >> + }
> > >> +
> > >> + interrupt_type = qemu_get_be32(f);
> > >> +
> > >> + if (interrupt_type == VFIO_INT_MSI) {
> > >> + uint32_t msi_flags, msi_addr_lo, msi_addr_hi = 0, msi_data;
> > >> + bool msi_64bit;
> > >> +
> > >> + /* restore msi configuration */
> > >> + msi_flags = pci_default_read_config(pdev,
> > >> + pdev->msi_cap +
> > >> PCI_MSI_FLAGS, 2);
> > >> + msi_64bit = (msi_flags & PCI_MSI_FLAGS_64BIT);
> > >> +
> > >> + vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
> > >> + msi_flags & (!PCI_MSI_FLAGS_ENABLE), 2);
> > >> +
> > >
> > > What if I migrate from a device with MSI support to a device without
> > > MSI support, or to a device with MSI support at a different offset, who
> > > is responsible for triggering a migration fault?
> > >
> >
> > Migration compatibility check should take care of that. If there is such
> > a big difference in hardware then other things would also fail.
>
>
> The division between what is our responsibility in QEMU and what we
> hope the vendor driver handles is not very clear imo. How do we avoid
> finger pointing when things break?
>
>
> > >> + msi_addr_lo = pci_default_read_config(pdev,
> > >> + pdev->msi_cap +
> > >> PCI_MSI_ADDRESS_LO, 4);
> > >> + vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_ADDRESS_LO,
> > >> + msi_addr_lo, 4);
> > >> +
> > >> + if (msi_64bit) {
> > >> + msi_addr_hi = pci_default_read_config(pdev,
> > >> + pdev->msi_cap +
> > >> PCI_MSI_ADDRESS_HI, 4);
> > >> + vfio_pci_write_config(pdev, pdev->msi_cap +
> > >> PCI_MSI_ADDRESS_HI,
> > >> + msi_addr_hi, 4);
> > >> + }
> > >> +
> > >> + msi_data = pci_default_read_config(pdev,
> > >> + pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
> > >> PCI_MSI_DATA_32),
> > >> + 2);
> > >> +
> > >> + vfio_pci_write_config(pdev,
> > >> + pdev->msi_cap + (msi_64bit ? PCI_MSI_DATA_64 :
> > >> PCI_MSI_DATA_32),
> > >> + msi_data, 2);
> > >> +
> > >> + vfio_pci_write_config(pdev, pdev->msi_cap + PCI_MSI_FLAGS,
> > >> + msi_flags | PCI_MSI_FLAGS_ENABLE, 2);
> > >> + } else if (interrupt_type == VFIO_INT_MSIX) {
> > >> + uint16_t offset;
> > >> +
> > >> + offset = pci_default_read_config(pdev,
> > >> + pdev->msix_cap + PCI_MSIX_FLAGS
> > >> + 1, 2);
> > >> + /* load enable bit and maskall bit */
> > >> + vfio_pci_write_config(pdev, pdev->msix_cap + PCI_MSIX_FLAGS + 1,
> > >> + offset, 2);
> > >> + msix_load(pdev, f);
> > >
> > > Isn't this ordering backwards, or at least less efficient? The config
> > > write will cause us to enable MSI-X; presumably we'd have nothing in
> > > the vector table though. Then msix_load() will write the vector
> > > and pba tables and trigger a use notifier for each vector. It seems
> > > like that would trigger a bunch of SET_IRQS ioctls as if the guest
> > > wrote individual unmasked vectors to the vector table, whereas if we
> > > setup the vector table and then enable MSI-X, we do it with one ioctl.
> > >
> >
> > Makes sense. Changing the order here.
> >
> > > Also same question as above, I'm not sure who is responsible for making
> > > sure both devices support MSI-X and that the capability exists at the
> > > same place on each. Repeat for essentially every capability. Are we
> > > leaning on the migration regions to fail these migrations before we get
> > > here? If so, should we be?
> > >
> > As I mentioned about it should be vendor drivers responsibility to have
> > compatibility check in that case.
>
>
> And we'd rather blindly assume the vendor driver included that
> requirement than to check for ourselves?
>
>
> > > Also, besides BARs, the command register, and MSI & MSI-X, there must
> > > be other places where the guest can write config data through to the
> > > device. pci_device_{save,load}() only sets QEMU's config space.
> > >
> >
> > From QEMU we can restore QEMU's software state. For mediated device,
> > emulated state at vendor driver should be maintained by vendor driver,
> > right?
>
> In this proposal we've determined that emulated_config_bits, wmask,
> emulated config space, and MSI/X state are part of QEMU's state that
> need to be transmitted to the target. It therefore shouldn't be
> difficult to imagine that adding support for another capability might
> involve QEMU emulation as well. How does the migration stream we've
> constructed here allow such emulation state to be included? For example
> we might have a feature like IGD where we can discern the
> incompatibility via differences in the emulated_config_bits and wmask,
> but that's not guaranteed.
>
> > > A couple more theoretical (probably not too distant) examples related
> > > to that; there's a resizable BAR capability that at some point we'll
> > > probably need to allow the guest to interact with (ie. manipulation of
> > > capability changes the reported region size for a BAR). How would we
> > > support that with this save/load scheme?
> >
> > Config space is saved at the start of stop-and-copy phase, that means
> > vCPUs are stopped. So QEMU's config space saved at this phase should
> > include the change. Will there be any other software state that would be
> > required to save/load?
>
>
> There might be, it seems inevitable that there would eventually be
> something that needs emulation state beyond this initial draft. Is
> this resizable BAR example another that we simply hand wave as the
> responsibility of the vendor driver?
>
>
> > > We'll likely also have SR-IOV
> > > PFs assigned where we'll perhaps have support for emulating the SR-IOV
> > > capability to call out to a privileged userspace helper to enable VFs,
> > > how does this get extended to support that type of emulation?
> > >
> > > I'm afraid that making carbon copies of emulated_config_bits, wmask,
> > > and invoking pci_device_save/load() doesn't address my concerns that
> > > saving and restoring config space between source and target really
> > > seems like a much more important task than outlined here. Thanks,
> > >
> >
> > Are you suggesting to load config space using vfio_pci_write_config()
> > from PCI_CONFIG_HEADER_SIZE to
> > PCI_CONFIG_SPACE_SIZE/PCIE_CONFIG_SPACE_SIZE? I was kind of avoiding it.
>
> I don't think we can do that, even the save/restore functions in the
> kernel only blindly overwrite the standard header and then use
> capability specific functions elsewhere. But I think what is missing
> here is the ability to hook in support for manipulating specific
> capabilities on save and restore, which might include QEMU emulation
> state data outside of what's provided here. Thanks,
>
> Alex
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
[PATCH QEMU v25 04/17] vfio: Add migration region initialization and finalize function, Kirti Wankhede, 2020/06/20
[PATCH QEMU v25 05/17] vfio: Add VM state change handler to know state of VM, Kirti Wankhede, 2020/06/20
Re: [PATCH QEMU v25 05/17] vfio: Add VM state change handler to know state of VM, Cornelia Huck, 2020/06/23
[PATCH QEMU v25 06/17] vfio: Add migration state change notifier, Kirti Wankhede, 2020/06/20