[PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Stephane Eranian]

AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:

	r->cache.arch_has_empty_bitmaps = true;

However given the unified code in cbm_validate(), checking for:

    val == 0 && !arch_has_empty_bitmaps

is not enough because you also have in cbm_validate():
	if ((zero_bit - first_bit) < r->cache.min_cbm_bits)

Default value for if r->cache.min_cbm_bits = 1

Leading to:

$ cd /sys/fs/resctrl
$ mkdir foo
$ cd foo
$ echo L3:0=0 > schemata
-bash: echo: write error: Invalid argument

Patch initializes fixes the logic in cbm_validate() to take into account
arch_has_empty_bitmaps when true and cbm value is 0.

Fixes: 316e7f901f5a ("x86/resctrl: Add struct rdt_cache::arch_has_{sparse, empty}_bitmaps")

Signed-off-by: Stephane Eranian <eranian@google.com>
---
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 87666275eed9..f376ed8bff8f 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 	first_bit = find_first_bit(&val, cbm_len);
 	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
 
+	/* no need to check bits if arch supports no bits set */
+	if (r->cache.arch_has_empty_bitmaps && val == 0)
+		goto done;
+
 	/* Are non-contiguous bitmaps allowed? */
 	if (!r->cache.arch_has_sparse_bitmaps &&
 	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
@@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 				    r->cache.min_cbm_bits);
 		return false;
 	}
-
+done:
 	*data = val;
 	return true;
 }
-- 
2.36.0.550.gb090851708-goog

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Fenghua Yu]

Hi, Eranian,

On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
...
> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>  	first_bit = find_first_bit(&val, cbm_len);
>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>  
> +	/* no need to check bits if arch supports no bits set */
> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
> +		goto done;
> +
>  	/* Are non-contiguous bitmaps allowed? */
>  	if (!r->cache.arch_has_sparse_bitmaps &&
>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>  				    r->cache.min_cbm_bits);
>  		return false;
>  	}
> -
> +done:
>  	*data = val;
>  	return true;
>  }

Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
Is the following patch a better fix? I don't have AMD machine and cannot
test the patch.

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 6055d05af4cc..031d77dd982d 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
 			r->cache.arch_has_sparse_bitmaps = true;
 			r->cache.arch_has_empty_bitmaps = true;
 			r->cache.arch_has_per_cpu_cfg = true;
+			r->cache.min_cbm_bits = 0;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
 			hw_res->msr_update = mba_wrmsr_amd;

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Reinette Chatre]

Hi Fenghua,

On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> Hi, Eranian,
> 
> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> ...
>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  	first_bit = find_first_bit(&val, cbm_len);
>>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>>  
>> +	/* no need to check bits if arch supports no bits set */
>> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
>> +		goto done;
>> +
>>  	/* Are non-contiguous bitmaps allowed? */
>>  	if (!r->cache.arch_has_sparse_bitmaps &&
>>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>  				    r->cache.min_cbm_bits);
>>  		return false;
>>  	}
>> -
>> +done:
>>  	*data = val;
>>  	return true;
>>  }
> 
> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> Is the following patch a better fix? I don't have AMD machine and cannot
> test the patch.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 6055d05af4cc..031d77dd982d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
>  			r->cache.arch_has_sparse_bitmaps = true;
>  			r->cache.arch_has_empty_bitmaps = true;
>  			r->cache.arch_has_per_cpu_cfg = true;
> +			r->cache.min_cbm_bits = 0;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>  			hw_res->msr_update = mba_wrmsr_amd;

That is actually what Stephane's V1 [1] did and I proposed that
he fixes it with (almost) what he has in V2 (I think the check
can be moved earlier before any bits are searched for).

The reasons why I proposed this change are:
- min_cbm_bits is a value that is exposed to user space and from the
  time AMD was supported this has always been 1 for those systems. I
  do not know how user space uses this value and unless I can be certain
  making this 0 will not affect user space I would prefer not to
  make such a change.

- this fix restores original behavior that was changed in the patch noted
  in the Fixes link.

- this fix itself relies on math on error returns of bit checking on an empty
  bitmap. I find that hides what the code does and this fix is more obvious.
  You can see this feedback in my response to V1. 

- a fix like the above snippet is incomplete. To be appropriate 
  the initialization of rdt_resources_all[] needs to be changed to
  not initialize min_cbm_bits anymore and move the platform specific bits
  to rdt_init_res_defs_amd() and rdt_init_res_defs_intel() respectively.


Reinette

[1] https://lore.kernel.org/lkml/20220516055055.2734840-1-eranian@google.com/

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Fenghua Yu]

Hi, Reinette,

On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote:
> Hi Fenghua,
> 
> On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> > Hi, Eranian,
> > 
> > On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
> >> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> > ...
> >> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> >>  	first_bit = find_first_bit(&val, cbm_len);
> >>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
> >>  
> >> +	/* no need to check bits if arch supports no bits set */
> >> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
> >> +		goto done;
> >> +
> >>  	/* Are non-contiguous bitmaps allowed? */
> >>  	if (!r->cache.arch_has_sparse_bitmaps &&
> >>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
> >> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> >>  				    r->cache.min_cbm_bits);
> >>  		return false;
> >>  	}
> >> -
> >> +done:
> >>  	*data = val;
> >>  	return true;
> >>  }
> > 
> > Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> > Is the following patch a better fix? I don't have AMD machine and cannot
> > test the patch.
> > 
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index 6055d05af4cc..031d77dd982d 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
> >  			r->cache.arch_has_sparse_bitmaps = true;
> >  			r->cache.arch_has_empty_bitmaps = true;
> >  			r->cache.arch_has_per_cpu_cfg = true;
> > +			r->cache.min_cbm_bits = 0;
> >  		} else if (r->rid == RDT_RESOURCE_MBA) {
> >  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
> >  			hw_res->msr_update = mba_wrmsr_amd;
> 
> That is actually what Stephane's V1 [1] did and I proposed that
> he fixes it with (almost) what he has in V2 (I think the check
> can be moved earlier before any bits are searched for).
> 
> The reasons why I proposed this change are:
> - min_cbm_bits is a value that is exposed to user space and from the
>   time AMD was supported this has always been 1 for those systems. I
>   do not know how user space uses this value and unless I can be certain
>   making this 0 will not affect user space I would prefer not to
>   make such a change.

But a user visible mismatch is created by the V2 patch:
User queries min_cbm_bits and finds it is 1 but turns out 0 can be written
to the schemata.

Is it an acceptable behavior? Shouldn't user read right min_cbm_bits (0)
on AMD?

Without the V2 patch, at least min_cbm_bits and writing to the schemata
are matched: only 1 about above bits can be searched and written.

By setting min_cbm_bits=0, it reflects the right value and user can see
the value as 0 and set schemata as 0 as well. Seems all match each other.

> - this fix restores original behavior that was changed in the patch noted
>   in the Fixes link.
> 
> - this fix itself relies on math on error returns of bit checking on an empty
>   bitmap. I find that hides what the code does and this fix is more obvious.
>   You can see this feedback in my response to V1. 
> 
> - a fix like the above snippet is incomplete. To be appropriate 
>   the initialization of rdt_resources_all[] needs to be changed to
>   not initialize min_cbm_bits anymore and move the platform specific bits
>   to rdt_init_res_defs_amd() and rdt_init_res_defs_intel() respectively.

Maybe that's better.

> 
> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/20220516055055.2734840-1-eranian@google.com/

Thanks.

-Fenghua

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Reinette Chatre]

Hi Fenghua,

On 5/17/2022 10:27 AM, Fenghua Yu wrote:
> Hi, Reinette,
> 
> On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote:
>> Hi Fenghua,
>>
>> On 5/17/2022 9:33 AM, Fenghua Yu wrote:
>>> Hi, Eranian,
>>>
>>> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
>>>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
>>> ...
>>>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>>>  	first_bit = find_first_bit(&val, cbm_len);
>>>>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
>>>>  
>>>> +	/* no need to check bits if arch supports no bits set */
>>>> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
>>>> +		goto done;
>>>> +
>>>>  	/* Are non-contiguous bitmaps allowed? */
>>>>  	if (!r->cache.arch_has_sparse_bitmaps &&
>>>>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
>>>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>>>>  				    r->cache.min_cbm_bits);
>>>>  		return false;
>>>>  	}
>>>> -
>>>> +done:
>>>>  	*data = val;
>>>>  	return true;
>>>>  }
>>>
>>> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
>>> Is the following patch a better fix? I don't have AMD machine and cannot
>>> test the patch.
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index 6055d05af4cc..031d77dd982d 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
>>>  			r->cache.arch_has_sparse_bitmaps = true;
>>>  			r->cache.arch_has_empty_bitmaps = true;
>>>  			r->cache.arch_has_per_cpu_cfg = true;
>>> +			r->cache.min_cbm_bits = 0;
>>>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>>>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>>>  			hw_res->msr_update = mba_wrmsr_amd;
>>
>> That is actually what Stephane's V1 [1] did and I proposed that
>> he fixes it with (almost) what he has in V2 (I think the check
>> can be moved earlier before any bits are searched for).
>>
>> The reasons why I proposed this change are:
>> - min_cbm_bits is a value that is exposed to user space and from the
>>   time AMD was supported this has always been 1 for those systems. I
>>   do not know how user space uses this value and unless I can be certain
>>   making this 0 will not affect user space I would prefer not to
>>   make such a change.
> 
> But a user visible mismatch is created by the V2 patch:

No. V2 does not create a user visible change, it restores original behavior.

> User queries min_cbm_bits and finds it is 1 but turns out 0 can be written
> to the schemata.
> 
> Is it an acceptable behavior? Shouldn't user read right min_cbm_bits (0)
> on AMD?

Original AMD enabling set min_cbm_bits as 1 while also supporting 0 to
be written to schemata file. Supporting 0 to be written to schemata file
was unintentionally broken during one of the MPAM prep patches. This
patch fixes that.

You are proposing a change to original AMD support that impacts user
space API while I find fixing to restore original behavior to
be the safest option. Perhaps Babu could pick the preferred solution
and I would step aside if you prefer to go with (an improved) V1.

Reinette

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Fenghua Yu]

Hi, Babu,

On Tue, May 17, 2022 at 11:10:40AM -0700, Reinette Chatre wrote:
> On 5/17/2022 10:27 AM, Fenghua Yu wrote:
> > On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote:
> >> On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> >>> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
> >>>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> >>> ...
> >>>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> >>>>  	first_bit = find_first_bit(&val, cbm_len);
> >>>>  	zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
> >>>>  
> >>>> +	/* no need to check bits if arch supports no bits set */
> >>>> +	if (r->cache.arch_has_empty_bitmaps && val == 0)
> >>>> +		goto done;
> >>>> +
> >>>>  	/* Are non-contiguous bitmaps allowed? */
> >>>>  	if (!r->cache.arch_has_sparse_bitmaps &&
> >>>>  	    (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
> >>>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> >>>>  				    r->cache.min_cbm_bits);
> >>>>  		return false;
> >>>>  	}
> >>>> -
> >>>> +done:
> >>>>  	*data = val;
> >>>>  	return true;
> >>>>  }
> >>>
> >>> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> >>> Is the following patch a better fix? I don't have AMD machine and cannot
> >>> test the patch.
> >>>
> >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> >>> index 6055d05af4cc..031d77dd982d 100644
> >>> --- a/arch/x86/kernel/cpu/resctrl/core.c
> >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> >>> @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
> >>>  			r->cache.arch_has_sparse_bitmaps = true;
> >>>  			r->cache.arch_has_empty_bitmaps = true;
> >>>  			r->cache.arch_has_per_cpu_cfg = true;
> >>> +			r->cache.min_cbm_bits = 0;
> >>>  		} else if (r->rid == RDT_RESOURCE_MBA) {
> >>>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
> >>>  			hw_res->msr_update = mba_wrmsr_amd;
> >>
> >> That is actually what Stephane's V1 [1] did and I proposed that
> >> he fixes it with (almost) what he has in V2 (I think the check
> >> can be moved earlier before any bits are searched for).
> >>
> >> The reasons why I proposed this change are:
> >> - min_cbm_bits is a value that is exposed to user space and from the
> >>   time AMD was supported this has always been 1 for those systems. I
> >>   do not know how user space uses this value and unless I can be certain
> >>   making this 0 will not affect user space I would prefer not to
> >>   make such a change.
> > 
> > But a user visible mismatch is created by the V2 patch:
> 
> No. V2 does not create a user visible change, it restores original behavior.
> 
> > User queries min_cbm_bits and finds it is 1 but turns out 0 can be written
> > to the schemata.
> > 
> > Is it an acceptable behavior? Shouldn't user read right min_cbm_bits (0)
> > on AMD?
> 
> Original AMD enabling set min_cbm_bits as 1 while also supporting 0 to
> be written to schemata file. Supporting 0 to be written to schemata file
> was unintentionally broken during one of the MPAM prep patches. This
> patch fixes that.
> 
> You are proposing a change to original AMD support that impacts user
> space API while I find fixing to restore original behavior to
> be the safest option. Perhaps Babu could pick the preferred solution
> and I would step aside if you prefer to go with (an improved) V1.

Is it OK for you to set min_cbm_bits to 0 on AMD?

Thanks.

-Fenghua

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Stephane Eranian]

On Wed, May 18, 2022 at 7:36 PM Fenghua Yu <fenghua.yu@intel.com> wrote:
>
> Hi, Babu,
>
> On Tue, May 17, 2022 at 11:10:40AM -0700, Reinette Chatre wrote:
> > On 5/17/2022 10:27 AM, Fenghua Yu wrote:
> > > On Tue, May 17, 2022 at 09:49:22AM -0700, Reinette Chatre wrote:
> > >> On 5/17/2022 9:33 AM, Fenghua Yu wrote:
> > >>> On Mon, May 16, 2022 at 05:12:34PM -0700, Stephane Eranian wrote:
> > >>>> AMD supports cbm with no bits set as reflected in rdt_init_res_defs_amd() by:
> > >>> ...
> > >>>> @@ -107,6 +107,10 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> > >>>>          first_bit = find_first_bit(&val, cbm_len);
> > >>>>          zero_bit = find_next_zero_bit(&val, cbm_len, first_bit);
> > >>>>
> > >>>> +        /* no need to check bits if arch supports no bits set */
> > >>>> +        if (r->cache.arch_has_empty_bitmaps && val == 0)
> > >>>> +                goto done;
> > >>>> +
> > >>>>          /* Are non-contiguous bitmaps allowed? */
> > >>>>          if (!r->cache.arch_has_sparse_bitmaps &&
> > >>>>              (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
> > >>>> @@ -119,7 +123,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
> > >>>>                                      r->cache.min_cbm_bits);
> > >>>>                  return false;
> > >>>>          }
> > >>>> -
> > >>>> +done:
> > >>>>          *data = val;
> > >>>>          return true;
> > >>>>  }
> > >>>
> > >>> Isn't it AMD supports 0 minimal CBM bits? Then should set its min_cbm_bits as 0.
> > >>> Is the following patch a better fix? I don't have AMD machine and cannot
> > >>> test the patch.
> > >>>
> > >>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > >>> index 6055d05af4cc..031d77dd982d 100644
> > >>> --- a/arch/x86/kernel/cpu/resctrl/core.c
> > >>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > >>> @@ -909,6 +909,7 @@ static __init void rdt_init_res_defs_amd(void)
> > >>>                   r->cache.arch_has_sparse_bitmaps = true;
> > >>>                   r->cache.arch_has_empty_bitmaps = true;
> > >>>                   r->cache.arch_has_per_cpu_cfg = true;
> > >>> +                 r->cache.min_cbm_bits = 0;
> > >>>           } else if (r->rid == RDT_RESOURCE_MBA) {
> > >>>                   hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
> > >>>                   hw_res->msr_update = mba_wrmsr_amd;
> > >>
> > >> That is actually what Stephane's V1 [1] did and I proposed that
> > >> he fixes it with (almost) what he has in V2 (I think the check
> > >> can be moved earlier before any bits are searched for).
> > >>
> > >> The reasons why I proposed this change are:
> > >> - min_cbm_bits is a value that is exposed to user space and from the
> > >>   time AMD was supported this has always been 1 for those systems. I
> > >>   do not know how user space uses this value and unless I can be certain
> > >>   making this 0 will not affect user space I would prefer not to
> > >>   make such a change.
> > >
> > > But a user visible mismatch is created by the V2 patch:
> >
> > No. V2 does not create a user visible change, it restores original behavior.
> >
> > > User queries min_cbm_bits and finds it is 1 but turns out 0 can be written
> > > to the schemata.
> > >
> > > Is it an acceptable behavior? Shouldn't user read right min_cbm_bits (0)
> > > on AMD?
> >
> > Original AMD enabling set min_cbm_bits as 1 while also supporting 0 to
> > be written to schemata file. Supporting 0 to be written to schemata file
> > was unintentionally broken during one of the MPAM prep patches. This
> > patch fixes that.
> >
> > You are proposing a change to original AMD support that impacts user
> > space API while I find fixing to restore original behavior to
> > be the safest option. Perhaps Babu could pick the preferred solution
> > and I would step aside if you prefer to go with (an improved) V1.
>
> Is it OK for you to set min_cbm_bits to 0 on AMD?
>
I agree with Fenghua here. The proper fix is my v1 which sets the
min_cbm_bits to the value it should have set to from the beginning.
This field is exposed via resctrl fs and it is still fine if the value
changes. Any app is supposed to read the content of the file and not
hardcode
any value or pre-suppose it is always 1.

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Babu Moger]

Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

Sorry, I didn't see this thread. Just noticed going thru the archives.
Replying using "git send-email" to the thread.
 
Thanks Stephane for the patch. Thanks Fenghua and Reinette for your comments.
 
Stephane, Are you planning to re-submit the patch with Fenghua's proposal?
If not I will resubmit with my current patch-set.
 
I agree with Fenghua's proposal. Here is my proposal with slight modification.
 
Thanks 

==================================================================================

Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

Signed-off-by: Babu Moger <babu.moger@amd.com>


diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index bb1c3f5f60c8..a5c51a14fbce 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -66,9 +66,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.rid			= RDT_RESOURCE_L3,
 			.name			= "L3",
 			.cache_level		= 3,
-			.cache = {
-				.min_cbm_bits	= 1,
-			},
 			.domains		= domain_init(RDT_RESOURCE_L3),
 			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
@@ -83,9 +80,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.rid			= RDT_RESOURCE_L2,
 			.name			= "L2",
 			.cache_level		= 2,
-			.cache = {
-				.min_cbm_bits	= 1,
-			},
 			.domains		= domain_init(RDT_RESOURCE_L2),
 			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
@@ -877,6 +871,7 @@ static __init void rdt_init_res_defs_intel(void)
 			r->cache.arch_has_sparse_bitmaps = false;
 			r->cache.arch_has_empty_bitmaps = false;
 			r->cache.arch_has_per_cpu_cfg = false;
+			r->cache.min_cbm_bits = 1;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_THRTL_BASE;
 			hw_res->msr_update = mba_wrmsr_intel;
@@ -897,6 +892,7 @@ static __init void rdt_init_res_defs_amd(void)
 			r->cache.arch_has_sparse_bitmaps = true;
 			r->cache.arch_has_empty_bitmaps = true;
 			r->cache.arch_has_per_cpu_cfg = true;
+			r->cache.min_cbm_bits = 0;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
 			hw_res->msr_update = mba_wrmsr_amd;

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Moger, Babu]

On 7/25/22 14:47, Babu Moger wrote:
> Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
>
> Sorry, I didn't see this thread. Just noticed going thru the archives.
> Replying using "git send-email" to the thread.
>  
> Thanks Stephane for the patch. Thanks Fenghua and Reinette for your comments.
>  
> Stephane, Are you planning to re-submit the patch with Fenghua's proposal?
> If not I will resubmit with my current patch-set.

Didn't see Stephan's response yet. I will add this patch in my QoS series.

Stephan, Let me know if you want me to add your "Signed-off-by".

Thanks

Babu

>  
> I agree with Fenghua's proposal. Here is my proposal with slight modification.
>  
> Thanks 
>
> ==================================================================================
>
> Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
>
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index bb1c3f5f60c8..a5c51a14fbce 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -66,9 +66,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  			.rid			= RDT_RESOURCE_L3,
>  			.name			= "L3",
>  			.cache_level		= 3,
> -			.cache = {
> -				.min_cbm_bits	= 1,
> -			},
>  			.domains		= domain_init(RDT_RESOURCE_L3),
>  			.parse_ctrlval		= parse_cbm,
>  			.format_str		= "%d=%0*x",
> @@ -83,9 +80,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
>  			.rid			= RDT_RESOURCE_L2,
>  			.name			= "L2",
>  			.cache_level		= 2,
> -			.cache = {
> -				.min_cbm_bits	= 1,
> -			},
>  			.domains		= domain_init(RDT_RESOURCE_L2),
>  			.parse_ctrlval		= parse_cbm,
>  			.format_str		= "%d=%0*x",
> @@ -877,6 +871,7 @@ static __init void rdt_init_res_defs_intel(void)
>  			r->cache.arch_has_sparse_bitmaps = false;
>  			r->cache.arch_has_empty_bitmaps = false;
>  			r->cache.arch_has_per_cpu_cfg = false;
> +			r->cache.min_cbm_bits = 1;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>  			hw_res->msr_base = MSR_IA32_MBA_THRTL_BASE;
>  			hw_res->msr_update = mba_wrmsr_intel;
> @@ -897,6 +892,7 @@ static __init void rdt_init_res_defs_amd(void)
>  			r->cache.arch_has_sparse_bitmaps = true;
>  			r->cache.arch_has_empty_bitmaps = true;
>  			r->cache.arch_has_per_cpu_cfg = true;
> +			r->cache.min_cbm_bits = 0;
>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>  			hw_res->msr_update = mba_wrmsr_amd;

-- 
Thanks
Babu Moger

Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate

[Stephane Eranian]

On Mon, Aug 1, 2022 at 5:58 PM Moger, Babu <babu.moger@amd.com> wrote:
>
>
> On 7/25/22 14:47, Babu Moger wrote:
> > Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
> >
> > Sorry, I didn't see this thread. Just noticed going thru the archives.
> > Replying using "git send-email" to the thread.
> >
> > Thanks Stephane for the patch. Thanks Fenghua and Reinette for your comments.
> >
> > Stephane, Are you planning to re-submit the patch with Fenghua's proposal?
> > If not I will resubmit with my current patch-set.
>
Haven't had a chance to ge to this yet. But it's been ongoing for a
while, so I am fine
with Fenghua's proposal at this point.

> Didn't see Stephan's response yet. I will add this patch in my QoS series.
>
> Stephan, Let me know if you want me to add your "Signed-off-by".
>
Sure. Thanks.

> Thanks
>
> Babu
>
> >
> > I agree with Fenghua's proposal. Here is my proposal with slight modification.
> >
> > Thanks
> >
> > ==================================================================================
> >
> > Subject: Re: [PATCH v2] x86/resctrl: Fix zero cbm for AMD in cbm_validate
> >
> > Signed-off-by: Babu Moger <babu.moger@amd.com>
> >
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> > index bb1c3f5f60c8..a5c51a14fbce 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -66,9 +66,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
> >                       .rid                    = RDT_RESOURCE_L3,
> >                       .name                   = "L3",
> >                       .cache_level            = 3,
> > -                     .cache = {
> > -                             .min_cbm_bits   = 1,
> > -                     },
> >                       .domains                = domain_init(RDT_RESOURCE_L3),
> >                       .parse_ctrlval          = parse_cbm,
> >                       .format_str             = "%d=%0*x",
> > @@ -83,9 +80,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
> >                       .rid                    = RDT_RESOURCE_L2,
> >                       .name                   = "L2",
> >                       .cache_level            = 2,
> > -                     .cache = {
> > -                             .min_cbm_bits   = 1,
> > -                     },
> >                       .domains                = domain_init(RDT_RESOURCE_L2),
> >                       .parse_ctrlval          = parse_cbm,
> >                       .format_str             = "%d=%0*x",
> > @@ -877,6 +871,7 @@ static __init void rdt_init_res_defs_intel(void)
> >                       r->cache.arch_has_sparse_bitmaps = false;
> >                       r->cache.arch_has_empty_bitmaps = false;
> >                       r->cache.arch_has_per_cpu_cfg = false;
> > +                     r->cache.min_cbm_bits = 1;
> >               } else if (r->rid == RDT_RESOURCE_MBA) {
> >                       hw_res->msr_base = MSR_IA32_MBA_THRTL_BASE;
> >                       hw_res->msr_update = mba_wrmsr_intel;
> > @@ -897,6 +892,7 @@ static __init void rdt_init_res_defs_amd(void)
> >                       r->cache.arch_has_sparse_bitmaps = true;
> >                       r->cache.arch_has_empty_bitmaps = true;
> >                       r->cache.arch_has_per_cpu_cfg = true;
> > +                     r->cache.min_cbm_bits = 0;
> >               } else if (r->rid == RDT_RESOURCE_MBA) {
> >                       hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
> >                       hw_res->msr_update = mba_wrmsr_amd;
>
> --
> Thanks
> Babu Moger
>

[tip: x86/urgent] x86/resctrl: Fix min_cbm_bits for AMD

[tip-bot2 for Babu Moger]

The following commit has been merged into the x86/urgent branch of tip:

Commit-ID:     67bf6493449b09590f9f71d7df29efb392b12d25
Gitweb:        https://git.kernel.org/tip/67bf6493449b09590f9f71d7df29efb392b12d25
Author:        Babu Moger <babu.moger@amd.com>
AuthorDate:    Tue, 27 Sep 2022 15:16:29 -05:00
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 18 Oct 2022 20:25:16 +02:00

x86/resctrl: Fix min_cbm_bits for AMD

AMD systems support zero CBM (capacity bit mask) for cache allocation.
That is reflected in rdt_init_res_defs_amd() by:

  r->cache.arch_has_empty_bitmaps = true;

However given the unified code in cbm_validate(), checking for:

  val == 0 && !arch_has_empty_bitmaps

is not enough because of another check in cbm_validate():

  if ((zero_bit - first_bit) < r->cache.min_cbm_bits)

The default value of r->cache.min_cbm_bits = 1.

Leading to:

  $ cd /sys/fs/resctrl
  $ mkdir foo
  $ cd foo
  $ echo L3:0=0 > schemata
    -bash: echo: write error: Invalid argument
  $ cat /sys/fs/resctrl/info/last_cmd_status
    Need at least 1 bits in the mask

Initialize the min_cbm_bits to 0 for AMD. Also, remove the default
setting of min_cbm_bits and initialize it separately.

After the fix:

  $ cd /sys/fs/resctrl
  $ mkdir foo
  $ cd foo
  $ echo L3:0=0 > schemata
  $ cat /sys/fs/resctrl/info/last_cmd_status
    ok

Fixes: 316e7f901f5a ("x86/resctrl: Add struct rdt_cache::arch_has_{sparse, empty}_bitmaps")
Co-developed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
Cc: <stable@vger.kernel.org>
Link: https://lore.kernel.org/lkml/20220517001234.3137157-1-eranian@google.com
---
 arch/x86/kernel/cpu/resctrl/core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index de62b0b..3266ea3 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -66,9 +66,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.rid			= RDT_RESOURCE_L3,
 			.name			= "L3",
 			.cache_level		= 3,
-			.cache = {
-				.min_cbm_bits	= 1,
-			},
 			.domains		= domain_init(RDT_RESOURCE_L3),
 			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
@@ -83,9 +80,6 @@ struct rdt_hw_resource rdt_resources_all[] = {
 			.rid			= RDT_RESOURCE_L2,
 			.name			= "L2",
 			.cache_level		= 2,
-			.cache = {
-				.min_cbm_bits	= 1,
-			},
 			.domains		= domain_init(RDT_RESOURCE_L2),
 			.parse_ctrlval		= parse_cbm,
 			.format_str		= "%d=%0*x",
@@ -836,6 +830,7 @@ static __init void rdt_init_res_defs_intel(void)
 			r->cache.arch_has_sparse_bitmaps = false;
 			r->cache.arch_has_empty_bitmaps = false;
 			r->cache.arch_has_per_cpu_cfg = false;
+			r->cache.min_cbm_bits = 1;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_THRTL_BASE;
 			hw_res->msr_update = mba_wrmsr_intel;
@@ -856,6 +851,7 @@ static __init void rdt_init_res_defs_amd(void)
 			r->cache.arch_has_sparse_bitmaps = true;
 			r->cache.arch_has_empty_bitmaps = true;
 			r->cache.arch_has_per_cpu_cfg = true;
+			r->cache.min_cbm_bits = 0;
 		} else if (r->rid == RDT_RESOURCE_MBA) {
 			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
 			hw_res->msr_update = mba_wrmsr_amd;