Re: [PATCH 2/6] drm/xe: Allow to assign GGTT region to the VF

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 4868 bytes --]


On 15-04-2024 14:31, Michal Wajdeczko wrote:
>
> On 15.04.2024 06:49, Ghimiray, Himal Prasad wrote:
>> On 15-04-2024 00:31, Michal Wajdeczko wrote:
>>> VF's drivers can't modify GGTT PTEs except the range explicitly
>>> assigned by the PF driver. To allow hardware enforcement of this
>>> requirement, each GGTT PTE has a field with the VF number that
>>> identifies which VF can modify that particular GGTT PTE entry.
>>>
>>> Only PF driver can modify this field and PF driver shall do that
>>> before VF drivers will be loaded. Add function to prepare PTEs.
>>> Since it will be used only by the PF driver, make it available
>>> only for CONFIG_PCI_IOV=y.
>>>
>>> Bspec: 45015
>>> Signed-off-by: Michal Wajdeczko<michal.wajdeczko@intel.com>
>>> ---
>>>    drivers/gpu/drm/xe/regs/xe_gtt_defs.h |  2 ++
>>>    drivers/gpu/drm/xe/xe_ggtt.c          | 44 +++++++++++++++++++++++++++
>>>    drivers/gpu/drm/xe/xe_ggtt.h          |  4 +++
>>>    3 files changed, 50 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/xe/regs/xe_gtt_defs.h
>>> b/drivers/gpu/drm/xe/regs/xe_gtt_defs.h
>>> index 558519ce48c7..4389e5a76f89 100644
>>> --- a/drivers/gpu/drm/xe/regs/xe_gtt_defs.h
>>> +++ b/drivers/gpu/drm/xe/regs/xe_gtt_defs.h
>>> @@ -9,6 +9,8 @@
>>>    #define XELPG_GGTT_PTE_PAT0    BIT_ULL(52)
>>>    #define XELPG_GGTT_PTE_PAT1    BIT_ULL(53)
>>>    +#define GGTT_PTE_VFID        GENMASK_ULL(11, 2)
>> Patch looks good to me.
>>
>> Just to confrim, based on BSPEC, this MASK applies to GRAPHICS_VER >=
>> 12.50. For versions before this, the applicable mask is (4, 2). I infer
>> that since SRIOV support on XE is expected beyond GRAPHICS_VER version
>> 12.50, you are using mask (11, 2).
> true, but even on earlier versions, which we may want to use as SDV,
> where real mask is (4, 2) this extended mask (11, 2) should be still
> harmless as number of VFs would be then always less than 7 so it will
> fit only into (4, 2)
Thanks for confirming.
>
>>> +
>>>    #define GUC_GGTT_TOP        0xFEE00000
>>>      #define XELPG_PPGTT_PTE_PAT3        BIT_ULL(62)
>>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
>>> index ff2239c0eda5..f090cab065b8 100644
>>> --- a/drivers/gpu/drm/xe/xe_ggtt.c
>>> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
>>> @@ -460,6 +460,50 @@ void xe_ggtt_remove_bo(struct xe_ggtt *ggtt,
>>> struct xe_bo *bo)
>>>                    bo->flags & XE_BO_FLAG_GGTT_INVALIDATE);
>>>    }
>>>    +#ifdef CONFIG_PCI_IOV
>>> +static u64 xe_encode_vfid_pte(u16 vfid)
>>> +{
>>> +    return FIELD_PREP(GGTT_PTE_VFID, vfid) | XE_PAGE_PRESENT;
>>> +}
>>> +
>>> +static void xe_ggtt_assign_locked(struct xe_ggtt *ggtt, const struct
>>> drm_mm_node *node, u16 vfid)
>>> +{
>>> +    u64 start = node->start;
>>> +    u64 size = node->size;
>>> +    u64 end = start + size - 1;
>>> +    u64 pte = xe_encode_vfid_pte(vfid);
>>> +
>>> +    lockdep_assert_held(&ggtt->lock);
>>> +
>>> +    if (!drm_mm_node_allocated(node))
>>> +        return;
>>> +
>>> +    while (start < end) {
>>> +        xe_ggtt_set_pte(ggtt, start, pte);
>>> +        start += XE_PAGE_SIZE;
>>> +    }
>>> +
>>> +    xe_ggtt_invalidate(ggtt);
>>> +}
>>> +
>>> +/**
>>> + * xe_ggtt_assign - assign a GGTT region to the VF
>>> + * @ggtt: the &xe_ggtt where the node belongs
>>> + * @node: the &drm_mm_node to update
>>> + * @vfid: the VF identifier
>>> + *
>>> + * This function is used by the PF driver to assign a GGTT region to
>>> the VF.
>>> + * In addition to PTE's VFID bits 11:2 also PRESENT bit 0 is set as
>>> on some
>>> + * platforms VFs can't modify that either.
>> This info regarding bit 0 not being modifiable by VF's on some platforms
>> is missing in Bspec.
> Piotr reminded me of another page:
>
> Bspec: 52395

Thanks.

LGTM

Reviewed-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> 
<mailto:himal.prasad.ghimiray@intel.com>

>
>>> + */
>>> +void xe_ggtt_assign(struct xe_ggtt *ggtt, const struct drm_mm_node
>>> *node, u16 vfid)
>>> +{
>>> +    mutex_lock(&ggtt->lock);
>>> +    xe_ggtt_assign_locked(ggtt, node, vfid);
>>> +    mutex_unlock(&ggtt->lock);
>>> +}
>>> +#endif
>>> +
>>>    int xe_ggtt_dump(struct xe_ggtt *ggtt, struct drm_printer *p)
>>>    {
>>>        int err;
>>> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
>>> index 8306ef74abc6..4a41a1762358 100644
>>> --- a/drivers/gpu/drm/xe/xe_ggtt.h
>>> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
>>> @@ -33,4 +33,8 @@ void xe_ggtt_remove_bo(struct xe_ggtt *ggtt, struct
>>> xe_bo *bo);
>>>      int xe_ggtt_dump(struct xe_ggtt *ggtt, struct drm_printer *p);
>>>    +#ifdef CONFIG_PCI_IOV
>>> +void xe_ggtt_assign(struct xe_ggtt *ggtt, const struct drm_mm_node
>>> *node, u16 vfid);
>>> +#endif
>>> +
>>>    #endif

[-- Attachment #2: Type: text/html, Size: 41876 bytes --]