On Wed, Jan 05, 2022 at 06:24:25PM -0500, Steven Sistare wrote: > On 1/5/2022 6:09 PM, Michael S. Tsirkin wrote: > > On Wed, Jan 05, 2022 at 04:40:43PM -0500, Steven Sistare wrote: > >> On 1/5/2022 4:14 PM, Michael S. Tsirkin wrote: > >>> On Wed, Jan 05, 2022 at 12:24:21PM -0500, Steven Sistare wrote: > >>>> On 12/22/2021 6:15 PM, Michael S. Tsirkin wrote: > >>>>> On Wed, Dec 22, 2021 at 11:05:24AM -0800, Steve Sistare wrote: > >>>>>> Enable vfio-pci devices to be saved and restored across an exec restart > >>>>>> of qemu. > >>>>>> > >>>>>> At vfio creation time, save the value of vfio container, group, and > >>>>>> device > >>>>>> descriptors in cpr state. > >>>>>> > >>>>>> In cpr-save and cpr-exec, suspend the use of virtual addresses in DMA > >>>>>> mappings with VFIO_DMA_UNMAP_FLAG_VADDR, because guest ram will be > >>>>>> remapped > >>>>>> at a different VA after exec. DMA to already-mapped pages continues. > >>>>>> Save > >>>>>> the msi message area as part of vfio-pci vmstate, save the interrupt > >>>>>> and > >>>>>> notifier eventfd's in cpr state, and clear the close-on-exec flag for > >>>>>> the > >>>>>> vfio descriptors. The flag is not cleared earlier because the > >>>>>> descriptors > >>>>>> should not persist across miscellaneous fork and exec calls that may be > >>>>>> performed during normal operation. > >>>>>> > >>>>>> On qemu restart, vfio_realize() finds the saved descriptors, uses > >>>>>> the descriptors, and notes that the device is being reused. Device and > >>>>>> iommu state is already configured, so operations in vfio_realize that > >>>>>> would modify the configuration are skipped for a reused device, > >>>>>> including > >>>>>> vfio ioctl's and writes to PCI configuration space. The result is that > >>>>>> vfio_realize constructs qemu data structures that reflect the current > >>>>>> state of the device. However, the reconstruction is not complete until > >>>>>> cpr-load is called. cpr-load loads the msi data and finds eventfds in > >>>>>> cpr > >>>>>> state. It rebuilds vector data structures and attaches the interrupts > >>>>>> to > >>>>>> the new KVM instance. cpr-load then invokes the main vfio listener > >>>>>> callback, > >>>>>> which walks the flattened ranges of the vfio_address_spaces and calls > >>>>>> VFIO_DMA_MAP_FLAG_VADDR to inform the kernel of the new VA's. Lastly, > >>>>>> it > >>>>>> starts the VM and suppresses vfio pci device reset. > >>>>>> > >>>>>> This functionality is delivered by 3 patches for clarity. Part 1 > >>>>>> handles > >>>>>> device file descriptors and DMA. Part 2 adds eventfd and MSI/MSI-X > >>>>>> vector > >>>>>> support. Part 3 adds INTX support. > >>>>>> > >>>>>> Signed-off-by: Steve Sistare <steven.sist...@oracle.com> > >>>>>> --- > >>>>>> MAINTAINERS | 1 + > >>>>>> hw/pci/pci.c | 10 ++++ > >>>>>> hw/vfio/common.c | 115 > >>>>>> ++++++++++++++++++++++++++++++++++++++---- > >>>>>> hw/vfio/cpr.c | 94 ++++++++++++++++++++++++++++++++++ > >>>>>> hw/vfio/meson.build | 1 + > >>>>>> hw/vfio/pci.c | 77 ++++++++++++++++++++++++++++ > >>>>>> hw/vfio/trace-events | 1 + > >>>>>> include/hw/pci/pci.h | 1 + > >>>>>> include/hw/vfio/vfio-common.h | 8 +++ > >>>>>> include/migration/cpr.h | 3 ++ > >>>>>> migration/cpr.c | 10 +++- > >>>>>> migration/target.c | 14 +++++ > >>>>>> 12 files changed, 324 insertions(+), 11 deletions(-) > >>>>>> create mode 100644 hw/vfio/cpr.c > >>>>>> > >>>>>> diff --git a/MAINTAINERS b/MAINTAINERS > >>>>>> index cfe7480..feed239 100644 > >>>>>> --- a/MAINTAINERS > >>>>>> +++ b/MAINTAINERS > >>>>>> @@ -2992,6 +2992,7 @@ CPR > >>>>>> M: Steve Sistare <steven.sist...@oracle.com> > >>>>>> M: Mark Kanda <mark.ka...@oracle.com> > >>>>>> S: Maintained > >>>>>> +F: hw/vfio/cpr.c > >>>>>> F: include/migration/cpr.h > >>>>>> F: migration/cpr.c > >>>>>> F: qapi/cpr.json > >>>>>> diff --git a/hw/pci/pci.c b/hw/pci/pci.c > >>>>>> index 0fd21e1..e35df4f 100644 > >>>>>> --- a/hw/pci/pci.c > >>>>>> +++ b/hw/pci/pci.c > >>>>>> @@ -307,6 +307,16 @@ static void pci_do_device_reset(PCIDevice *dev) > >>>>>> { > >>>>>> int r; > >>>>>> > >>>>>> + /* > >>>>>> + * A reused vfio-pci device is already configured, so do not > >>>>>> reset it > >>>>>> + * during qemu_system_reset prior to cpr-load, else interrupts > >>>>>> may be > >>>>>> + * lost. By contrast, pure-virtual pci devices may be reset here > >>>>>> and > >>>>>> + * updated with new state in cpr-load with no ill effects. > >>>>>> + */ > >>>>>> + if (dev->reused) { > >>>>>> + return; > >>>>>> + } > >>>>>> + > >>>>>> pci_device_deassert_intx(dev); > >>>>>> assert(dev->irq_state == 0); > >>>>>> > >>>>> > >>>>> > >>>>> Hmm that's a weird thing to do. I suspect this works because > >>>>> "reused" means something like "in the process of being restored"? > >>>>> Because clearly, we do not want to skip this part e.g. when > >>>>> guest resets the device. > >>>> > >>>> Exactly. vfio_realize sets the flag if it detects the device is reused > >>>> during > >>>> a restart, and vfio_pci_post_load clears the reused flag. > >>>> > >>>>> So a better name could be called for, but really I don't > >>>>> love how vfio gets to poke at internal PCI state. > >>>>> I'd rather we found a way just not to call this function. > >>>>> If we can't, maybe an explicit API, and make it > >>>>> actually say what it's doing? > >>>> > >>>> How about: > >>>> > >>>> pci_set_restore(PCIDevice *dev) { dev->restore = true; } > >>>> pci_clr_restore(PCIDevice *dev) { dev->restore = false; } > >>>> > >>>> vfio_realize() > >>>> pci_set_restore(pdev) > >>>> > >>>> vfio_pci_post_load() > >>>> pci_clr_restore(pdev) > >>>> > >>>> pci_do_device_reset() > >>>> if (dev->restore) > >>>> return; > >>>> > >>>> - Steve > >>> > >>> > >>> Not too bad. I'd like a better definition of what dev->restore is > >>> exactly and to add them in comments near where it > >>> is defined and used. > >> > >> Will do. > >> > >>> E.g. does this mean "device is being restored because of qemu restart"? > >>> > >>> Do we need a per device flag for this thing or would a global > >>> "qemu restart in progress" flag be enough? > >> > >> A global flag (or function, which already exists) would suppress reset for > >> all > >> PCI devices, not just vfio-pci. I am concerned that for some devices, > >> vmstate > >> load may implicitly depend on the device having been reset for > >> correctness, by > >> virtue of some fields being initialized in the reset function. > >> > >> - Steve
I took a look and I don't really see any cases like this. I think pci_qdev_realize will initialize the pci core to a correct state, pci_do_device_reset isn't necessary right after realize. It seems safe to just skip it for all devices unconditionally. A bunch of devices do depend on reset to init them correctly, e.g. hw/ide/piix.c sets pci status in piix_ide_reset. But pci core does not seem to. > > So just so I understand, how do these other devices work with restart? > > Do they use the save/loadvm machinery? And the reason vfio doesn't > > is because it generally does not support savevm/loadvm? > > They all use save/loadvm. vfio-pci also uses save/loadvm to preserve its > soft state, > plus it preserves its device descriptors. The only bit we are skipping is > the reset > function for vfio-pci, because the hardware device is actively processing dma > and > interrupts, and they would be lost. Reset is called unconditionally for all > devices > during qemu startup, prior to loadvm, by the path > qdev_machine_creation_done() -> > qemu_system_reset(). > > - Steve