Re: [PATCH 1/9] drm/xe/rtp: Add RING_FORCE_TO_NONPRIV_DENY to OA whitelists

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Wed, 27 May 2026 13:03:53 -0700, Umesh Nerlige Ramappa wrote:
>

Hi Umesh,

> On Tue, May 26, 2026 at 12:12:33PM -0700, Umesh Nerlige Ramappa wrote:
> > On Thu, May 21, 2026 at 04:35:25PM -0700, Dixit, Ashutosh wrote:
> >> On Thu, 21 May 2026 16:14:50 -0700, Umesh Nerlige Ramappa wrote:
> >>>
> >>> On Mon, May 18, 2026 at 04:47:08PM -0700, Ashutosh Dixit wrote:
> >>>> Unconditionally whitelisting OA registers is a security violation. Set
> >>>> RING_FORCE_TO_NONPRIV_DENY bit in OA nonpriv slots, so that OA registers
> >>>> don't get whitelisted by default after probe/reset/restart.
>
> probe/gt-reset/resume to be precise.

Yes, good idea, I'll change these and maybe add a comment too near
WHITELIST_OA_MMIO_TRG().

> During resume and gt-reset flows KMD will apply the reg_sr to mmio, so
> you are ensuring DENY is enforced in these paths as well. As for
> engine-reset, the hwe->reg_sr is registered with GuC and GuC will save
> and restore these values.
>

This will likely need to go to Patch 5 ("drm/xe/rtp: Save OA nonpriv
registers to register save/restore lists"), let me see. I already added a
hint there, but will see if the commit message can be improved in the next
rev. We can take a look again at v2.

> Some of this info would be helpful in the commit messages OR comments to
> understand the bigger picture (for reviewers at least)

Thanks.
--
Ashutosh



>
> >>>>
> >>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >>>> ---
> >>>> drivers/gpu/drm/xe/xe_reg_whitelist.c | 7 ++++---
> >>>> 1 file changed, 4 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/xe/xe_reg_whitelist.c b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >>>> index fb65940848d7a..d6a5d499373bc 100644
> >>>> --- a/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >>>> +++ b/drivers/gpu/drm/xe/xe_reg_whitelist.c
> >>>> @@ -105,9 +105,10 @@ static const struct xe_rtp_entry_sr register_whitelist[] = {
> >>>>	},
> >>>>
> >>>> #define WHITELIST_OA_MMIO_TRG(trg, status, head) \
> >>>> -	WHITELIST(trg, RING_FORCE_TO_NONPRIV_ACCESS_RW), \
> >>>> -	WHITELIST(status, RING_FORCE_TO_NONPRIV_ACCESS_RD), \
> >>>> -	WHITELIST(head, RING_FORCE_TO_NONPRIV_ACCESS_RD | RING_FORCE_TO_NONPRIV_RANGE_4)
> >>>> +	WHITELIST(trg, RING_FORCE_TO_NONPRIV_ACCESS_RW | RING_FORCE_TO_NONPRIV_DENY), \
> >>>> +	WHITELIST(status, RING_FORCE_TO_NONPRIV_ACCESS_RD | RING_FORCE_TO_NONPRIV_DENY), \
> >>>> +	WHITELIST(head, RING_FORCE_TO_NONPRIV_ACCESS_RD | RING_FORCE_TO_NONPRIV_RANGE_4 | \
> >>>> +		  RING_FORCE_TO_NONPRIV_DENY)
> >>>
> >>> status and head should be clubbed into one slot, starting with status and
> >>> RANGE_4. Maybe that can be a patch before this one.
> >>
> >> No, e.g. for OAG, status is 0xdafc which is not a multiple of 16, which is
> >> a requirement for RANGE_4.
> >
> > I missed that. I thought HW compared ranges differently. In that case,
> > this is correct and the individual registers can be whitelisted
> > separately if needed. Otherwise some registers can be dropped whenever
> > you plan to do it.
> >
> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >
> >
> > Umesh.
> >>
> >> Also, about RANGE_4 above, there were different suggestions, e.g. tail and
> >> oabuffer should be different slots, rather than grouping in a single
> >> RANGE_4 above. To avoid any such controversy, I decided to focus this
> >> series only on removing unconditional whitelisting for OA registers. Any
> >> other changes, such as removing or retaining RANGE_4, we can do after this
> >> series is reviewed/merged.
> >>
> >> Thanks.
> >> --
> >> Ashutosh
> >>
> >>>> #define WHITELIST_OAG_MMIO_TRG \
> >>>>	WHITELIST_OA_MMIO_TRG(OAG_MMIOTRIGGER, OAG_OASTATUS, OAG_OAHEADPTR)
> >>>> --
> >>>> 2.54.0
> >>>>