[AMD Official Use Only - General] Hi Reinette, > -----Original Message----- > From: Reinette Chatre > Sent: Wednesday, November 30, 2022 6:36 PM > To: Moger, Babu ; corbet@lwn.net; > tglx@linutronix.de; mingo@redhat.com; bp@alien8.de > Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org; > hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org; > quic_neeraju@quicinc.com; rdunlap@infradead.org; > damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com; > peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com; > chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com; > jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan > ; tony.luck@intel.com; james.morse@arm.com; > linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org; > bagasdotme@gmail.com; eranian@google.com > Subject: Re: [PATCH v8 05/13] x86/resctrl: Detect and configure Slow Memory > Bandwidth Allocation > > Hi Babu, > > On 11/30/2022 12:40 PM, Moger, Babu wrote: > > On 11/30/22 14:07, Reinette Chatre wrote: > >> 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 > >>>>> --- > >>>>> 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. > >> > > I dont have any strong argument. I was thinking, in case Intel > > supports this resource in the future then they only have to change > > rdt_init_res_defs_intel. > > I agree that this is not a strong argument. If this happens then Intel can split the > initialization also. This is also not the only bits that would need changing since > only __rdt_get_mem_config_amd() can initialize an SMBA resource. > > It does not sound like there is a clear winner. To answer your earlier question > more succinctly, yes, from my perspective you can keep the change to > rdt_init_res_defs_amd(). At least with this change things would be more > familiar between MBA and SMBA and it will be obvious that SMBA is not > supported by Intel. Will do. Thanks Babu