The QOS Memory Bandwidth Enforcement Limit is reported by
CPUID_Fn80000020_EAX_x01.
Bits Description
31:0 BW_LEN: Size of the QOS Memory Bandwidth Enforcement Limit.
Remove the hardcoded bandwidth limit value and detect using CPUID command.
The CPUID details are documentation in the PPR listed below [1].
[1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
11h B1 - 55901 Rev 0.25.
Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
Signed-off-by: Babu Moger <[email protected]>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
---
arch/x86/kernel/cpu/resctrl/core.c | 2 +-
arch/x86/kernel/cpu/resctrl/internal.h | 1 -
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 19e0681f0435..3fbae10b662d 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -243,7 +243,7 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
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;
+ r->default_ctrl = 1 << eax.full;
/* AMD does not use delay */
r->membw.delay_linear = false;
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index a4f1aa15f0a2..d2979748fae4 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -18,7 +18,6 @@
#define MBM_OVERFLOW_INTERVAL 1000
#define MAX_MBA_BW 100u
#define MBA_IS_LINEAR 0x4
-#define MAX_MBA_BW_AMD 0x800
#define MBM_CNTR_WIDTH_OFFSET_AMD 20
#define RMID_VAL_ERROR BIT_ULL(63)
--
2.34.1
Hi Babu,
On 11/30/2023 4:57 PM, Babu Moger wrote:
> The QOS Memory Bandwidth Enforcement Limit is reported by
> CPUID_Fn80000020_EAX_x01.
> Bits Description
> 31:0 BW_LEN: Size of the QOS Memory Bandwidth Enforcement Limit.
>
> Remove the hardcoded bandwidth limit value and detect using CPUID command.
>
> The CPUID details are documentation in the PPR listed below [1].
> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
> 11h B1 - 55901 Rev 0.25.
>
> Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
What makes this change worthy of the "Fixes:" tag. What is the impact
of this? Does this mean that there is AMD hardware out there that is
not being used correctly? In this case it should be highlighted and
the stable folks included?
This also does not seem like it belongs in this series and should
be sent separately as a fix.
> Signed-off-by: Babu Moger <[email protected]>
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 2 +-
> arch/x86/kernel/cpu/resctrl/internal.h | 1 -
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 19e0681f0435..3fbae10b662d 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -243,7 +243,7 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>
> 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;
> + r->default_ctrl = 1 << eax.full;
This does not seem appropriate. You are using eax because it
it convenient but if you take a look at its definition it does not
match the AMD CPUID instruction output at all.
>
> /* AMD does not use delay */
> r->membw.delay_linear = false;
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a4f1aa15f0a2..d2979748fae4 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -18,7 +18,6 @@
> #define MBM_OVERFLOW_INTERVAL 1000
> #define MAX_MBA_BW 100u
> #define MBA_IS_LINEAR 0x4
> -#define MAX_MBA_BW_AMD 0x800
> #define MBM_CNTR_WIDTH_OFFSET_AMD 20
>
> #define RMID_VAL_ERROR BIT_ULL(63)
Reinette
Hi Reinette,
On 12/5/23 17:18, Reinette Chatre wrote:
> Hi Babu,
>
> On 11/30/2023 4:57 PM, Babu Moger wrote:
>> The QOS Memory Bandwidth Enforcement Limit is reported by
>> CPUID_Fn80000020_EAX_x01.
>> Bits Description
>> 31:0 BW_LEN: Size of the QOS Memory Bandwidth Enforcement Limit.
>>
>> Remove the hardcoded bandwidth limit value and detect using CPUID command.
>>
>> The CPUID details are documentation in the PPR listed below [1].
>> [1] Processor Programming Reference (PPR) Vol 1.1 for AMD Family 19h Model
>> 11h B1 - 55901 Rev 0.25.
>>
>> Fixes: 4d05bf71f157 ("x86/resctrl: Introduce AMD QOS feature")
>
> What makes this change worthy of the "Fixes:" tag. What is the impact
> of this? Does this mean that there is AMD hardware out there that is
> not being used correctly? In this case it should be highlighted and
> the stable folks included?
This was reported during the internal testing on upcoming hardware which
supports higher bandwidth limit. It could be a problem with new hardware
and old kernel.
>
> This also does not seem like it belongs in this series and should
> be sent separately as a fix.
Agree. Will send as separate patch.
>
>> Signed-off-by: Babu Moger <[email protected]>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> arch/x86/kernel/cpu/resctrl/core.c | 2 +-
>> arch/x86/kernel/cpu/resctrl/internal.h | 1 -
>> 2 files changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>> index 19e0681f0435..3fbae10b662d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -243,7 +243,7 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>
>> 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;
>> + r->default_ctrl = 1 << eax.full;
>
> This does not seem appropriate. You are using eax because it
> it convenient but if you take a look at its definition it does not
> match the AMD CPUID instruction output at all.
Not sure where you see it. Here it is.
https://bugzilla.kernel.org/attachment.cgi?id=303986
Here is the definition.
CPUID_Fn80000020_EAX_x01 [Platform QoS Enforcement for Memory Bandwidth]
(Core::X86::Cpuid::PqeBandwidthEax1)
Read-only. Reset: 0000_000Bh.
_ccd[11:0]_lthree0_core[7:0]_thread[1:0]; CPUID_Fn80000020_EAX_x01
Bits Description
31:0 BW_LEN: QOS Memory Bandwidth Enforcement Limit Size. Read-only.
Reset: 0000_000Bh. Size of the QOS Memory Bandwidth Enforcement Limit.
In this case, limit size is 12 (0BH) bits. Max limit is 1 << 12.
>
>>
>> /* AMD does not use delay */
>> r->membw.delay_linear = false;
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index a4f1aa15f0a2..d2979748fae4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -18,7 +18,6 @@
>> #define MBM_OVERFLOW_INTERVAL 1000
>> #define MAX_MBA_BW 100u
>> #define MBA_IS_LINEAR 0x4
>> -#define MAX_MBA_BW_AMD 0x800
>> #define MBM_CNTR_WIDTH_OFFSET_AMD 20
>>
>> #define RMID_VAL_ERROR BIT_ULL(63)
>
> Reinette
--
Thanks
Babu Moger
Hi Babu,
On 12/6/2023 8:29 AM, Moger, Babu wrote:
> On 12/5/23 17:18, Reinette Chatre wrote:
>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>>> Signed-off-by: Babu Moger <[email protected]>
>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>> ---
>>> arch/x86/kernel/cpu/resctrl/core.c | 2 +-
>>> arch/x86/kernel/cpu/resctrl/internal.h | 1 -
>>> 2 files changed, 1 insertion(+), 2 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index 19e0681f0435..3fbae10b662d 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -243,7 +243,7 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>>
>>> 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;
>>> + r->default_ctrl = 1 << eax.full;
>>
>> This does not seem appropriate. You are using eax because it
>> it convenient but if you take a look at its definition it does not
>> match the AMD CPUID instruction output at all.
>
> Not sure where you see it. Here it is.
> https://bugzilla.kernel.org/attachment.cgi?id=303986
>
> Here is the definition.
>
> CPUID_Fn80000020_EAX_x01 [Platform QoS Enforcement for Memory Bandwidth]
> (Core::X86::Cpuid::PqeBandwidthEax1)
> Read-only. Reset: 0000_000Bh.
> _ccd[11:0]_lthree0_core[7:0]_thread[1:0]; CPUID_Fn80000020_EAX_x01
> Bits Description
> 31:0 BW_LEN: QOS Memory Bandwidth Enforcement Limit Size. Read-only.
> Reset: 0000_000Bh. Size of the QOS Memory Bandwidth Enforcement Limit.
>
> In this case, limit size is 12 (0BH) bits. Max limit is 1 << 12.
>
I see it in the definition of the data type you are using. Specifically
it is:
/* CPUID.(EAX=10H, ECX=ResID=3).EAX */
union cpuid_0x10_3_eax {
struct {
unsigned int max_delay:12;
} split;
unsigned int full;
};
How the kernel interprets the register does not match with what you paste
from the spec. This is an AMD specific function, __rdt_get_mem_config_amd().
Tt does not seem appropriate to use the register definition of Intel
systems if the Intel and AMD registers do not have the same format.
Reinette
Hi Reinetee,
On 12/6/23 11:09, Reinette Chatre wrote:
> Hi Babu,
>
> On 12/6/2023 8:29 AM, Moger, Babu wrote:
>> On 12/5/23 17:18, Reinette Chatre wrote:
>>> On 11/30/2023 4:57 PM, Babu Moger wrote:
>
>>>> Signed-off-by: Babu Moger <[email protected]>
>>>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>>>> ---
>>>> arch/x86/kernel/cpu/resctrl/core.c | 2 +-
>>>> arch/x86/kernel/cpu/resctrl/internal.h | 1 -
>>>> 2 files changed, 1 insertion(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>>> index 19e0681f0435..3fbae10b662d 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>>> @@ -243,7 +243,7 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r)
>>>>
>>>> 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;
>>>> + r->default_ctrl = 1 << eax.full;
>>>
>>> This does not seem appropriate. You are using eax because it
>>> it convenient but if you take a look at its definition it does not
>>> match the AMD CPUID instruction output at all.
>>
>> Not sure where you see it. Here it is.
>> https://bugzilla.kernel.org/attachment.cgi?id=303986
>>
>> Here is the definition.
>>
>> CPUID_Fn80000020_EAX_x01 [Platform QoS Enforcement for Memory Bandwidth]
>> (Core::X86::Cpuid::PqeBandwidthEax1)
>> Read-only. Reset: 0000_000Bh.
>> _ccd[11:0]_lthree0_core[7:0]_thread[1:0]; CPUID_Fn80000020_EAX_x01
>> Bits Description
>> 31:0 BW_LEN: QOS Memory Bandwidth Enforcement Limit Size. Read-only.
>> Reset: 0000_000Bh. Size of the QOS Memory Bandwidth Enforcement Limit.
>>
>> In this case, limit size is 12 (0BH) bits. Max limit is 1 << 12.
>>
>
> I see it in the definition of the data type you are using. Specifically
> it is:
>
> /* CPUID.(EAX=10H, ECX=ResID=3).EAX */
> union cpuid_0x10_3_eax {
> struct {
> unsigned int max_delay:12;
> } split;
> unsigned int full;
> };
>
> How the kernel interprets the register does not match with what you paste
> from the spec. This is an AMD specific function, __rdt_get_mem_config_amd().
> Tt does not seem appropriate to use the register definition of Intel
> systems if the Intel and AMD registers do not have the same format.
> Yes. You are right. Our current code has the problem already.
union cpuid_0x10_3_eax eax;
union cpuid_0x10_x_edx edx;
u32 ebx, ecx, subleaf;
Will fix both. Just "u32 eax, edx" should be fine for AMD.
Thanks for pointing.
--
Thanks
Babu Moger