Re: [PATCH v5 2/5] drm/xe/vf: Document SRIOV VF restore flow

["Lis, Tomasz" <tomasz.lis@intel.com>]

On 03.11.2024 22:21, Michal Wajdeczko wrote:
>
> On 29.10.2024 20:39, Tomasz Lis wrote:
>> This adds a documentation chapter, containing high level flow
>> of VF restore procedure.
>>
>> Signed-off-by: Tomasz Lis <tomasz.lis@intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_sriov_vf.h | 93 ++++++++++++++++++++++++++++++++
>>   1 file changed, 93 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_vf.h b/drivers/gpu/drm/xe/xe_sriov_vf.h
>> index 7b8622cff2b7..22683dd643f8 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_vf.h
>> +++ b/drivers/gpu/drm/xe/xe_sriov_vf.h
>> @@ -8,6 +8,99 @@
>>   
>>   struct xe_device;
>>   
>> +/**
>> + * DOC: VF restore procedure in PF KMD and VF KMD
>> + *
>> + * Restoring previously saved state of a VF is one of core features of
>> + * SR-IOV. All major VM Management applications allow saving and restoring
>> + * the VM state, and doing that to a VM which uses SRIOV VF as one of
>> + * the accessible devices requires support from KMD on both PF and VF side.
>> + * VMM initiates all required operations through VFIO module, which then
>> + * translates them into PF KMD calls. This description will focus on these
>> + * calls, leaving out the module which initiates these steps (VFIO).
>> + *
>> + * In order to start the restore procedure, GuC needs to keep the VF in
>> + * proper state. The PF driver can ensure VF entered this state by
>> + * performing Function Level Reset on said VF. The FLR procedure ends with
>> + * GuC sending message `GUC_PF_NOTIFY_VF_FLR_DONE`, and then the actual
>> + * restore can begin.
> hmm, I would do some rewording here, as it's not that VF_FLR_DONE
> message is a prerequisite for the restore flow, it's that VF must be in
> VF_READY state, which will be once the VF is successfully provisioned.
>
> The FLR is required only if the VF was previously used.
Will do. Though for what I understand, creation of a VF includes a FLR 
as well.
>
>> + *
>> + * During VF Restore, state of several resources is restored. These may
>> + * include local memory content (system memory is restored by VMM itself),
>> + * values of MMIO registers, stateless compression metadata and others.
>> + * The final resource which also needs restoring is state of the VF
>> + * submission maintained within GuC. For that, `GUC_PF_OPCODE_VF_RESTORE`
>> + * message is used, with reference to the state blob to be consumed by
>> + * GuC.
>> + *
>> + * Next, when VFIO is asked to set the VM into running state, the PF driver
>> + * sends `GUC_PF_TRIGGER_VF_RESUME` to GuC. When sent after restore, this
>> + * changes VF state within GuC to `VF_RESFIX_BLOCKED` rather than the
>> + * usual `VF_RUNNING`. At this point GuC triggers an interrupt to inform
>> + * the VF KMD within the VM that it was migrated.
>> + *
>> + * As soon as Virtual GPU of the VM starts, the VF driver within receives
>> + * the MIGRATED interrupt and schedules post-migration recovery worker.
>> + * That worker queries GuC for new provisioning (using MMIO communication),
>> + * and applies fixups to any non-virtualized resources used by the VF.
>> + *
>> + * When the VF driver is ready to continue operation on the newly connected
>> + * hardware, it sends `VF2GUC_NOTIFY_RESFIX_DONE` which causes it to
>> + * enter the long awaited `VF_RUNNING` state, and therefore start handling
>> + * CTB messages and scheduling workloads from the VF::
>> + *
> nit: maybe this would look little nicer with leading lines like:

no, it's not a matter of nice look. The lines start thin if the 
component does not currently participate in the execution.

Since it's not a proper diagram and just simplified ASCII art, we do not 
start the flow correctly with an actor initiating everything. So 
starting the lines thin would misrepresent the flow.

>
>> + *      PF                             GuC                              VF
>             |                               |
>> + *     [ ]                             [ ]                              |
>> + *     [ ] GUC_PF_NOTIFY_VF_FLR_DONE   [ ]                              |
> nit: this label should be aligned to the right, closer to GuC
this is a "return" label. These labels are bound to the position of arrow.
>
>> + *     [ ] <---------------------------[ ]                              |
> but IMO the whole diagram should start here, as it's about the restore
> flow, not the FLR flow (which is just a prerequisite, like a VF
> re-provisioning, that also must happen before the restore)
>
> if you really want to include prerequisite steps in diagram then maybe
> do it like this:
>
>            PF                             GuC
>             |                               |
>            [ ] VF provisioning/FLR          |
>            [ ] --------------------------> [ ]
>            [ ]                             [ ]--- VF_READY
>            [ ]                             [ ]<---|
>             |                               |

This is not ok because provisioning would need return arrow, it's not a 
one-way notify.

Will just delete.

>
>
>> + *     [ ]                              |                               |
>> + *     [ ] PF loads resources from the  |                               |
>> + *     [ ]------- saved image supplied  |                               |
>> + *     [ ]      |                       |                               |
>> + *     [ ] <-----                       |                               |
>> + *     [ ]                              |                               |
>> + *     [ ] GUC_PF_OPCODE_VF_RESTORE     |                               |
>> + *     [ ]---------------------------> [ ]                              |
>> + *     [ ]                             [ ]  GuC loads contexts and CTB  |
>> + *     [ ]                             [ ]------- state from image      |
> maybe also say that GuC moves VF to VF_RESFIX_PAUSED state?

The documentation is to explain kernel workings, not GuC states. So I 
don't really like putting all the GuC states here, especially because 
KMD doesn't even track all of them.

..But it does make sense at this point, especially because I will have 
to add VF_READY_PAUSED as well. So, will add.

>
>> + *     [ ]                             [ ]      |                       |
>> + *     [ ] success                     [ ] <-----                       |
>> + *     [ ] <---------------------------[ ]                              |
>> + *     [ ]                              |                               |
>> + *     [ ] GUC_PF_TRIGGER_VF_RESUME     |                               |
>> + *     [ ]---------------------------> [ ]                              |
>> + *     [ ]                             [ ]  GuC sets new VF state to    |
>> + *     [ ]                             [ ]------- VF_RESFIX_BLOCKED     |
>> + *     [ ]                             [ ]      |                       |
>> + *     [ ]                             [ ] <-----                       |
>> + *     [ ]                             [ ]                              |
>> + *     [ ]                             [ ] GUC_INTR_SW_INT_0            |
>> + *     [ ] success                     [ ]---------------------------> [ ]
>> + *     [ ] <---------------------------[ ]                             [ ]
>> + *      |                               |      VF2GUC_QUERY_SINGLE_KLV [ ]
>> + *      |                              [ ] <---------------------------[ ]
>> + *      |                              [ ]                             [ ]
>> + *      |                              [ ]        new VF provisioning  [ ]
>> + *      |                              [ ]---------------------------> [ ]
>> + *      |                               |                              [ ]
>> + *      |                               |       VF driver applies post [ ]
>> + *      |                               |      migration fixups -------[ ]
>> + *      |                               |                       |      [ ]
>> + *      |                               |                       -----> [ ]
>> + *      |                               |                              [ ]
>> + *      |                               |    VF2GUC_NOTIFY_RESFIX_DONE [ ]
>> + *      |                              [ ] <---------------------------[ ]
>> + *      |                              [ ]                             [ ]
>> + *      |                              [ ]  GuC sets new VF state to   [ ]
>> + *      |                              [ ]------- VF_READY             [ ]
> this should say VF_RUNNING state

ack

-Tomasz

>> + *      |                              [ ]      |                      [ ]
>> + *      |                              [ ] <-----                      [ ]
>> + *      |                              [ ]                     success [ ]
>> + *      |                              [ ]---------------------------> [ ]
>> + *      |                               |                               |
>> + *      |                               |                               |
>> + */
>> +
>>   void xe_sriov_vf_init_early(struct xe_device *xe);
>>   void xe_sriov_vf_start_migration_recovery(struct xe_device *xe);
>>