Re: [PATCH v8 05/13] x86/resctrl: Detect and configure Slow Memory Bandwidth Allocation

[Reinette Chatre]

Hi Babu,

On 11/30/2022 10:43 AM, Moger, Babu wrote:
> On 11/22/22 18:12, Reinette Chatre wrote:
>> On 11/4/2022 1:00 PM, Babu Moger wrote:
>>> The QoS slow memory configuration details are available via
>>> CPUID_Fn80000020_EDX_x02. Detect the available details and
>>> initialize the rest to defaults.
>>>
>>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/core.c        |   36 +++++++++++++++++++++++++++--
>>>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |    2 +-
>>>  arch/x86/kernel/cpu/resctrl/internal.h    |    1 +
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    |    8 ++++--
>>>  4 files changed, 41 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index e31c98e2fafc..6571d08e2b0d 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -162,6 +162,13 @@ bool is_mba_sc(struct rdt_resource *r)
>>>  	if (!r)
>>>  		return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;
>>>  
>>> +	/*
>>> +	 * The software controller support is only applicable to MBA resource.
>>> +	 * Make sure to check for resource type again.
>>> +	 */
>> /again/d
>>
>> Not all callers of is_mba_sc() check if it is called for an MBA resource.
>>
>>> +	if (r->rid != RDT_RESOURCE_MBA)
>>> +		return false;
>>> +
>>>  	return r->membw.mba_sc;
>>>  }
>>>  
>>> @@ -225,9 +232,15 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>>  	union cpuid_0x10_3_eax eax;
>>>  	union cpuid_0x10_x_edx edx;
>>> -	u32 ebx, ecx;
>>> +	u32 ebx, ecx, subleaf;
>>>  
>>> -	cpuid_count(0x80000020, 1, &eax.full, &ebx, &ecx, &edx.full);
>>> +	/*
>>> +	 * Query CPUID_Fn80000020_EDX_x01 for MBA and
>>> +	 * CPUID_Fn80000020_EDX_x02 for SMBA
>>> +	 */
>>> +	subleaf = (r->rid == RDT_RESOURCE_SMBA) ? 2 :  1;
>>> +
>>> +	cpuid_count(0x80000020, subleaf, &eax.full, &ebx, &ecx, &edx.full);
>>>  	hw_res->num_closid = edx.split.cos_max + 1;
>>>  	r->default_ctrl = MAX_MBA_BW_AMD;
>>>  
>>> @@ -750,6 +763,19 @@ static __init bool get_mem_config(void)
>>>  	return false;
>>>  }
>>>  
>>> +static __init bool get_slow_mem_config(void)
>>> +{
>>> +	struct rdt_hw_resource *hw_res = &rdt_resources_all[RDT_RESOURCE_SMBA];
>>> +
>>> +	if (!rdt_cpu_has(X86_FEATURE_SMBA))
>>> +		return false;
>>> +
>>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>>> +		return __rdt_get_mem_config_amd(&hw_res->r_resctrl);
>>> +
>>> +	return false;
>>> +}
>>> +
>>>  static __init bool get_rdt_alloc_resources(void)
>>>  {
>>>  	struct rdt_resource *r;
>>> @@ -780,6 +806,9 @@ static __init bool get_rdt_alloc_resources(void)
>>>  	if (get_mem_config())
>>>  		ret = true;
>>>  
>>> +	if (get_slow_mem_config())
>>> +		ret = true;
>>> +
>>>  	return ret;
>>>  }
>>>  
>>> @@ -869,6 +898,9 @@ static __init void rdt_init_res_defs_amd(void)
>>>  		} else if (r->rid == RDT_RESOURCE_MBA) {
>>>  			hw_res->msr_base = MSR_IA32_MBA_BW_BASE;
>>>  			hw_res->msr_update = mba_wrmsr_amd;
>>> +		} else if (r->rid == RDT_RESOURCE_SMBA) {
>>> +			hw_res->msr_base = MSR_IA32_SMBA_BW_BASE;
>>> +			hw_res->msr_update = mba_wrmsr_amd;
>>>  		}
>>>  	}
>>>  }
>> I mentioned earlier that this can be moved to init of
>> rdt_resources_all[]. No strong preference, leaving here works
>> also.
> 
> I am little confused about this comment. Initialization of
> rdt_resources_all in core.c is mostly generic initialization. The msr_base
> and msr_update routines here are vendor specific. I would prefer to keep
> this in

This is a contradiction. Yes, rdt_resources_all[] initialization in core.c
is indeed generic initialization, so why is SMBA there? If this was really
generic initialization then the entire initialization of SMBA resource
should rather move to AMD specific code.

SMBA is an AMD only feature yet its resource initialization is fragmented
with one portion treated as generic and another portion treated as vendor
specific while it all is vendor specific.

The current fragmentation is not clear to me. Keeping the initialization
as you have in patch #2 is the simplest and that is what prompted me
to suggest the move to keep initialization together at that location.

> 
> rdt_init_res_defs_amd.Is that ok?

The generic vs non-generic initialization argument is not convincing to me. 
Could you please elaborate why you prefer it this way? I already mentioned
that I do not have a strong preference but I would like to understand what
the motivation for this split initialization is.

Reinette