Re: [RFC PATCH] vfio: Update/Clarify migration uAPI, add NDMA state

[Cornelia Huck <cohuck@redhat.com>]

On Mon, Dec 20 2021, Alex Williamson <alex.williamson@redhat.com> wrote:

> On Mon, 20 Dec 2021 18:38:26 +0100
> Cornelia Huck <cohuck@redhat.com> wrote:
>
>> On Thu, Dec 09 2021, Alex Williamson <alex.williamson@redhat.com> wrote:
>> 
>> > A new NDMA state is being proposed to support a quiescent state for
>> > contexts containing multiple devices with peer-to-peer DMA support.
>> > Formally define it.  
>> 
>> [I'm wondering if we would want to use NDMA in other cases as well. Just
>> thinking out loud below.]
>> 
>> >
>> > Clarify various aspects of the migration region data fields and
>> > protocol.  Remove QEMU related terminology and flows from the uAPI;
>> > these will be provided in Documentation/ so as not to confuse the
>> > device_state bitfield with a finite state machine with restricted
>> > state transitions.
>> >
>> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>> > ---
>> >  include/uapi/linux/vfio.h |  405 ++++++++++++++++++++++++---------------------
>> >  1 file changed, 214 insertions(+), 191 deletions(-)
>> >
>> > diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h
>> > index ef33ea002b0b..1fdbc928f886 100644
>> > --- a/include/uapi/linux/vfio.h
>> > +++ b/include/uapi/linux/vfio.h  
>> 
>> (...)
>> 
>> > + *   The device_state field defines the following bitfield use:
>> > + *
>> > + *     - Bit 0 (RUNNING) [REQUIRED]:
>> > + *        - Setting this bit indicates the device is fully operational, the
>> > + *          device may generate interrupts, DMA, respond to MMIO, all vfio
>> > + *          device regions are functional, and the device may advance its
>> > + *          internal state.  The default device_state must indicate the device
>> > + *          in exclusively the RUNNING state, with no other bits in this field
>> > + *          set.
>> > + *        - Clearing this bit (ie. !RUNNING) must stop the operation of the
>> > + *          device.  The device must not generate interrupts, DMA, or advance
>> > + *          its internal state.  The user should take steps to restrict access
>> > + *          to vfio device regions other than the migration region while the
>> > + *          device is !RUNNING or risk corruption of the device migration data
>> > + *          stream.  The device and kernel migration driver must accept and
>> > + *          respond to interaction to support external subsystems in the
>> > + *          !RUNNING state, for example PCI MSI-X and PCI config space.
>> > + *          Failure by the user to restrict device access while !RUNNING must
>> > + *          not result in error conditions outside the user context (ex.
>> > + *          host system faults).  
>> 
>> If I consider ccw, this would mean that user space would need to stop
>> writing to the regions that initiate start/halt/... when RUNNING is
>> cleared (makes sense) and that the subchannel must be idle or even
>> disabled (so that it does not become status pending). The question is,
>> does it make sense to stop new requests and wait for the subchannel to
>> become idle during the !RUNNING transition (or even forcefully kill
>> outstanding I/O), or...
>> 
>
>> > + *     - Bit 3 (NDMA) [OPTIONAL]:
>> > + *        The NDMA or "No DMA" state is intended to be a quiescent state for
>> > + *        the device for the purposes of managing multiple devices within a
>> > + *        user context where peer-to-peer DMA between devices may be active.
>> > + *        Support for the NDMA bit is indicated through the presence of the
>> > + *        VFIO_REGION_INFO_CAP_MIG_NDMA capability as reported by
>> > + *        VFIO_DEVICE_GET_REGION_INFO for the associated device migration
>> > + *        region.
>> > + *        - Setting this bit must prevent the device from initiating any
>> > + *          new DMA or interrupt transactions.  The migration driver must
>> > + *          complete any such outstanding operations prior to completing
>> > + *          the transition to the NDMA state.  The NDMA device_state
>> > + *          essentially represents a sub-set of the !RUNNING state for the
>> > + *          purpose of quiescing the device, therefore the NDMA device_state
>> > + *          bit is superfluous in combinations including !RUNNING.
>> > + *        - Clearing this bit (ie. !NDMA) negates the device operational
>> > + *          restrictions required by the NDMA state.  
>> 
>> ...should we use NDMA as the "stop new requests" state, but allow
>> running channel programs to conclude? I'm not entirely sure whether
>> that's in the spirit of NDMA (subchannels are independent of each
>> other), but it would be kind of "quiescing" already.
>> 
>> (We should probably clarify things like that in the Documentation/
>> file.)
>
> This bumps into the discussion in my other thread with Jason, we need
> to refine what NDMA means.  Based on my reply there and our previous
> discussion that QEMU could exclude p2p mappings to support VMs with
> multiple devices that don't support NDMA, I think that NDMA is only
> quiescing p2p traffic (if so, maybe should be NOP2P).  So this use of
> it seems out of scope to me.

Ok, makes sense. If the scope of this flag is indeed to be supposed
quite narrow, it might make sense to rename it.

>
> Userspace necessarily needs to stop vCPUs before stopping devices,
> which should mean that there are no new requests when a ccw device is
> transitioning to !RUNNING.  Therefore I'd expect that the transition to
> any !RUNNING state would wait from completion of running channel
> programs.

Indeed, it should not be any problem to do this for !RUNNING, I had just
been wondering about possible alternative implementations.