Re: [RFC PATCH 03/10] arch/x86: Re-arrange RDT init code

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 9/24/2018 12:19 PM, Moger, Babu wrote:
> Re-organize the RDT init code. Separate the call sequence for each
> feature. That way, it is easy to call quirks or features separately
> for each vendor if there are differences.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/rdt.c | 44 ++++++++++++++++++++++++++++-----------
>  1 file changed, 32 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/rdt.c b/arch/x86/kernel/cpu/rdt.c
> index b361c63170d7..736715b81fd8 100644
> --- a/arch/x86/kernel/cpu/rdt.c
> +++ b/arch/x86/kernel/cpu/rdt.c
> @@ -813,10 +813,6 @@ static __init bool get_rdt_alloc_resources(void)
>  		ret = true;
>  	}
>  
> -	if (rdt_cpu_has(X86_FEATURE_MBA)) {
> -		if (rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]))
> -			ret = true;
> -	}

The commit message mentions that the call sequence for each feature is
separated, but here only the MBA feature is separated.

The MBA feature detection is removed above .... (more later)

>  	return ret;
>  }
>  
> @@ -831,11 +827,12 @@ static __init bool get_rdt_mon_resources(void)
>  
>  	if (!rdt_mon_features)
>  		return false;
> +	else
> +		return true;
>  
> -	return !rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);
>  }
>  
> -static __init void rdt_quirks(void)
> +static __init void rdt_quirks_intel(void)
>  {
>  	switch (boot_cpu_data.x86_model) {
>  	case INTEL_FAM6_HASWELL_X:
> @@ -850,13 +847,22 @@ static __init void rdt_quirks(void)
>  	}
>  }
>  
> -static __init bool get_rdt_resources(void)
> +static __init void rdt_quirks(void)
>  {
> -	rdt_quirks();
> -	rdt_alloc_capable = get_rdt_alloc_resources();
> -	rdt_mon_capable = get_rdt_mon_resources();
> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
> +		rdt_quirks_intel();
> +}
> +
> +static __init void rdt_detect_l3_mon(void)
> +{
> +	if (rdt_mon_capable)
> +		rdt_get_mon_l3_config(&rdt_resources_all[RDT_RESOURCE_L3]);

The possible errors from this configuration is now lost.

> +}
>  
> -	return (rdt_mon_capable || rdt_alloc_capable);
> +static __init void rdt_check_mba(void)
> +{
> +	if (rdt_cpu_has(X86_FEATURE_MBA))
> +		rdt_get_mem_config(&rdt_resources_all[RDT_RESOURCE_MBA]);

Here too the possible failure of this configuration is now lost.

>  }
>  
>  static enum cpuhp_state rdt_online;
> @@ -866,8 +872,22 @@ static int __init rdt_late_init(void)
>  	struct rdt_resource *r;
>  	int state, ret;
>  
> -	if (!get_rdt_resources())
> +	/* Run quirks first */
> +	rdt_quirks();
> +
> +	rdt_alloc_capable = get_rdt_alloc_resources();
> +	rdt_mon_capable = get_rdt_mon_resources();
> +
> +	if (!(rdt_alloc_capable || rdt_mon_capable)) {
> +		pr_info("RDT allocation or monitoring not detected\n");

This function ends with a log entry for every resource discovered. Is
this new log entry needed to indicate that such resources have not been
found? Could it not just be the absence of the other message?

>  		return -ENODEV;
> +	}

... (continued from above) ... since the MBA feature detection was
removed from get_rdt_alloc_resources() would the above not cause failure
on systems that only support MBA?

> +
> +	/* Detect l3 monitoring resources */

I do not think this comment is accurate ... has the monitoring resources
not been detected earlier in get_rdt_mon_resources() and now they will
be configured?

> +	rdt_detect_l3_mon();
> +
> +	/* Check for Memory Bandwidth Allocation */
> +	rdt_check_mba();

To follow up on above .. the potential failure of these configurations
are now lost here. Initialization should not continue if these
configurations failed.

>  
>  	rdt_init_padding();
>  
> 

Reinette