diff mbox series

[v7,05/12] x86/resctrl: Detect and configure Slow Memory Bandwidth allocation

Message ID 166604559954.5345.14619487558472213422.stgit@bmoger-ubuntu
State New
Headers
Series x86/resctrl: Support for AMD QoS new features |

Commit Message

Moger, Babu Oct. 17, 2022, 10:26 p.m. UTC 
  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@amd.com>
---
 arch/x86/kernel/cpu/resctrl/core.c        |   29 +++++++++++++++++++++++++++--
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |    2 +-
 arch/x86/kernel/cpu/resctrl/internal.h    |    1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    |   16 ++++++++++------
 4 files changed, 39 insertions(+), 9 deletions(-)

Comments

Reinette Chatre Oct. 25, 2022, 11:43 p.m. UTC | #1 
Hi Babu,

Nitpick in Subject ... "allocation" -> "Allocation"?

On 10/17/2022 3:26 PM, Babu Moger wrote:

...

> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>  
>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>  		r = s->res;
> -		if (r->rid == RDT_RESOURCE_MBA) {
> +		if (r->rid == RDT_RESOURCE_MBA ||
> +		    r->rid == RDT_RESOURCE_SMBA) {
>  			rdtgroup_init_mba(r, rdtgrp->closid);
>  			if (is_mba_sc(r))
>  				continue;

The above hunk and the ones that follow are unexpected.

Note that the software controller, when resctrl is mounted with "mba_MBps", is 
only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
over the place, for example:

static int set_mba_sc(bool mba_sc)
{
	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
	...

}

Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
controller", not "SMBA software controller". Why does it check above if the MBA software
controller is enabled on SMBA?			

> @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>  {
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
> +	if (supports_mba_mbps() &&
> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA))
>  		mba_sc_domain_destroy(r, d);
>  
>  	if (!r->mon_capable)
> @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
> -		/* RDT_RESOURCE_MBA is never mon_capable */
> +	if (supports_mba_mbps() &&
> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA))
> +		/* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */

What does this change do? Did you mean to add a r->rid == RDT_RESOURCE_SMBA check?

>  		return mba_sc_domain_allocate(r, d);
>  
>  	if (!r->mon_capable)
> 
> 

Why are the MBA software controller resources allocated/destroyed for a SMBA resource? If
you want to support the software controller for SMBA then there are a lot of other changes
missing.

Reinette
Moger, Babu Oct. 26, 2022, 7:07 p.m. UTC | #2 
Hi Reinette,

On 10/25/22 18:43, Reinette Chatre wrote:
> Hi Babu,
>
> Nitpick in Subject ... "allocation" -> "Allocation"?
Sure.
>
> On 10/17/2022 3:26 PM, Babu Moger wrote:
>
> ...
>
>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>  
>>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>>  		r = s->res;
>> -		if (r->rid == RDT_RESOURCE_MBA) {
>> +		if (r->rid == RDT_RESOURCE_MBA ||
>> +		    r->rid == RDT_RESOURCE_SMBA) {
>>  			rdtgroup_init_mba(r, rdtgrp->closid);
>>  			if (is_mba_sc(r))
>>  				continue;
> The above hunk and the ones that follow are unexpected.

I am thinking the above check is required, It is updating the
staged_config with default values. Right now, the default value for SMBA
is same as MBA default value. So, I used this code to initialize.

Did I miss something?

>
> Note that the software controller, when resctrl is mounted with "mba_MBps", is 
> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
> over the place, for example:
>
> static int set_mba_sc(bool mba_sc)
> {
> 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> 	...
>
> }
>
> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
> controller", not "SMBA software controller". Why does it check above if the MBA software
> controller is enabled on SMBA?

There is no plan to support SMBA software controller. Yes, I think below
checks are not required.


> 			
>
>> @@ -3287,7 +3289,8 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>>  {
>>  	lockdep_assert_held(&rdtgroup_mutex);
>>  
>> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
>> +	if (supports_mba_mbps() &&
>> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA))
>>  		mba_sc_domain_destroy(r, d);
This check is not required.
>>  
>>  	if (!r->mon_capable)
>> @@ -3354,8 +3357,9 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>>  
>>  	lockdep_assert_held(&rdtgroup_mutex);
>>  
>> -	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
>> -		/* RDT_RESOURCE_MBA is never mon_capable */
>> +	if (supports_mba_mbps() &&
>> +	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA))
>> +		/* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */
> What does this change do? Did you mean to add a r->rid == RDT_RESOURCE_SMBA check?

Good catch. I meant  r->rid == RDT_RESOURCE_SMBA.

But this check is not required at all.

>
>>  		return mba_sc_domain_allocate(r, d);
>>  
>>  	if (!r->mon_capable)
>>
>>
> Why are the MBA software controller resources allocated/destroyed for a SMBA resource? If
> you want to support the software controller for SMBA then there are a lot of other changes

No..There is no plan to support software controller for SMBA.

Thanks

Babu
Reinette Chatre Oct. 26, 2022, 8:23 p.m. UTC | #3 
Hi Babu,

On 10/26/2022 12:07 PM, Moger, Babu wrote:
> On 10/25/22 18:43, Reinette Chatre wrote:
>> On 10/17/2022 3:26 PM, Babu Moger wrote:
>>
>> ...
>>
>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>>  
>>>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>>>  		r = s->res;
>>> -		if (r->rid == RDT_RESOURCE_MBA) {
>>> +		if (r->rid == RDT_RESOURCE_MBA ||
>>> +		    r->rid == RDT_RESOURCE_SMBA) {
>>>  			rdtgroup_init_mba(r, rdtgrp->closid);
>>>  			if (is_mba_sc(r))
>>>  				continue;
>> The above hunk and the ones that follow are unexpected.
> 
> I am thinking the above check is required, It is updating the
> staged_config with default values. Right now, the default value for SMBA
> is same as MBA default value. So, I used this code to initialize.
> 
> Did I miss something?

As I described in the following comments my concern is related to all the
software controller code still executing for SMBA. Yes, in the above hunk
SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
software controller checks and in the above hunk its call is also followed
by another software controller check.

The software controller is just applicable to MBA and these checks have been
isolated to the MBA resource. Using it for SMBA that does not support
software controller at all is making the code harder to follow and sets this
code up for future mistakes. I think it would make the code easier to understand
if this is made very clear that software controller is not applicable to SMBA at
all instead of repurposing these flows.

>> Note that the software controller, when resctrl is mounted with "mba_MBps", is 
>> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
>> over the place, for example:
>>
>> static int set_mba_sc(bool mba_sc)
>> {
>> 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>> 	...
>>
>> }
>>
>> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
>> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
>> controller", not "SMBA software controller". Why does it check above if the MBA software
>> controller is enabled on SMBA?
> 
> There is no plan to support SMBA software controller. Yes, I think below
> checks are not required.

Thank you for clarifying. Having the code reflect that clearly would make everything
easier to understand and maintain.

Reinette
Moger, Babu Oct. 27, 2022, 3:30 p.m. UTC | #4 
Hi Reinette,

On 10/26/22 15:23, Reinette Chatre wrote:
> Hi Babu,
>
> On 10/26/2022 12:07 PM, Moger, Babu wrote:
>> On 10/25/22 18:43, Reinette Chatre wrote:
>>> On 10/17/2022 3:26 PM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>>>  
>>>>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>>>>  		r = s->res;
>>>> -		if (r->rid == RDT_RESOURCE_MBA) {
>>>> +		if (r->rid == RDT_RESOURCE_MBA ||
>>>> +		    r->rid == RDT_RESOURCE_SMBA) {
>>>>  			rdtgroup_init_mba(r, rdtgrp->closid);
>>>>  			if (is_mba_sc(r))
>>>>  				continue;
>>> The above hunk and the ones that follow are unexpected.
>> I am thinking the above check is required, It is updating the
>> staged_config with default values. Right now, the default value for SMBA
>> is same as MBA default value. So, I used this code to initialize.
>>
>> Did I miss something?
> As I described in the following comments my concern is related to all the
> software controller code still executing for SMBA. Yes, in the above hunk
> SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
> software controller checks and in the above hunk its call is also followed
> by another software controller check.
>
> The software controller is just applicable to MBA and these checks have been
> isolated to the MBA resource. Using it for SMBA that does not support
> software controller at all is making the code harder to follow and sets this
> code up for future mistakes. I think it would make the code easier to understand
> if this is made very clear that software controller is not applicable to SMBA at
> all instead of repurposing these flows.

Yes. Understood.  How about this? I feel this is much more cleaner.

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e5a48f05e787..d91a6a513681 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup
*rdtgrp)
 
        list_for_each_entry(s, &resctrl_schema_all, list) {
                r = s->res;
-               if (r->rid == RDT_RESOURCE_MBA) {
+               if (r->rid == RDT_RESOURCE_MBA ||
+                   r->rid == RDT_RESOURCE_SMBA) {
                        rdtgroup_init_mba(r, rdtgrp->closid);
-                       if (is_mba_sc(r))
-                               continue;
                } else {
                        ret = rdtgroup_init_cat(s, rdtgrp->closid);
                        if (ret < 0)
                                return ret;
                }
 
+               if (is_mba_sc(r))
+                       continue;
+
                ret = resctrl_arch_update_domains(r, rdtgrp->closid);
                if (ret < 0) {
                        rdt_last_cmd_puts("Failed to initialize
allocations\n");

Thanks

Babu

>
>>> Note that the software controller, when resctrl is mounted with "mba_MBps", is 
>>> only supported by RDT_RESOURCE_MBA. At this time this really is hard coded all
>>> over the place, for example:
>>>
>>> static int set_mba_sc(bool mba_sc)
>>> {
>>> 	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>>> 	...
>>>
>>> }
>>>
>>> Since SMBA hardcodes "delay_linear = false" I do not expect it to support the software
>>> controller ... but these hunks appear to treat SMBA as though it does. It is the "MBA software
>>> controller", not "SMBA software controller". Why does it check above if the MBA software
>>> controller is enabled on SMBA?
>> There is no plan to support SMBA software controller. Yes, I think below
>> checks are not required.
> Thank you for clarifying. Having the code reflect that clearly would make everything
> easier to understand and maintain.
>
> Reinette
>
Reinette Chatre Oct. 27, 2022, 6:37 p.m. UTC | #5 
Hi Babu,

On 10/27/2022 8:30 AM, Moger, Babu wrote:
> On 10/26/22 15:23, Reinette Chatre wrote:
>> On 10/26/2022 12:07 PM, Moger, Babu wrote:
>>> On 10/25/22 18:43, Reinette Chatre wrote:
>>>> On 10/17/2022 3:26 PM, Babu Moger wrote:
>>>>
>>>> ...
>>>>
>>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
>>>>>  
>>>>>  	list_for_each_entry(s, &resctrl_schema_all, list) {
>>>>>  		r = s->res;
>>>>> -		if (r->rid == RDT_RESOURCE_MBA) {
>>>>> +		if (r->rid == RDT_RESOURCE_MBA ||
>>>>> +		    r->rid == RDT_RESOURCE_SMBA) {
>>>>>  			rdtgroup_init_mba(r, rdtgrp->closid);
>>>>>  			if (is_mba_sc(r))
>>>>>  				continue;
>>>> The above hunk and the ones that follow are unexpected.
>>> I am thinking the above check is required, It is updating the
>>> staged_config with default values. Right now, the default value for SMBA
>>> is same as MBA default value. So, I used this code to initialize.
>>>
>>> Did I miss something?
>> As I described in the following comments my concern is related to all the
>> software controller code still executing for SMBA. Yes, in the above hunk
>> SMBA would need (some of) rdtgroup_init_mba() ... but note that it contains
>> software controller checks and in the above hunk its call is also followed
>> by another software controller check.
>>
>> The software controller is just applicable to MBA and these checks have been
>> isolated to the MBA resource. Using it for SMBA that does not support
>> software controller at all is making the code harder to follow and sets this
>> code up for future mistakes. I think it would make the code easier to understand
>> if this is made very clear that software controller is not applicable to SMBA at
>> all instead of repurposing these flows.
> 
> Yes. Understood.  How about this? I feel this is much more cleaner.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index e5a48f05e787..d91a6a513681 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup
> *rdtgrp)
>  
>         list_for_each_entry(s, &resctrl_schema_all, list) {
>                 r = s->res;
> -               if (r->rid == RDT_RESOURCE_MBA) {
> +               if (r->rid == RDT_RESOURCE_MBA ||
> +                   r->rid == RDT_RESOURCE_SMBA) {
>                         rdtgroup_init_mba(r, rdtgrp->closid);
> -                       if (is_mba_sc(r))
> -                               continue;
>                 } else {
>                         ret = rdtgroup_init_cat(s, rdtgrp->closid);
>                         if (ret < 0)
>                                 return ret;
>                 }
>  
> +               if (is_mba_sc(r))
> +                       continue;
> +
>                 ret = resctrl_arch_update_domains(r, rdtgrp->closid);
>                 if (ret < 0) {
>                         rdt_last_cmd_puts("Failed to initialize
> allocations\n");
> 

I do not see how that move changes what is run in the SMBA case and it ignores the
is_mba_sc() call within rdtgroup_init_mba(). How about making is_mba_sc() more
robust in support of your original snippet?

Something like:

bool is_mba_sc(struct rdt_resource *r)
{
	if (!r)
		return rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;

	if (r->rid != RDT_RESOURCE_MBA)
		return false;

	return r->membw.mba_sc;
}

Reinette
Moger, Babu Oct. 28, 2022, 3:16 p.m. UTC | #6 
[AMD Official Use Only - General]

Hi Reinette,

> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Thursday, October 27, 2022 1:38 PM
> To: Moger, Babu <Babu.Moger@amd.com>; 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
> <Sandipan.Das@amd.com>; 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 v7 05/12] x86/resctrl: Detect and configure Slow Memory
> Bandwidth allocation
> 
> Hi Babu,
> 
> On 10/27/2022 8:30 AM, Moger, Babu wrote:
> > On 10/26/22 15:23, Reinette Chatre wrote:
> >> On 10/26/2022 12:07 PM, Moger, Babu wrote:
> >>> On 10/25/22 18:43, Reinette Chatre wrote:
> >>>> On 10/17/2022 3:26 PM, Babu Moger wrote:
> >>>>
> >>>> ...
> >>>>
> >>>>> @@ -2845,7 +2846,8 @@ static int rdtgroup_init_alloc(struct
> >>>>> rdtgroup *rdtgrp)
> >>>>>
> >>>>>  	list_for_each_entry(s, &resctrl_schema_all, list) {
> >>>>>  		r = s->res;
> >>>>> -		if (r->rid == RDT_RESOURCE_MBA) {
> >>>>> +		if (r->rid == RDT_RESOURCE_MBA ||
> >>>>> +		    r->rid == RDT_RESOURCE_SMBA) {
> >>>>>  			rdtgroup_init_mba(r, rdtgrp->closid);
> >>>>>  			if (is_mba_sc(r))
> >>>>>  				continue;
> >>>> The above hunk and the ones that follow are unexpected.
> >>> I am thinking the above check is required, It is updating the
> >>> staged_config with default values. Right now, the default value for
> >>> SMBA is same as MBA default value. So, I used this code to initialize.
> >>>
> >>> Did I miss something?
> >> As I described in the following comments my concern is related to all
> >> the software controller code still executing for SMBA. Yes, in the
> >> above hunk SMBA would need (some of) rdtgroup_init_mba() ... but note
> >> that it contains software controller checks and in the above hunk its
> >> call is also followed by another software controller check.
> >>
> >> The software controller is just applicable to MBA and these checks
> >> have been isolated to the MBA resource. Using it for SMBA that does
> >> not support software controller at all is making the code harder to
> >> follow and sets this code up for future mistakes. I think it would
> >> make the code easier to understand if this is made very clear that
> >> software controller is not applicable to SMBA at all instead of repurposing
> these flows.
> >
> > Yes. Understood.  How about this? I feel this is much more cleaner.
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index e5a48f05e787..d91a6a513681 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -2845,16 +2845,18 @@ static int rdtgroup_init_alloc(struct rdtgroup
> > *rdtgrp)
> >
> >         list_for_each_entry(s, &resctrl_schema_all, list) {
> >                 r = s->res;
> > -               if (r->rid == RDT_RESOURCE_MBA) {
> > +               if (r->rid == RDT_RESOURCE_MBA ||
> > +                   r->rid == RDT_RESOURCE_SMBA) {
> >                         rdtgroup_init_mba(r, rdtgrp->closid);
> > -                       if (is_mba_sc(r))
> > -                               continue;
> >                 } else {
> >                         ret = rdtgroup_init_cat(s, rdtgrp->closid);
> >                         if (ret < 0)
> >                                 return ret;
> >                 }
> >
> > +               if (is_mba_sc(r))
> > +                       continue;
> > +
> >                 ret = resctrl_arch_update_domains(r, rdtgrp->closid);
> >                 if (ret < 0) {
> >                         rdt_last_cmd_puts("Failed to initialize
> > allocations\n");
> >
> 
> I do not see how that move changes what is run in the SMBA case and it ignores
> the
> is_mba_sc() call within rdtgroup_init_mba(). How about making is_mba_sc()
> more robust in support of your original snippet?
> 
> Something like:
> 
> bool is_mba_sc(struct rdt_resource *r)
> {
> 	if (!r)
> 		return
> rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl.membw.mba_sc;
> 
> 	if (r->rid != RDT_RESOURCE_MBA)
> 		return false;
> 
> 	return r->membw.mba_sc;
> }

Yes. Sure. That should work.
Thanks
Babu
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index c7561c613209..d79f494a4e91 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -231,9 +231,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;
+
+	/*
+	 * 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, 1, &eax.full, &ebx, &ecx, &edx.full);
+	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;
 
@@ -756,6 +762,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;
@@ -786,6 +805,9 @@  static __init bool get_rdt_alloc_resources(void)
 	if (get_mem_config())
 		ret = true;
 
+	if (get_slow_mem_config())
+		ret = true;
+
 	return ret;
 }
 
@@ -875,6 +897,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;
 		}
 	}
 }
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index 1dafbdc5ac31..42e2ef6fbdb8 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -210,7 +210,7 @@  static int parse_line(char *line, struct resctrl_schema *s,
 	unsigned long dom_id;
 
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP &&
-	    r->rid == RDT_RESOURCE_MBA) {
+	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)) {
 		rdt_last_cmd_puts("Cannot pseudo-lock MBA resource\n");
 		return -EINVAL;
 	}
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 43d9f6f5a931..16e3c6e03c79 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -14,6 +14,7 @@ 
 #define MSR_IA32_L2_CBM_BASE		0xd10
 #define MSR_IA32_MBA_THRTL_BASE		0xd50
 #define MSR_IA32_MBA_BW_BASE		0xc0000200
+#define MSR_IA32_SMBA_BW_BASE		0xc0000280
 
 #define MSR_IA32_QM_CTR			0x0c8e
 #define MSR_IA32_QM_EVTSEL		0x0c8d
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e5a48f05e787..1271fd1ae2f3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1213,7 +1213,7 @@  static bool rdtgroup_mode_test_exclusive(struct rdtgroup *rdtgrp)
 
 	list_for_each_entry(s, &resctrl_schema_all, list) {
 		r = s->res;
-		if (r->rid == RDT_RESOURCE_MBA)
+		if (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA)
 			continue;
 		has_cache = true;
 		list_for_each_entry(d, &r->domains, list) {
@@ -1402,7 +1402,8 @@  static int rdtgroup_size_show(struct kernfs_open_file *of,
 					ctrl = resctrl_arch_get_config(r, d,
 								       closid,
 								       type);
-				if (r->rid == RDT_RESOURCE_MBA)
+				if (r->rid == RDT_RESOURCE_MBA ||
+				    r->rid == RDT_RESOURCE_SMBA)
 					size = ctrl;
 				else
 					size = rdtgroup_cbm_to_size(r, d, ctrl);
@@ -2845,7 +2846,8 @@  static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 
 	list_for_each_entry(s, &resctrl_schema_all, list) {
 		r = s->res;
-		if (r->rid == RDT_RESOURCE_MBA) {
+		if (r->rid == RDT_RESOURCE_MBA ||
+		    r->rid == RDT_RESOURCE_SMBA) {
 			rdtgroup_init_mba(r, rdtgrp->closid);
 			if (is_mba_sc(r))
 				continue;
@@ -3287,7 +3289,8 @@  void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
 {
 	lockdep_assert_held(&rdtgroup_mutex);
 
-	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
+	if (supports_mba_mbps() &&
+	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_SMBA))
 		mba_sc_domain_destroy(r, d);
 
 	if (!r->mon_capable)
@@ -3354,8 +3357,9 @@  int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
 
 	lockdep_assert_held(&rdtgroup_mutex);
 
-	if (supports_mba_mbps() && r->rid == RDT_RESOURCE_MBA)
-		/* RDT_RESOURCE_MBA is never mon_capable */
+	if (supports_mba_mbps() &&
+	    (r->rid == RDT_RESOURCE_MBA || r->rid == RDT_RESOURCE_MBA))
+		/* RDT_RESOURCE_MBA (or SMBA) is never mon_capable */
 		return mba_sc_domain_allocate(r, d);
 
 	if (!r->mon_capable)