Re: [PATCH 3/9] drm/xe/xe3: Generate and store the L3 bank mask

[Francois Dugast <francois.dugast@intel.com>]

On Fri, Sep 06, 2024 at 04:43:15PM -0700, Matt Roper wrote:
> On Fri, Sep 06, 2024 at 02:51:47PM -0700, Matt Atwood wrote:
> > From: Francois Dugast <francois.dugast@intel.com>
> > 
> > On Xe3, the register used to indicate which L3 banks are enabled on
> > the system is a new one called MIRROR_L3BANK_ENABLE. Each bit
> > represents one bank enabled in each node.
> > Extend the existing topology code for Xe3 to read this register and
> > generate the correct L3 bank mask, which can be read by user space
> > throug the topology query.
> > 
> > Bspec: 72573, 73439
> > 
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast@intel.com>
> > Signed-off-by: Matt Atwood <matthew.s.atwood@intel.com>
> > ---
> >  drivers/gpu/drm/xe/regs/xe_gt_regs.h |  3 +++
> >  drivers/gpu/drm/xe/xe_gt_topology.c  | 11 ++++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/regs/xe_gt_regs.h b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > index 0d1a4a9f4e11..8ed855b31e95 100644
> > --- a/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > +++ b/drivers/gpu/drm/xe/regs/xe_gt_regs.h
> > @@ -218,6 +218,9 @@
> >  
> >  #define MIRROR_FUSE1				XE_REG(0x911c)
> >  
> > +#define MIRROR_L3BANK_ENABLE			XE_REG(0x9130)
> > +#define   XE3_L3BANK_ENABLE			REG_GENMASK(31, 0)
> > +
> >  #define XELP_EU_ENABLE				XE_REG(0x9134)	/* "_DISABLE" on Xe_LP */
> >  #define   XELP_EU_MASK				REG_GENMASK(7, 0)
> >  #define XELP_GT_SLICE_ENABLE			XE_REG(0x9138)
> > diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
> > index 0662f71c6ede..56571380a2b5 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_topology.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
> > @@ -129,7 +129,16 @@ load_l3_bank_mask(struct xe_gt *gt, xe_l3_bank_mask_t l3_bank_mask)
> >  	struct xe_device *xe = gt_to_xe(gt);
> >  	u32 fuse3 = xe_mmio_read32(gt, MIRROR_FUSE3);
> >  
> > -	if (GRAPHICS_VER(xe) >= 20) {
> > +	if (GRAPHICS_VER(xe) >= 30) {
> > +		xe_l3_bank_mask_t per_node = {};
> > +		u32 meml3_en = REG_FIELD_GET(XE2_NODE_ENABLE_MASK, fuse3);
> 
> This is a 16-bit mask...
> 
> > +		u32 mirror_l3bank_enable = xe_mmio_read32(gt, MIRROR_L3BANK_ENABLE);
> > +		u32 bank_val = REG_FIELD_GET(XE3_L3BANK_ENABLE, mirror_l3bank_enable);
> 
> ...and this is a 32-bit mask...
> 
> > +
> > +		bitmap_from_arr32(per_node, &bank_val, 32);
> > +		gen_l3_mask_from_pattern(xe, l3_bank_mask, per_node, 32,
> > +					 meml3_en);
> 
> ...so when we combine them the resulting mask could potentially be up to
> 16 * 32 = 512 bits.  Even if Xe3 platforms don't really have so many
> banks to use all of those bits, we could still read such values from the
> registers (e.g., if a PCI link glitch or other temporary issue causes
> the fuse registers to read out as 0xFFFFFFFF).  Since
> XE_MAX_L3_BANK_MASK_BITS is currently just 64, we can potentially run
> past the bounds of the allocated memory for the bitmask and probably
> cause a kernel panic.  So I think we need to extend that define to 512
> bits to cover all possible values, even if we don't expect to ever see
> them on a properly functioning system.

This would make the query response unnecessarily large and maybe misleading
if we know already we do not need this size.

I believe the scenario you described is currently not possible. This line
in gen_l3_mask_from_pattern() would warn if the mask would go beyond
XE_MAX_L3_BANK_MASK_BITS:

    xe_assert(xe, !mask || patternbits * (__fls(mask) + 1) <= XE_MAX_L3_BANK_MASK_BITS);

Even with the test in xe_assert() compiled out, we should be fine as long
as the last argument passed to the linux/bitmap.h functions bitmap_*() is
XE_MAX_L3_BANK_MASK_BITS, which is the case in gen_l3_mask_from_pattern().

Francois

> 
> 
> Matt
> 
> > +	} else if (GRAPHICS_VER(xe) >= 20) {
> >  		xe_l3_bank_mask_t per_node = {};
> >  		u32 meml3_en = REG_FIELD_GET(XE2_NODE_ENABLE_MASK, fuse3);
> >  		u32 bank_val = REG_FIELD_GET(XE2_GT_L3_MODE_MASK, fuse3);
> > -- 
> > 2.44.0
> > 
> 
> -- 
> Matt Roper
> Graphics Software Engineer
> Linux GPU Platform Enablement
> Intel Corporation