Re: [RFC PATCH 1/3] x86/resctrl: Get max rmid and occupancy scale directly from CPUID instead of cpuinfo_x86

[Borislav Petkov <bp@alien8.de>]

On Thu, Jun 13, 2019 at 01:51:02PM -0700, Fenghua Yu wrote:
> Although x86_cache_max_rmid and x86_cache_occ_scale are read only once
> during resctrl initialization, they are always stored in cpuinfo_x86 on
> each CPU during run time even if resctrl is not configured.
> 
> To save cpuinfo_x86 space and make CPU and resctrl initialization simpler,
> remove the two fields from cpuinfo_x86 and get max rmid and occupancy
> scale directly from CPUID during resctrl initialization. And since each
> known platform that supports resctrl has same max rmid on all CPUs, no
> need to scan all CPUs to find minimum of max rmid values, i.e. getting
> max rmid from CPUID on the current CPU is fine.
> 
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
>  arch/x86/include/asm/processor.h       |  3 ---
>  arch/x86/kernel/cpu/common.c           | 28 --------------------------
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +-
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 28 +++++++++++++++++++++++---
>  4 files changed, 26 insertions(+), 35 deletions(-)
> 
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index c34a35c78618..27e875d4ca7d 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -99,9 +99,6 @@ struct cpuinfo_x86 {
>  	/* in KB - valid for CPUS which support this call: */
>  	unsigned int		x86_cache_size;
>  	int			x86_cache_alignment;	/* In bytes */
> -	/* Cache QoS architectural values: */
> -	int			x86_cache_max_rmid;	/* max index */
> -	int			x86_cache_occ_scale;	/* scale to bytes */
>  	int			x86_power;
>  	unsigned long		loops_per_jiffy;
>  	/* cpuid returned max cores value: */
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 2c57fffebf9b..38e4b1a9005e 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -840,22 +840,9 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
>  		c->x86_capability[CPUID_F_0_EDX] = edx;
>  
>  		if (cpu_has(c, X86_FEATURE_CQM_LLC)) {
> -			/* will be overridden if occupancy monitoring exists */
> -			c->x86_cache_max_rmid = ebx;
> -
>  			/* QoS sub-leaf, EAX=0Fh, ECX=1 */
>  			cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);
>  			c->x86_capability[CPUID_F_1_EDX] = edx;
> -
> -			if ((cpu_has(c, X86_FEATURE_CQM_OCCUP_LLC)) ||
> -			      ((cpu_has(c, X86_FEATURE_CQM_MBM_TOTAL)) ||
> -			       (cpu_has(c, X86_FEATURE_CQM_MBM_LOCAL)))) {
> -				c->x86_cache_max_rmid = ecx;
> -				c->x86_cache_occ_scale = ebx;
> -			}
> -		} else {
> -			c->x86_cache_max_rmid = -1;
> -			c->x86_cache_occ_scale = -1;
>  		}
>  	}
>  
> @@ -1269,20 +1256,6 @@ static void generic_identify(struct cpuinfo_x86 *c)
>  #endif
>  }
>  
> -static void x86_init_cache_qos(struct cpuinfo_x86 *c)
> -{
> -	/*
> -	 * The heavy lifting of max_rmid and cache_occ_scale are handled
> -	 * in get_cpu_cap().  Here we just set the max_rmid for the boot_cpu
> -	 * in case CQM bits really aren't there in this CPU.
> -	 */
> -	if (c != &boot_cpu_data) {
> -		boot_cpu_data.x86_cache_max_rmid =
> -			min(boot_cpu_data.x86_cache_max_rmid,
> -			    c->x86_cache_max_rmid);
> -	}
> -}
> -
>  /*
>   * Validate that ACPI/mptables have the same information about the
>   * effective APIC id and update the package map.
> @@ -1391,7 +1364,6 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>  #endif
>  
>  	x86_init_rdrand(c);
> -	x86_init_cache_qos(c);
>  	setup_pku(c);
>  
>  	/*
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e49b77283924..474a7090d2dd 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -579,7 +579,7 @@ int closids_supported(void);
>  void closid_free(int closid);
>  int alloc_rmid(void);
>  void free_rmid(u32 rmid);
> -int rdt_get_mon_l3_config(struct rdt_resource *r);
> +int __init rdt_get_mon_l3_config(struct rdt_resource *r);
>  void mon_event_count(void *info);
>  int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>  void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r,
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 1573a0a6b525..e9d876c25703 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -617,13 +617,35 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>  		list_add_tail(&mbm_local_event.list, &r->evt_list);
>  }
>  
> -int rdt_get_mon_l3_config(struct rdt_resource *r)
> +static void __init get_cqm_info(struct rdt_resource *r)
> +{
> +	u32 eax, ebx, ecx, edx;
> +
> +	/*
> +	 * At this point, CQM LLC and one of occupancy, MBM total, and
> +	 * MBM local monitoring features must be supported.
> +	 */
> +	cpuid_count(0x0000000F, 0, &eax, &ebx, &ecx, &edx);
> +	/* will be overridden if occupancy monitoring exists */
> +	r->num_rmid = ebx + 1;
> +
> +	cpuid_count(0x0000000F, 1, &eax, &ebx, &ecx, &edx);

Those CPUID accesses should be done *after* testing features, not
before.

> +
> +	if (boot_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))

That is already done in get_rdt_mon_resources() and rdt_mon_features
caches those bits. I think you wanna test QOS_L3_OCCUP_EVENT_ID in there
and then read CPUID 0xf and set ->num_rmid.

> +		r->num_rmid = ecx + 1;
> +
> +	if (boot_cpu_has(X86_FEATURE_CQM_MBM_TOTAL) || boot_cpu_has(X86_FEATURE_CQM_MBM_LOCAL))

Ditto.

Other than that, I like where this cleanup is going...

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.