RE: [PATCH v3 09/10] x86/resctrl: Add arch_has_{sparse,empty}_bitmaps to explain CAT differences

2020-05-22 Thread Babu Moger 



> -Original Message-
> From: James Morse 
> Sent: Monday, May 18, 2020 8:19 AM
> To: x...@kernel.org; linux-kernel@vger.kernel.org
> Cc: Fenghua Yu ; Reinette Chatre
> ; Thomas Gleixner ; Ingo
> Molnar ; Borislav Petkov ; H Peter Anvin
> ; Moger, Babu ; James Morse
> 
> Subject: [PATCH v3 09/10] x86/resctrl: Add arch_has_{sparse,empty}_bitmaps to
> explain CAT differences
> 
> Intel CPUs expect the cache bitmap provided by user-space to have on
> a single span of 1s, whereas AMD can support bitmaps like 0xf00f.
> Arm's MPAM support also allows sparse bitmaps.
> 
> Similarly, Intel CPUs check at least one bit set, whereas AMD CPUs
> are quite happy with an empty bitmap. Arm's MPAM allows an empty
> bitmap.
> 
> To move resctrl out to /fs/ we need to explain platform differences
> like this. Add two resource properties arch_has_{empty,sparse}_bitmaps.
> Test these around the relevant parts of cbm_validate().
> 
> Merging the validate calls causes AMD to gain the min_cbm_bits test
> needed for Haswell, but as it always sets this value to 1, it will
> never match.
> 
> CC: Babu Moger 
> CC: Reinette Chatre 
> Signed-off-by: James Morse 

Reviewed-by: Babu Moger 

> ---
>  arch/x86/kernel/cpu/resctrl/core.c| 14 
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 39 ++-
>  arch/x86/kernel/cpu/resctrl/internal.h|  8 ++---
>  3 files changed, 22 insertions(+), 39 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> b/arch/x86/kernel/cpu/resctrl/core.c
> index 223e5b90bcfd..587f9791d2a6 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -922,9 +922,10 @@ static __init void rdt_init_res_defs_intel(void)
>   r->rid == RDT_RESOURCE_L3CODE ||
>   r->rid == RDT_RESOURCE_L2 ||
>   r->rid == RDT_RESOURCE_L2DATA ||
> - r->rid == RDT_RESOURCE_L2CODE)
> - r->cbm_validate = cbm_validate_intel;
> - else if (r->rid == RDT_RESOURCE_MBA) {
> + r->rid == RDT_RESOURCE_L2CODE) {
> + r->cache.arch_has_sparse_bitmaps = false;
> + r->cache.arch_has_empty_bitmaps = false;
> + } else if (r->rid == RDT_RESOURCE_MBA) {
>   r->msr_base = MSR_IA32_MBA_THRTL_BASE;
>   r->msr_update = mba_wrmsr_intel;
>   }
> @@ -941,9 +942,10 @@ static __init void rdt_init_res_defs_amd(void)
>   r->rid == RDT_RESOURCE_L3CODE ||
>   r->rid == RDT_RESOURCE_L2 ||
>   r->rid == RDT_RESOURCE_L2DATA ||
> - r->rid == RDT_RESOURCE_L2CODE)
> - r->cbm_validate = cbm_validate_amd;
> - else if (r->rid == RDT_RESOURCE_MBA) {
> + r->rid == RDT_RESOURCE_L2CODE) {
> + r->cache.arch_has_sparse_bitmaps = true;
> + r->cache.arch_has_empty_bitmaps = true;
> + } else if (r->rid == RDT_RESOURCE_MBA) {
>   r->msr_base = MSR_IA32_MBA_BW_BASE;
>   r->msr_update = mba_wrmsr_amd;
>   }
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b0e24cb6f85c..cc854bb369c9 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -76,12 +76,14 @@ int parse_bw(struct rdt_parse_data *data, struct
> rdt_resource *r,
>  }
> 
>  /*
> - * Check whether a cache bit mask is valid. The SDM says:
> + * Check whether a cache bit mask is valid.
> + * For Intel the SDM says:
>   *   Please note that all (and only) contiguous '1' combinations
>   *   are allowed (e.g. H, 0FF0H, 003CH, etc.).
>   * Additionally Haswell requires at least two bits set.
> + * AMD allows non-contiguous bitmasks.
>   */
> -bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r)
> +static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>  {
>   unsigned long first_bit, zero_bit, val;
>   unsigned int cbm_len = r->cache.cbm_len;
> @@ -93,7 +95,8 @@ bool cbm_validate_intel(char *buf, u32 *data, struct
> rdt_resource *r)
>   return false;
>   }
> 
> - if (val == 0 || val > r->default_ctrl) {
> + if ((!r->cache.arch_has_empty_bitmaps && val == 0) ||
> + (val > r->default_ctrl)) {
>   rdt_last_cmd_puts("Mask out of range\n");
>   return false;
>

Re: [PATCH v3 09/10] x86/resctrl: Add arch_has_{sparse,empty}_bitmaps to explain CAT differences

2020-05-19 Thread Reinette Chatre 
Hi James,

Thank you very much for adding the handling of empty bitmaps. This looks
good to me, just one comment ...

On 5/18/2020 6:19 AM, James Morse wrote:
> -bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r)
> +static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
>  {
>   unsigned long first_bit, zero_bit, val;
>   unsigned int cbm_len = r->cache.cbm_len;
> @@ -93,7 +95,8 @@ bool cbm_validate_intel(char *buf, u32 *data, struct 
> rdt_resource *r)
>   return false;
>   }
>  
> - if (val == 0 || val > r->default_ctrl) {
> + if ((!r->cache.arch_has_empty_bitmaps && val == 0) ||
> + (val > r->default_ctrl)) {
>   rdt_last_cmd_puts("Mask out of range\n");
>   return false;

There is unnecessary parentheses around 'val > r->default_ctrl'

Reinette

[PATCH v3 09/10] x86/resctrl: Add arch_has_{sparse,empty}_bitmaps to explain CAT differences

2020-05-18 Thread James Morse 
Intel CPUs expect the cache bitmap provided by user-space to have on
a single span of 1s, whereas AMD can support bitmaps like 0xf00f.
Arm's MPAM support also allows sparse bitmaps.

Similarly, Intel CPUs check at least one bit set, whereas AMD CPUs
are quite happy with an empty bitmap. Arm's MPAM allows an empty
bitmap.

To move resctrl out to /fs/ we need to explain platform differences
like this. Add two resource properties arch_has_{empty,sparse}_bitmaps.
Test these around the relevant parts of cbm_validate().

Merging the validate calls causes AMD to gain the min_cbm_bits test
needed for Haswell, but as it always sets this value to 1, it will
never match.

CC: Babu Moger 
CC: Reinette Chatre 
Signed-off-by: James Morse 
---
 arch/x86/kernel/cpu/resctrl/core.c| 14 
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 39 ++-
 arch/x86/kernel/cpu/resctrl/internal.h|  8 ++---
 3 files changed, 22 insertions(+), 39 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/core.c 
b/arch/x86/kernel/cpu/resctrl/core.c
index 223e5b90bcfd..587f9791d2a6 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -922,9 +922,10 @@ static __init void rdt_init_res_defs_intel(void)
r->rid == RDT_RESOURCE_L3CODE ||
r->rid == RDT_RESOURCE_L2 ||
r->rid == RDT_RESOURCE_L2DATA ||
-   r->rid == RDT_RESOURCE_L2CODE)
-   r->cbm_validate = cbm_validate_intel;
-   else if (r->rid == RDT_RESOURCE_MBA) {
+   r->rid == RDT_RESOURCE_L2CODE) {
+   r->cache.arch_has_sparse_bitmaps = false;
+   r->cache.arch_has_empty_bitmaps = false;
+   } else if (r->rid == RDT_RESOURCE_MBA) {
r->msr_base = MSR_IA32_MBA_THRTL_BASE;
r->msr_update = mba_wrmsr_intel;
}
@@ -941,9 +942,10 @@ static __init void rdt_init_res_defs_amd(void)
r->rid == RDT_RESOURCE_L3CODE ||
r->rid == RDT_RESOURCE_L2 ||
r->rid == RDT_RESOURCE_L2DATA ||
-   r->rid == RDT_RESOURCE_L2CODE)
-   r->cbm_validate = cbm_validate_amd;
-   else if (r->rid == RDT_RESOURCE_MBA) {
+   r->rid == RDT_RESOURCE_L2CODE) {
+   r->cache.arch_has_sparse_bitmaps = true;
+   r->cache.arch_has_empty_bitmaps = true;
+   } else if (r->rid == RDT_RESOURCE_MBA) {
r->msr_base = MSR_IA32_MBA_BW_BASE;
r->msr_update = mba_wrmsr_amd;
}
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c 
b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b0e24cb6f85c..cc854bb369c9 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -76,12 +76,14 @@ int parse_bw(struct rdt_parse_data *data, struct 
rdt_resource *r,
 }
 
 /*
- * Check whether a cache bit mask is valid. The SDM says:
+ * Check whether a cache bit mask is valid.
+ * For Intel the SDM says:
  * Please note that all (and only) contiguous '1' combinations
  * are allowed (e.g. H, 0FF0H, 003CH, etc.).
  * Additionally Haswell requires at least two bits set.
+ * AMD allows non-contiguous bitmasks.
  */
-bool cbm_validate_intel(char *buf, u32 *data, struct rdt_resource *r)
+static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r)
 {
unsigned long first_bit, zero_bit, val;
unsigned int cbm_len = r->cache.cbm_len;
@@ -93,7 +95,8 @@ bool cbm_validate_intel(char *buf, u32 *data, struct 
rdt_resource *r)
return false;
}
 
-   if (val == 0 || val > r->default_ctrl) {
+   if ((!r->cache.arch_has_empty_bitmaps && val == 0) ||
+   (val > r->default_ctrl)) {
rdt_last_cmd_puts("Mask out of range\n");
return false;
}
@@ -101,7 +104,9 @@ bool cbm_validate_intel(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);
 
-   if (find_next_bit(&val, cbm_len, zero_bit) < cbm_len) {
+   /* Are non-contiguous bitmaps allowed? */
+   if (!r->cache.arch_has_sparse_bitmaps &&
+   (find_next_bit(&val, cbm_len, zero_bit) < cbm_len)) {
rdt_last_cmd_printf("The mask %lx has non-consecutive 
1-bits\n", val);
return false;
}
@@ -116,30 +121,6 @@ bool cbm_validate_intel(char *buf, u32 *data, struct 
rdt_resource *r)
return true;
 }
 
-/*
- * Check whether a cache bit mask is valid. AMD allows non-contiguous
- * bitmasks
- */
-bool cbm_validate_amd(char *buf, u32 *data, struct rdt_resource *r)
-{
-   unsigned long val;
-   int ret;
-
-   ret = kstrtoul(buf, 16, &val);
-   if (ret) {
-   rdt_