[PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Babu Moger]

This series adds a bug fix for AMD and a minor code cleanup.

---
v5:
  This is bug. So, submitting these patches as a separate series from the old v4 series.
  This was a comment from James Morse. 

v4
https://lore.kernel.org/lkml/166257348081.1043018.11227924488792315932.stgit@bmoger-ubuntu/

Babu Moger (2):
      x86/resctrl: Fix min_cbm_bits for AMD
      x86/resctrl: Remove arch_has_empty_bitmaps


 arch/x86/kernel/cpu/resctrl/core.c        | 10 ++--------
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  3 +--
 include/linux/resctrl.h                   |  6 +++---
 3 files changed, 6 insertions(+), 13 deletions(-)

--

[PATCH v5 1/2] x86/resctrl: Fix min_cbm_bits for AMD

[Babu Moger]

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

Fix the issue by initializing 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

Link: https://lore.kernel.org/lkml/20220517001234.3137157-1-eranian@google.com/
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>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
Reviewed-by: James Morse <james.morse@arm.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.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 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 v5 1/2] x86/resctrl: Fix min_cbm_bits for AMD

[Yu, Fenghua]

> 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
> 
> Fix the issue by initializing 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
> 
> Link: https://lore.kernel.org/lkml/20220517001234.3137157-1-
> eranian@google.com/
> 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>
> 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>

[PATCH v5 2/2] x86/resctrl: Remove arch_has_empty_bitmaps

[Babu Moger]

The field arch_has_empty_bitmaps is not required anymore. The field
min_cbm_bits is enough to validate the CBM (capacity bit mask) if the
architecture can support the zero CBM or not.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/core.c        |    2 --
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |    3 +--
 include/linux/resctrl.h                   |    6 +++---
 3 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index a5c51a14fbce..c2657754672e 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -869,7 +869,6 @@ static __init void rdt_init_res_defs_intel(void)
 		if (r->rid == RDT_RESOURCE_L3 ||
 		    r->rid == RDT_RESOURCE_L2) {
 			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) {
@@ -890,7 +889,6 @@ static __init void rdt_init_res_defs_amd(void)
 		if (r->rid == RDT_RESOURCE_L3 ||
 		    r->rid == RDT_RESOURCE_L2) {
 			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) {
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 87666275eed9..7f38c8bd8429 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -98,8 +98,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 		return false;
 	}
 
-	if ((!r->cache.arch_has_empty_bitmaps && val == 0) ||
-	    val > r->default_ctrl) {
+	if ((r->cache.min_cbm_bits > 0 && val == 0) || val > r->default_ctrl) {
 		rdt_last_cmd_puts("Mask out of range\n");
 		return false;
 	}
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 21deb5212bbd..46ed8589857c 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -72,11 +72,12 @@ struct rdt_domain {
 /**
  * struct resctrl_cache - Cache allocation related data
  * @cbm_len:		Length of the cache bit mask
- * @min_cbm_bits:	Minimum number of consecutive bits to be set
+ * @min_cbm_bits:	Minimum number of consecutive bits to be set.
+ *			The value 0 means the architecture can support
+ *			zero CBM.
  * @shareable_bits:	Bitmask of shareable resource with other
  *			executing entities
  * @arch_has_sparse_bitmaps:	True if a bitmap like f00f is valid.
- * @arch_has_empty_bitmaps:	True if the '0' bitmap is valid.
  * @arch_has_per_cpu_cfg:	True if QOS_CFG register for this cache
  *				level has CPU scope.
  */
@@ -85,7 +86,6 @@ struct resctrl_cache {
 	unsigned int	min_cbm_bits;
 	unsigned int	shareable_bits;
 	bool		arch_has_sparse_bitmaps;
-	bool		arch_has_empty_bitmaps;
 	bool		arch_has_per_cpu_cfg;
 };
 

RE: [PATCH v5 2/2] x86/resctrl: Remove arch_has_empty_bitmaps

[Yu, Fenghua]

> The field arch_has_empty_bitmaps is not required anymore. The field
> min_cbm_bits is enough to validate the CBM (capacity bit mask) if the
> architecture can support the zero CBM or not.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Re: [PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Reinette Chatre]

Hi X86 Maintainers,

Could you please consider this fix and cleanup for inclusion?

Thank you very much

Reinette

On 9/27/2022 1:16 PM, Babu Moger wrote:
> This series adds a bug fix for AMD and a minor code cleanup.
> 
> ---
> v5:
>   This is bug. So, submitting these patches as a separate series from the old v4 series.
>   This was a comment from James Morse. 
> 
> v4
> https://lore.kernel.org/lkml/166257348081.1043018.11227924488792315932.stgit@bmoger-ubuntu/
> 
> Babu Moger (2):
>       x86/resctrl: Fix min_cbm_bits for AMD
>       x86/resctrl: Remove arch_has_empty_bitmaps
> 
> 
>  arch/x86/kernel/cpu/resctrl/core.c        | 10 ++--------
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  3 +--
>  include/linux/resctrl.h                   |  6 +++---
>  3 files changed, 6 insertions(+), 13 deletions(-)
> 
> --
> 
> 

Re: [PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Borislav Petkov]

On Tue, Oct 18, 2022 at 09:26:41AM -0700, Reinette Chatre wrote:
> Hi X86 Maintainers,
> 
> Could you please consider this fix and cleanup for inclusion?

Sure.

From the looks of it, the first one needs to go to stable, right?

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Reinette Chatre]

Hi Boris,

On 10/18/2022 9:35 AM, Borislav Petkov wrote:
> On Tue, Oct 18, 2022 at 09:26:41AM -0700, Reinette Chatre wrote:
>> Hi X86 Maintainers,
>>
>> Could you please consider this fix and cleanup for inclusion?
> 
> Sure.

Thank you very much.

> 
> From the looks of it, the first one needs to go to stable, right?

Correct. Apologies for missing the "Cc: stable@vger.kernel.org" in
that one.

Reinette
 

Re: [PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Borislav Petkov]

On Tue, Oct 18, 2022 at 09:43:15AM -0700, Reinette Chatre wrote:
> Correct. Apologies for missing the "Cc: stable@vger.kernel.org" in
> that one.

Nothing to apologize, all good.

Lemme take care of them.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [PATCH v5 0/2] x86/resctrl: Fix min_cbm_bits for AMD and code cleanup

[Moger, Babu]

On 10/18/22 11:47, Borislav Petkov wrote:
> On Tue, Oct 18, 2022 at 09:43:15AM -0700, Reinette Chatre wrote:
>> Correct. Apologies for missing the "Cc: stable@vger.kernel.org" in
>> that one.
> Nothing to apologize, all good.
>
> Lemme take care of them.
>
Thank you Reinette, Boris.