Re: [Qemu-devel] [PATCH 1/5] VFIO KABI for migration interface

[Tian, Kevin]

> From: Kirti Wankhede [mailto:address@hidden
> Sent: Wednesday, November 21, 2018 12:24 PM
> 
> 
> On 11/21/2018 5:56 AM, Tian, Kevin wrote:
> >> From: Kirti Wankhede [mailto:address@hidden
> >> Sent: Wednesday, November 21, 2018 4:40 AM
> >>
> >> - Defined MIGRATION region type and sub-type.
> >> - Defined VFIO device states during migration process.
> >> - Defined vfio_device_migration_info structure which will be placed at
> 0th
> >>   offset of migration region to get/set VFIO device related information.
> >>   Defined actions and members of structure usage for each action:
> >>     * To convey VFIO device state to be transitioned to.
> >>     * To get pending bytes yet to be migrated for VFIO device
> >>     * To ask driver to write data to migration region and return number of
> >> bytes
> >>       written in the region
> >>     * In migration resume path, user space app writes to migration region
> >> and
> >>       communicates it to vendor driver.
> >>     * Get bitmap of dirty pages from vendor driver from given start
> address
> >>
> >> Signed-off-by: Kirti Wankhede <address@hidden>
> >> Reviewed-by: Neo Jia <address@hidden>
> >> ---
> >>  linux-headers/linux/vfio.h | 130
> >> +++++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 130 insertions(+)
> >>
> >> diff --git a/linux-headers/linux/vfio.h b/linux-headers/linux/vfio.h
> >> index 3615a269d378..a6e45cb2cae2 100644
> >> --- a/linux-headers/linux/vfio.h
> >> +++ b/linux-headers/linux/vfio.h
> >> @@ -301,6 +301,10 @@ struct vfio_region_info_cap_type {
> >>  #define VFIO_REGION_SUBTYPE_INTEL_IGD_HOST_CFG    (2)
> >>  #define VFIO_REGION_SUBTYPE_INTEL_IGD_LPC_CFG     (3)
> >>
> >> +/* Migration region type and sub-type */
> >> +#define VFIO_REGION_TYPE_MIGRATION                (1 << 30)
> >> +#define VFIO_REGION_SUBTYPE_MIGRATION             (1)
> >> +
> >>  /*
> >>   * The MSIX mappable capability informs that MSIX data of a BAR can be
> >> mmapped
> >>   * which allows direct access to non-MSIX registers which happened to
> be
> >> within
> >> @@ -602,6 +606,132 @@ struct vfio_device_ioeventfd {
> >>
> >>  #define VFIO_DEVICE_IOEVENTFD             _IO(VFIO_TYPE, VFIO_BASE
> >> + 16)
> >>
> >> +/**
> >> + * VFIO device states :
> >> + * VFIO User space application should set the device state to indicate
> >> vendor
> >> + * driver in which state the VFIO device should transitioned.
> >> + * - VFIO_DEVICE_STATE_NONE:
> >> + *   State when VFIO device is initialized but not yet running.
> >> + * - VFIO_DEVICE_STATE_RUNNING:
> >> + *   Transition VFIO device in running state, that is, user space
> application
> >> or
> >> + *   VM is active.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_SETUP:
> >> + *   Transition VFIO device in migration setup state. This is used to
> prepare
> >> + *   VFIO device for migration while application or VM and vCPUs are
> still in
> >> + *   running state.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_PRECOPY:
> >> + *   When VFIO user space application or VM is active and vCPUs are
> >> running,
> >> + *   transition VFIO device in pre-copy state.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_STOPNCOPY:
> >> + *   When VFIO user space application or VM is stopped and vCPUs are
> >> halted,
> >> + *   transition VFIO device in stop-and-copy state.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_SAVE_COMPLETED:
> >> + *   When VFIO user space application has copied data provided by
> vendor
> >> driver.
> >> + *   This state is used by vendor driver to clean up all software state 
> >> that
> >> was
> >> + *   setup during MIGRATION_SETUP state.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_RESUME:
> >> + *   Transition VFIO device to resume state, that is, start resuming VFIO
> >> device
> >> + *   when user space application or VM is not running and vCPUs are
> >> halted.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_RESUME_COMPLETED:
> >> + *   When user space application completes iterations of providing
> device
> >> state
> >> + *   data, transition device in resume completed state.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_FAILED:
> >> + *   Migration process failed due to some reason, transition device to
> >> failed
> >> + *   state. If migration process fails while saving at source, resume
> device
> >> at
> >> + *   source. If migration process fails while resuming application or VM
> at
> >> + *   destination, stop restoration at destination and resume at source.
> >> + * - VFIO_DEVICE_STATE_MIGRATION_CANCELLED:
> >> + *   User space application has cancelled migration process either for
> some
> >> + *   known reason or due to user's intervention. Transition device to
> >> Cancelled
> >> + *   state, that is, resume device state as it was during running state at
> >> + *   source.
> >> + */
> >> +
> >> +enum {
> >> +    VFIO_DEVICE_STATE_NONE,
> >> +    VFIO_DEVICE_STATE_RUNNING,
> >> +    VFIO_DEVICE_STATE_MIGRATION_SETUP,
> >> +    VFIO_DEVICE_STATE_MIGRATION_PRECOPY,
> >> +    VFIO_DEVICE_STATE_MIGRATION_STOPNCOPY,
> >> +    VFIO_DEVICE_STATE_MIGRATION_SAVE_COMPLETED,
> >> +    VFIO_DEVICE_STATE_MIGRATION_RESUME,
> >> +    VFIO_DEVICE_STATE_MIGRATION_RESUME_COMPLETED,
> >> +    VFIO_DEVICE_STATE_MIGRATION_FAILED,
> >> +    VFIO_DEVICE_STATE_MIGRATION_CANCELLED,
> >> +};
> >
> > We discussed in KVM forum to define the interfaces around the state
> > itself, instead of around live migration flow. Looks this version doesn't
> > move that way?
> >
> 
> This is patch series is along the discussion we had at KVM forum.
> 
> > quote the summary from Alex, which though high level but simple
> > enough to demonstrate the idea:
> >
> > --
> > Here we would define "registers" for putting the device in various
> > states through the migration process, for example enabling dirty logging,
> > suspending the device, resuming the device, direction of data flow
> > through the device state area, etc.
> > --
> >
> 
> Defined a packed structure here to map it at 0th offset of migration
> region so that offset can be calculated by offset_of(), you may call
> same as register definitions.

yes, this part is a good change. My comment was around state definition
itself.

> 
> 
> > based on that we just need much fewer states, e.g. {RUNNING,
> > RUNNING_DIRTYLOG, STOPPED}. data flow direction doesn't need
> > to be a state. could just a flag in the region.
> 
> Flag is not preferred here, multiple flags can be set at a time.
> Here need finite states with its proper definition what that device
> state means to driver and user space application.
> For Intel or others who don't need other states can ignore the state in
> driver by taking no action on pwrite on .device_state offset. For
> example for Intel driver could only take action on state change to
> VFIO_DEVICE_STATE_RUNNING and
> VFIO_DEVICE_STATE_MIGRATION_STOPNCOPY
> 
> I think dirty page logging is not a VFIO device's state.
> .log_sync of MemoryListener is called during both :
> - PRECOPY phase i.e. while vCPUs are still running and
> - during STOPNCOPY phase i.e. when vCPUs are stopped.
> 
> 
> > Those are sufficient to
> > enable vGPU live migration on Intel platform. nvidia or other vendors
> > may have more requirements, which could lead to addition of new
> > states - but again, they should be defined in a way not tied to migration
> > flow.
> >
> 
> I had tried to explain the intend of each state. Please go through the
> comments above.
> Also please take a look at other patches, mainly
> 0003-Add-migration-functions-for-VFIO-devices.patch to understand why
> these states are required.
> 

I looked at the explanations in this patch, but still didn't get the intention, 
e.g.:

+ * - VFIO_DEVICE_STATE_MIGRATION_SETUP:
+ *   Transition VFIO device in migration setup state. This is used to prepare
+ *   VFIO device for migration while application or VM and vCPUs are still in
+ *   running state.

what preparation is actually required? any example?

+ * - VFIO_DEVICE_STATE_MIGRATION_PRECOPY:
+ *   When VFIO user space application or VM is active and vCPUs are running,
+ *   transition VFIO device in pre-copy state.

why does device driver need know this stage? in precopy phase, the VM
is still running. Just dirty page tracking is in progress. the dirty bitmap 
could
be retrieved through its own action interface.

you have code to demonstrate how those states are transitioned in Qemu,
but you didn't show evidence why those states are necessary in device side,
which leads to the puzzle whether the definition is over-killed and limiting.

the flow in my mind is like below:

1. an interface to turn on/off dirty page tracking on VFIO device:
        * vendor driver can do whatever required to enable device specific
dirty page tracking mechanism here
        * device state is not changed here. still in running state

2. an interface to get dirty page bitmap

3. an interface to start/stop device activity
        * the effect of stop is to stop and drain in-the-fly device activities 
and
make device state ready for dump-out. vendor driver can do specific preparation 
here
        * the effect of start is to check validity of device state and then 
resume
device activities. again, vendor driver can do specific cleanup/preparation here

4. an interface to save/restore device state
        * should happen when device is stopped
        * of course there is still an open how to check state compatibility as
Alex pointed earlier

this way above interfaces are not tied to migration. other usages which are
interested in device state could also work (e.g. snapshot). If it doesn't work
with your device, it's better that you can elaborate your requirement with more
concrete examples. Then people will judge the necessity of a more complex
interface as proposed in this series...

Thanks
Kevin