Re: [PATCH 01/15] x86/resctrl: Remove hard-coded memory bandwidth limit

[Moger, Babu]

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 <babu.moger@xxxxxxx>
>>>> 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