Re: [PATCH] drm/xe: Add warn when level can not be zero.

[Matthew Brost <matthew.brost@intel.com>]

On Tue, May 21, 2024 at 03:52:45PM +0100, Matthew Auld wrote:
> On 21/05/2024 15:08, Matthew Brost wrote:
> > On Tue, May 21, 2024 at 12:36:23PM +0200, Nirmoy Das wrote:
> > > At xe_pt_zap_ptes_entry() and xe_pt_stage_unbind_entry, the level cannot
> > > be 0. Therefore, add an independent check for the level. Since the level
> > > cannot be zero at this point, there is no need to check for `is_compact`,
> > > so remove that instead.
> > > 
> > 
> > This doesn't look right. Both xe_pt_zap_ptes_entry &
> > xe_pt_stage_unbind_entry can be at level 0 is 4K page entries are used,
> > right? CI looks good though so confused by that. I think maybe 2
> > independent VMAs would have to mapped within a 2M range for these paths
> > to decend to level 0. Maybe we don't have tests in place that do this.
> 
> Not too sure, but in both cases this is followed by doing a level-1 which is
> then used as the index into some array AFAICT. So if level can indeed by
> zero here then that would be a serious bug. Improving the assert here to
> catch that looked reasonable to me.
> 

Ah, yes. The comment below explains this. The this function is called on
the parent while operating on the child. So to write 4k page entries the
level would be 1.

This LGTM.

Matt

> > 
> > Regardless please don't merge this until my concerns are addresesed.
> > 
> > Matt
> > 
> > > Cc: Matthew Auld <matthew.auld@intel.com>
> > > Signed-off-by: Nirmoy Das <nirmoy.das@intel.com>
> > > ---
> > >   drivers/gpu/drm/xe/xe_pt.c | 4 ++--
> > >   1 file changed, 2 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> > > index 11dd0988ffda..cd60c009b679 100644
> > > --- a/drivers/gpu/drm/xe/xe_pt.c
> > > +++ b/drivers/gpu/drm/xe/xe_pt.c
> > > @@ -763,7 +763,7 @@ static int xe_pt_zap_ptes_entry(struct xe_ptw *parent, pgoff_t offset,
> > >   	pgoff_t end_offset;
> > >   	XE_WARN_ON(!*child);
> > > -	XE_WARN_ON(!level && xe_child->is_compact);
> > > +	XE_WARN_ON(!level);
> > >   	/*
> > >   	 * Note that we're called from an entry callback, and we're dealing
> > > @@ -1445,7 +1445,7 @@ static int xe_pt_stage_unbind_entry(struct xe_ptw *parent, pgoff_t offset,
> > >   	struct xe_pt *xe_child = container_of(*child, typeof(*xe_child), base);
> > >   	XE_WARN_ON(!*child);
> > > -	XE_WARN_ON(!level && xe_child->is_compact);
> > > +	XE_WARN_ON(!level);
> > >   	xe_pt_check_kill(addr, next, level - 1, xe_child, action, walk);
> > > -- 
> > > 2.42.0
> > >