Re: [PATCH V7 19/29] vfio-pci: cpr part 1 (fd and dma)

[Michael S. Tsirkin]

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.sistare@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.sistare@oracle.com>
> >>>>>>  M: Mark Kanda <mark.kanda@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