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

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-kwi-15 11:01:21 +0200]:
> 
> 
> 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)

Additionally, these bits are reserved for GRAPHICS_VER < 12.50 anyway.

> 
> > 
> >> +
> >>   #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
> 
> > 
> >> + */
> >> +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

Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

--