Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <13b360ac-4292-5056-6d00-6eb1d4186b11@amd.com>
Date:   Thu, 28 Sep 2023 10:08:56 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.15.1
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v2 1/4] x86/resctrl: Enable non-contiguous bits in Intel
 CAT
Content-Language: en-US
To:     =?UTF-8?Q?Maciej_Wiecz=c3=b3r-Retman?= 
        <maciej.wieczor-retman@intel.com>
Cc:     Fenghua Yu <fenghua.yu@intel.com>,
        Reinette Chatre <reinette.chatre@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, linux-kernel@vger.kernel.org
References: <cover.1695371055.git.maciej.wieczor-retman@intel.com>
 <918e147601697b1d4b8f8589f5751e05d6ceccdf.1695371055.git.maciej.wieczor-retman@intel.com>
 <9751cbaf-8653-d32a-6058-13c251c59b3d@amd.com>
 <4sxjxe4cgvookcobwbrmsoeszjordi3bpjg22zxut3lanjc46j@xpqt6nblx6uc>
From:   "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <4sxjxe4cgvookcobwbrmsoeszjordi3bpjg22zxut3lanjc46j@xpqt6nblx6uc>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?UVVXUFIwa1BFQXNrM2hlMVl3U0NwQi9UNUw0YkliUjdWNmJtVUhwYk1PK2Fj?=
 =?utf-8?B?bWdkYlpLaTZKb1lSM2g3cm1RMGlDeGZhMUh6QUdyL3k3YXIwVWlZWE1NMXpZ?=
 =?utf-8?B?ZDlveGw5dE5oQ2ZOa01QYXFhc1lJMmtISHplTHVRaXAxTEttdFUwamd6OVFz?=
 =?utf-8?B?TnlFcTN0ZGFTYmcwTjVlOVRpeHF5NlNZMzlXblR1NkN5NjRaV0ZUZGVBWmFr?=
 =?utf-8?B?M1BGZ2wveTNiZ0NtQUtaVEczaGtDeEZxaGkraDZvZ0RwVnl0a3RjY0ljOFhB?=
 =?utf-8?B?bE11RGs3cVZyMjVjUDBjbVQvTDFCNlUwV2VTU1JRNUF4VFFwOHpqMlZYcmhM?=
 =?utf-8?B?cWg1T0VaYW1VUzc1VGwydzlxZGNMU0hlbWJUdUtXWDlnNVpLQnA3d0tLdVZq?=
 =?utf-8?B?NU5OVGVYQWdJdklqbW0rWGdDM1FBcEhqNGlJNFNmMlhSOXlXQUFmRXBwNW54?=
 =?utf-8?B?ejFCcWRUNUhENTlseG1jMHhybStvcnRFTDIrdlNQVXJQUXRhd2ZGaFZZaGU1?=
 =?utf-8?B?Vjk3VkVkWTV0bkF5RDdVTmlCS3BES1hGVjdUbUdoRmFaZzFaM2R4VkRBSzJN?=
 =?utf-8?B?c1ZsRGZrRlhDMkpUWVZjU0RRUUFxU3RDRm93VUNGWDN5L1UvQkJESTJTNzdt?=
 =?utf-8?B?ckJVbVFEVWs0TGEzNVpCQlY2a0FpTUE4WW9CWUFmU0NFWkRldFZaZlUxbjRt?=
 =?utf-8?B?NTN4WTJuQk9mZlVxbE5ZL3hnMHVSdUNsMG9HSjVJQmFMNE1lWVpBSGM0M2F3?=
 =?utf-8?B?S2IzNEFGNnlQR3o1R2loQjQ1b2RFQzdoV3pSb0ZQQisweVFlVThhMUZMRi9P?=
 =?utf-8?B?SFF0UmZpODJSTXJPTUdNRUJXQzZ4UUNkOFphNU1xbC9zYWRWQnpBem93OG5m?=
 =?utf-8?B?VDBpclBpMTlsS25JaXcvSVFUdTlFNzVIYS9zN2o5RHdENkVBN0dSSUcwRFF1?=
 =?utf-8?B?WkNZSmo4Q01XaVJuRGxtVWJaeXJyNUtWM0RXTkFHMWVlUzR1ZDRyZWRKUHNv?=
 =?utf-8?B?dmlkUVovQ3ZnRHc5YXlwQVd6LythMTlZcFZLbWxCVG1Hdzk5bzdJSFVrMkNs?=
 =?utf-8?B?aytXUk1TMW5PalBISWZnVTd0cUdFY0dZcFFEWXo1NzYvTHZEZFhLU2ZpY01Z?=
 =?utf-8?B?ZHMzTnRmMmRMcWlXSlh2dW9ZWks1WHE5WlAvc1pQRGI5c0ZZSEd0Szk1Y0gy?=
 =?utf-8?B?dWZKRTlRMGJYU1B5VTA2dlByVHMzUlBlRVQyQkh4WUVBelB4TFZwNzBJSUV6?=
 =?utf-8?B?Y05xdHpKckluRWI5R25MY3dISzlCMXViZXJDajE1NkxZTmNzcjN3Uy9DNWlJ?=
 =?utf-8?B?eFRJMmd1TGdvMjBaTG04RElQRFBLNHR2MC81ZmpDK1pvMTU4ZzNqSkZFUTk2?=
 =?utf-8?B?NjdCTjIzU1Z0U2VUc2p1d1ZDNVZ4N3dVZGxnUHgyZXVyd3NONnYyV09PZ3M5?=
 =?utf-8?B?OWxSZVJqa0lMbnNNSG9GeFFBaGh6WnJFTEF4WFIveHNtN25QRVZFcHJUYzRT?=
 =?utf-8?B?TDJWVUJxK1paQjNmd2wwYlFYLytyZXdtKzRGellncnNDSjhQWVBXbFpNSlpj?=
 =?utf-8?B?UXIwWUVNL2phNGc2TnRIcFZZS1JqZXJCNEx2dE1ReFlDbDV0Z0dDWGh6Mjdy?=
 =?utf-8?B?WE5FZFgwQnhIZWVjQm44N25idW9GKzREQzF6aThIUUpsbnBXaDBzRCs1Y0NG?=
 =?utf-8?B?MXM0MlhxZ0lWQ1phYVdZTGp1d2NiRXExWE9NUVNFbWNuWWYycENKekMvV2g5?=
 =?utf-8?B?ZGhIZ3kxNkY0ZTUrbU5ZMnpuTVdrK1ZsMlMrMDZoSG9KOXpkb3BTQWl2N1Ja?=
 =?utf-8?B?NXBpVjU4dHBjVEhydGZueDFLaUxMZXBIOGxmZDhwaVEwWDZXaTNLU3NhYW9i?=
 =?utf-8?B?K1ZCbWZwM3QyL1pQQVBVaTFWZHF0YU5QdTNxYzBRNGhUenlZRER4Y1E2Zm9a?=
 =?utf-8?B?OEREMVdwcHI1c2JRNHltc2VBMWVIcGdYeGhaL1RQZENGcm0rbmE1RG5EUW1D?=
 =?utf-8?B?S1VmdXNUeFNqM0ZXVG5NK1lQd0pjSlNXb1YzQWlpcGxmOTdLeVoxTFRwb1ov?=
 =?utf-8?B?TisrYi9aUm9kWlZaNTg0dDJSOEpxME5kSWFEeVM3MkFZRTBzR0U5SkZGZG8y?=
 =?utf-8?Q?IJSfQGtHgVVE0ZbV/84CImsnB?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: efa1f5af-803d-459c-b0c8-08dbc034d7a2
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 28 Sep 2023 15:08:59.2944
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: xd7Pb2xxc6bw8iKEVGYgNJAmof9kBjpKTs5q8THRzmN3LB2pt1zbMJ44P8NRrPnI
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR12MB7888
X-Spam-Status: No, score=-2.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (howler.vger.email [0.0.0.0]); Thu, 28 Sep 2023 08:09:13 -0700 (PDT)

Hi Maciej,

On 9/28/23 02:06, Maciej Wieczór-Retman wrote:
> Hi, thanks for reviewing the series,
> 
> On 2023-09-27 at 17:34:27 -0500, Moger, Babu wrote:
>> Hi Maciej,
>>
>> How about this subject line?
>>
>> x86/resctrl: Enable non-contiguous CBMs on Intel CAT
> 
> Changing "bits" to "CBMs" does indeed seem sensible so I'll do that if
> there are no objections.
> 
> But I'm not sure the preposition collocation change from "in" to "on"
> would be grammatical (at least from what I've read in docs about Intel
> CAT so far).
> 
>> On 9/22/2023 3:48 AM, Maciej Wieczor-Retman wrote:
>>> The setting for non-contiguous 1s support in Intel CAT is
>>
>>> hardcoded to false. On these systems, writing non-contiguous
>>> 1s into the schemata file will fail before resctrl passes
>>> the value to the hardware.
>>>
>>> In Intel CAT CPUID.0x10.1:ECX[3] and CPUID.0x10.2:ECX[3] stopped
>>> being reserved and now carry information about non-contiguous 1s
>>> value support for L3 and L2 cache respectively. The CAT
>>> capacity bitmask (CBM) supports a non-contiguous 1s value if
>>> the bit is set.
>>>
>>> Replace the hardcoded non-contiguous support value with
>>> the support learned from the hardware. Add hardcoded non-contiguous
>>> support value to Haswell probe since it can't make use of CPUID for
>>> Cache allocation.
>>>
>>> Originally-by: Fenghua Yu <fenghua.yu@intel.com>
>>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>>> ---
>>> Changelog v2:
>>> - Rewrite part of a comment concerning Haswell. (Reinette)
>>>
>>>   arch/x86/kernel/cpu/resctrl/core.c        |  9 ++++++---
>>>   arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 10 ++++++----
>>>   arch/x86/kernel/cpu/resctrl/internal.h    |  9 +++++++++
>>>   3 files changed, 21 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
>>> index 030d3b409768..c783a873147c 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>>> @@ -152,6 +152,7 @@ static inline void cache_alloc_hsw_probe(void)
>>>   	r->cache.cbm_len = 20;
>>>   	r->cache.shareable_bits = 0xc0000;
>>>   	r->cache.min_cbm_bits = 2;
>>> +	r->cache.arch_has_sparse_bitmaps = false;
>>
>> Is this change required?
>>
>> This is always set to false in rdt_init_res_defs_intel().
> 
> The logic behind moving this variable initialization from
> rdt_init_res_defs_intel() into both cache_alloc_hsw_probe() and
> rdt_get_cache_alloc_cfg() is that the variable doesn't really have a
> default value anymore. It used to when the CPUID.0x10.1:ECX[3] and
> CPUID.0x10.2:ECX[3] bits were reserved.
> 
> Now for the general case the variable is dependent on CPUID output.
> And only for Haswell case it needs to be hardcoded to "false", so the
> assignment makes more sense in Haswell probe rather than in the default
> section.

Here is the current sequence order with your change.

1.
resctrl_late_init -> check_quirks -> __check_quirks_intel ->
cache_alloc_hsw_probe
   r->cache.arch_has_sparse_bitmaps = false; (new code)

2. resctrl_late_init -> rdt_init_res_defs -> rdt_init_res_defs_intel
   r->cache.arch_has_sparse_bitmaps = false; (old code)

3. resctrl_late_init -> get_rdt_resources -> get_rdt_alloc_resources ->
rdt_get_cache_alloc_cfg
   r->cache.arch_has_sparse_bitmaps = ecx.split.noncont; (new code)

The code in (3) is going to overwrite whatever is set in (1) or (2).

I would say you can just remove initialization in both (1) and (2). That
makes the code clearer to me. I assume reserved bits in Intel is always 0.

Thanks
Babu


> 
>>>   	r->alloc_capable = true;
>>>   	rdt_alloc_capable = true;
>>> @@ -267,15 +268,18 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
>>>   {
>>>   	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>>>   	union cpuid_0x10_1_eax eax;
>>> +	union cpuid_0x10_x_ecx ecx;
>>>   	union cpuid_0x10_x_edx edx;
>>> -	u32 ebx, ecx;
>>> +	u32 ebx;
>>> -	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx, &edx.full);
>>> +	cpuid_count(0x00000010, idx, &eax.full, &ebx, &ecx.full, &edx.full);
>>>   	hw_res->num_closid = edx.split.cos_max + 1;
>>>   	r->cache.cbm_len = eax.split.cbm_len + 1;
>>>   	r->default_ctrl = BIT_MASK(eax.split.cbm_len + 1) - 1;
>>>   	r->cache.shareable_bits = ebx & r->default_ctrl;
>>>   	r->data_width = (r->cache.cbm_len + 3) / 4;
>>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL)
>>> +		r->cache.arch_has_sparse_bitmaps = ecx.split.noncont;
>>>   	r->alloc_capable = true;
>>>   }
>>> @@ -872,7 +876,6 @@ static __init void rdt_init_res_defs_intel(void)
>>>   		if (r->rid == RDT_RESOURCE_L3 ||
>>>   		    r->rid == RDT_RESOURCE_L2) {
>>> -			r->cache.arch_has_sparse_bitmaps = false;
>>
>> Why do you have to remove this one here?   This seems like a right place to
>> initialize.
> 
> Look at the previous comment.
> 

-- 
Thanks
Babu Moger