[PATCH v2] drm/xe/gt: Fix assert in L3 bank mask generation

[Francois Dugast]

On Tue, Apr 30, 2024 at 08:43:15AM -0500, Lucas De Marchi wrote:
> On Tue, Apr 30, 2024 at 11:40:45AM GMT, Francois Dugast wrote:
> > What needs to be asserted is that the pattern fits in the number
> > of bits provided by the user in patternbits, otherwise it would
> > be truncated when replicated according to the mask, which is
> > likely not the intended use of this function.
> > The pattern argument is a bitmap so use find_last_bit() instead
> > of fls(). The bit position starts at index 0 so remove "or equal"
> > from the comparison, and XE_MAX_L3_BANK_MASK_BITS would be the
> > returned value if the pattern is 0.
> > 
> > Cc: Matt Roper <matthew.d.roper at intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_gt_topology.c | 5 ++++-
> > 1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_gt_topology.c b/drivers/gpu/drm/xe/xe_gt_topology.c
> > index af841d801a8f..efe9e5d6fd52 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_topology.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_topology.c
> > @@ -107,8 +107,11 @@ gen_l3_mask_from_pattern(struct xe_device *xe, xe_l3_bank_mask_t dst,
> > 			 unsigned long mask)
> > {
> > 	unsigned long bit;
> > +	unsigned long last_bit = find_last_bit(pattern,
> > +					       XE_MAX_L3_BANK_MASK_BITS);
> > 
> > -	xe_assert(xe, fls(mask) <= patternbits);
> > +	xe_assert(xe, last_bit < patternbits ||
> > +		  last_bit == XE_MAX_L3_BANK_MASK_BITS);
> 
> then if xe_assert() compiles to nothing we may be left with "unused
> last_bit" variable" :(.  I think it would be simpler if pattern was
> unsigned long long as apparently it would be sufficient for our needs
> and we could simply do
> 
> 	xe_assert(xe, pattern <= GENMASK_ULL(patternbits, 0));
> 
> As for XE_MAX_L3_BANK_MASK_BITS, the assert here doesn't seem sufficient
> as you want to check if the final result doesn't overflow the array.
> 
> 
> Wihout changing the pattern type, I think what we want is:
> 
> 	xe_assert(xe, find_last_bit(pattern, XE_MAX_L3_BANK_MASK_BITS) < patternbits);
> 	xe_assert(xe, patternbits * fls(mask) <= XE_MAX_L3_BANK_MASK_BITS);

The array overflow check is a great addition, but the other assertion
would fail if the pattern is 0 as find_last_bit would return
XE_MAX_L3_BANK_MASK_BITS which is greater than patternbits.

What about this:

	xe_assert(xe, find_last_bit(pattern, XE_MAX_L3_BANK_MASK_BITS) < patternbits || bitmap_empty(pattern, XE_MAX_L3_BANK_MASK_BITS));
	xe_assert(xe, patternbits * fls(mask) <= XE_MAX_L3_BANK_MASK_BITS);

Francois

> 
> 
> Lucas De Marchi
> 
> > 	for_each_set_bit(bit, &mask, 32) {
> > 		xe_l3_bank_mask_t shifted_pattern = {};
> > 
> > -- 
> > 2.34.1
> >