[PATCH] drm/xe/pf: Clear all LMTT pages on alloc

[Piotr Piórkowski]

Michal Wajdeczko <michal.wajdeczko at intel.com> wrote on śro [2025-lip-02 11:37:08 +0200]:
> 
> 
> On 02.07.2025 10:02, Piotr Piórkowski wrote:
> > Michal Wajdeczko <michal.wajdeczko at intel.com> wrote on śro [2025-lip-02 00:00:52 +0200]:
> >> Our LMEM buffer objects are not cleared by default on alloc
> >> and during VF provisioning we only setup LMTT PTEs for the
> >> actually provisioned LMEM range. But beyond that valid range
> >> we might leave some stale data that could either point to some
> >> other VFs allocations or even to the PF pages.
> >>
> >> Explicitly clear all new LMTT page to avoid the risk that a
> >> malicious VF would try to exploit that gap.
> >>
> >> While around add asserts to catch any undesired PTE overwrites
> >> and low-level debug traces to track LMTT PT life-cycle.
> >>
> >> Fixes: b1d204058218 ("drm/xe/pf: Introduce Local Memory Translation Table")
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
> >> Cc: Michał Winiarski <michal.winiarski at intel.com>
> >> Cc: Lukasz Laguna <lukasz.laguna at intel.com>
> >> ---
> >>  drivers/gpu/drm/xe/xe_lmtt.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/xe/xe_lmtt.c b/drivers/gpu/drm/xe/xe_lmtt.c
> >> index b56437a816e4..381f576036d0 100644
> >> --- a/drivers/gpu/drm/xe/xe_lmtt.c
> >> +++ b/drivers/gpu/drm/xe/xe_lmtt.c
> >> @@ -78,6 +78,9 @@ static struct xe_lmtt_pt *lmtt_pt_alloc(struct xe_lmtt *lmtt, unsigned int level
> >>  	}
> >>  
> >>  	lmtt_assert(lmtt, xe_bo_is_vram(bo));
> >> +	lmtt_debug(lmtt, "level=%u addr=%#llx\n", level, (u64)xe_bo_main_addr(bo, XE_PAGE_SIZE));
> >> +
> >> +	xe_map_memset(lmtt_to_xe(lmtt), &bo->vmap, 0, 0, xe_bo_size(bo));
> > 
> > We have a dedicated macro LMTT_PTE_INVALID
> > Maybe it's worth using it here.
> 
> this would require to introduce  {xe,iosys}_map_memset64() helpers
> 
> but at this point we only care about BO being zeroed, not filled with
> any specific value (like LMTT_PTE_INVALID which accidentally is also 0)
> 
> > 
> > I wonder why we care about cleaning LMTT only during allocation and
> > not in lmtt_drop_pages and lmtt_destroy_pt.
> 
> because at lmtt_drop_pages() we explicitly remove all references to
> existing PTs by resetting root entry at the PD level, so HW can't walk
> those pages anymore
> 
> > To be precise, in lmtt_drop_pages we do lmtt_write_pte(lmtt, pd, LMTT_PTE_INVALID, vfid),
> > but from what I understand, this is missing at the lmtt_destroy_pt level.
> 
> the designed flow is:
>  - call xe_lmtt_prepare_pages() to build PT tree with clear leaf PTs
>  - call xe_lmtt_populate_pages() to populate leaf PT with PTEs
>  - call xe_lmtt_populate_pages() to populate leaf PT with PTEs
>  - ...
>  - call xe_lmtt_drop_pages() to remove whole PT tree
> 
> since currently there is no way to selectively unmap any address range
> or change the back-end BOs, thus there is no point in doing selective
> clear of any PTE, except at the top level PD, which we do

Thanks for your explanation
In that case, it looks fine to me:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski at intel.com>

> 
> > 
> > Thanks,
> > Piotr
> >>  
> >>  	pt->level = level;
> >>  	pt->bo = bo;
> >> @@ -91,6 +94,9 @@ static struct xe_lmtt_pt *lmtt_pt_alloc(struct xe_lmtt *lmtt, unsigned int level
> >>  
> >>  static void lmtt_pt_free(struct xe_lmtt_pt *pt)
> >>  {
> >> +	lmtt_debug(&pt->bo->tile->sriov.pf.lmtt, "level=%u addr=%llx\n",
> >> +		   pt->level, (u64)xe_bo_main_addr(pt->bo, XE_PAGE_SIZE));
> >> +
> >>  	xe_bo_unpin_map_no_vm(pt->bo);
> >>  	kfree(pt);
> >>  }
> >> @@ -226,9 +232,14 @@ static void lmtt_write_pte(struct xe_lmtt *lmtt, struct xe_lmtt_pt *pt,
> >>  
> >>  	switch (lmtt->ops->lmtt_pte_size(level)) {
> >>  	case sizeof(u32):
> >> +		lmtt_assert(lmtt, !overflows_type(pte, u32));
> >> +		lmtt_assert(lmtt, !pte || !iosys_map_rd(&pt->bo->vmap, idx * sizeof(u32), u32));
> >> +
> >>  		xe_map_wr(lmtt_to_xe(lmtt), &pt->bo->vmap, idx * sizeof(u32), u32, pte);
> >>  		break;
> >>  	case sizeof(u64):
> >> +		lmtt_assert(lmtt, !pte || !iosys_map_rd(&pt->bo->vmap, idx * sizeof(u64), u64));
> >> +
> >>  		xe_map_wr(lmtt_to_xe(lmtt), &pt->bo->vmap, idx * sizeof(u64), u64, pte);
> >>  		break;
> >>  	default:
> >> -- 
> >> 2.47.1
> >>
> > 
> 

--