[PATCH v5 1/6] drm/xe/xe_gt_pagefault: Disallow writes to read-only VMAs

[Cavitt, Jonathan]

-----Original Message-----
From: Mun, Gwan-gyeong <gwan-gyeong.mun at intel.com> 
Sent: Friday, March 7, 2025 2:35 AM
To: Cavitt, Jonathan <jonathan.cavitt at intel.com>; intel-xe at lists.freedesktop.org
Cc: Gupta, saurabhg <saurabhg.gupta at intel.com>; Zuo, Alex <alex.zuo at intel.com>; joonas.lahtinen at linux.intel.com; Brost, Matthew <matthew.brost at intel.com>; Zhang, Jianxun <jianxun.zhang at intel.com>; Lin, Shuicheng <shuicheng.lin at intel.com>; dri-devel at lists.freedesktop.org
Subject: Re: [PATCH v5 1/6] drm/xe/xe_gt_pagefault: Disallow writes to read-only VMAs
> 
> On 3/4/25 7:08 PM, Jonathan Cavitt wrote:
> > The page fault handler should reject write/atomic access to read only
> > VMAs.  Add code to handle this in handle_pagefault after the VMA lookup.
> > 
> > Fixes: 3d420e9fa848 ("drm/xe: Rework GPU page fault handling")
> > Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> > Suggested-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_gt_pagefault.c | 5 +++++
> >   1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_pagefault.c b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > index 17d69039b866..f608a765fa7c 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_pagefault.c
> > @@ -235,6 +235,11 @@ static int handle_pagefault(struct xe_gt *gt, struct pagefault *pf)
> >   		goto unlock_vm;
> >   	}
> >   
> > +	if (xe_vma_read_only(vma) && pf->access_type != ACCESS_TYPE_READ) {
> one question, if the PTE Present bit is disabled and the page fault is 
> caused by atomic load/store eu instruction, will the GuC deliver 
> pagefault request to KMD with ACCESS_TYPE_READ/ACCESS_TYPE_WRITE Fault 
> type instead of ACCESS_TYPE_ATOMIC?

I'm not certain.  Matthew Brost would probably know better.
Though, if this becomes an issue, we can always update the pagefault handler in the
future to manage this case separately.  And at any rate, I think it would be best to
manage that case in a separate patch.

-Jonathan Cavitt

> 
> G.G.
> > +		err = -EPERM;
> > +		goto unlock_vm;
> > +	}
> > +
> >   	err = handle_vma_pagefault(gt, pf, vma);
> >   
> >   unlock_vm:
> 
>