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

[Steven Sistare]

On 1/6/2022 4:12 AM, Michael S. Tsirkin wrote:
> 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.

Cool.  After comparing pci_do_device_reset to pci_qdev_realize -> 
do_pci_register_device,
I concur.

This will do the trick.  Mode is set before reset is called, and cleared in 
cpr-load.

--------------------------
#include "migration/cpr.h"

static void pci_do_device_reset(PCIDevice *dev)
{
    int r;

    /*
     * A PCI device that is resuming for cpr is already configured, so do
     * not reset it here when we are called from qemu_system_reset prior to
     * cpr-load, else interrupts may be lost for vfio-pci devices.  It is
     * safe to skip this reset for all PCI devices, because cpr-load will set
     * all fields that would have been set here.
     */
    if (cpr_get_mode() == CPR_MODE_RESTART) {
        return;
    }
-----------------------------

- Steve

>>> 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
>