Received-SPF: pass (google.com: domain of linux-kernel+bounces-206591-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <edc31538-3aa1-a35a-ea67-b13f626a84ec@amd.com>
Date: Fri, 7 Jun 2024 13:16:22 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH] selftests/resctrl: Fix noncont_cat_run_test for AMD
Content-Language: en-US
To: Reinette Chatre <reinette.chatre@intel.com>, babu.moger@amd.com,
 fenghua.yu@intel.com, shuah@kernel.org
Cc: linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
 ilpo.jarvinen@linux.intel.com, maciej.wieczor-retman@intel.com,
 peternewman@google.com, eranian@google.com
References: <3a6c9dd9dc6bda6e2582db049bfe853cd836139f.1717622080.git.babu.moger@amd.com>
 <9a7213d2-a782-4342-93f2-acf191c745fd@intel.com>
 <3bd755d7-e5c7-3dc3-9875-4884c857e325@amd.com>
 <8553ddc2-ee0f-447d-8fe4-5a7dd84375f2@intel.com>
From: "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <8553ddc2-ee0f-447d-8fe4-5a7dd84375f2@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?d3lqdElwM0pYSlJMTFJPRit5WlhqNzNHVlltUUs5ckJ6NDZJcHpvTklmWUQ1?=
 =?utf-8?B?cStzN1Z1UTdiRDBiZmdVN3l3emt3WnJuSnpXOStUS2Jya1lxTitrOTVzaE5V?=
 =?utf-8?B?eEtRbFNGSG9FcVNoRmlCMDlQSm1YeFUrelBkMUlFR0MyOTNVcGplczJFenNo?=
 =?utf-8?B?NzBJUk5QVGM1UjdrcjVVM0VKM05RTVZEcEJXUGJ5Tis3VW9tRUU2cWg5dlRq?=
 =?utf-8?B?WHg0dHZLRm80eG1kNGt6ZURydlVZRDdXNGV6Skh3LzBKYnh0a3l5QVVDbk5W?=
 =?utf-8?B?L25MOXNTejdhb2pqeE05VW9CeTg5MTE5NGg1K09TOFM4TE0xUHprWlZST3cz?=
 =?utf-8?B?S1ZKNXRKcE5lR2FyWjJFUG1NSjMzOFJwblpWQWZ5OXBHMWFWR3RaYU5UejY4?=
 =?utf-8?B?eHlXWGZjRW1SaXUxbVo1VXZDb2dKQnJpemhzTWs5ZXd5alJuMElGOE1sTXVD?=
 =?utf-8?B?cUR6MFZnUG1HcTNaQzhQelh2cEUwWVM2aWJSYnZ6QzkwdXB6VmxJYTdkSjgr?=
 =?utf-8?B?L05hS0dqRHFpUTFsMUNQK05Jd2Vac3dicGROV3I3NElNRXpMSjNqTHRkcHpN?=
 =?utf-8?B?WDluM1NMYy9HZTdsTHVmV3Z3NGQ2eUZiMXRwckpvUTJmclNWeTh2dSs4Wm9j?=
 =?utf-8?B?R3JPckM2UUhQWTNEYmNkWUFUTUVQTy8yZ1F0blhCL2JiT1BrU0hlTTh0eHR0?=
 =?utf-8?B?RmlpbWZXVTlmSjZpYUlXdXpOQ1NXd0xpZ1NwRlhuUVdKWXBCQkZGbTR5SXdR?=
 =?utf-8?B?a1NuT0lxZFhiZEhtVUQxOHNYM2IxL2ljbTNZNDZzMHhqVUJSMmhDdkxwdFVC?=
 =?utf-8?B?elpnQXMzN3ZkSmZTbzREbXdCT28vQUxsMk9HdllZcFo3M3k4VzZ6ODJGdnRp?=
 =?utf-8?B?K1R6SGoxS3V3NnBub09Dd2FmUzJiL0FLWTBtNHMwNkFJV0NoV2NscnFwem9V?=
 =?utf-8?B?K2RlVmNxRWJrSEZpYkhKV3VxZ1dqMFlpK3JlZnUrRXhtV1J3bjA4UktXbkdQ?=
 =?utf-8?B?N0plSzBJbllRQmlJazJhcWN6U0FjY2IxOCtwQWgzZTNpOG95cjRKWGtiamJx?=
 =?utf-8?B?U3gvOFpOdmJZWnVwekpuWGRyMmd0b3RDUVEvSHJXNWdPVExXdzNqSTNoUW8w?=
 =?utf-8?B?TnBjUmY2NzVMSzhLcDBlV1lRZXpXWGN1Sk05SGdFWlN2Z25BUE9mZU5ibVd5?=
 =?utf-8?B?OFZYQ1F0d29qanVDL0txdDIzcDl5MFcvRDdkaVFTbmJPaWhFSjdsd1pISVZy?=
 =?utf-8?B?RHJ6eXNyOFdjSjUrM3FBdEVJT1dDK3d5WWVVVWRGYTd3ckhFRFQzSHZpTVVj?=
 =?utf-8?B?aWxvRHVqYS9Ham1NZDIrVThFYklabWxnK2pJc0kxOXZiRDZyaGNHcGFTUnRr?=
 =?utf-8?B?REFGOVQxeW82Q1FPYkZzWU4vUnZraEZ6NHNaK29jdkJtdU1JbmkwTkRwK0tR?=
 =?utf-8?B?ZmNzcCs0VUJoNiszdmg5bXA3K3M0NkdqL2RTaUdqRkk5Z3dMNW1JdTFVNWVR?=
 =?utf-8?B?VGxQd0FQUGpRL05JMHZGYzBtSTdrUWphS0QxaTczeVBwYXRWTVYyRzBaRTEv?=
 =?utf-8?B?NVIxcWo1TmhuU1hzNk1MK05lR2JENlFQRGxmemNwT0QrZzhBNU1oZkhjOGpC?=
 =?utf-8?B?cmF1NXl6b2h4ZEwvalYrS2t0Ymo4c2ZobG55VksxMDczVUxMNDlDTUJUVHhR?=
 =?utf-8?B?cFl2UEcvL1lzTXA1RGw0ZlFRbVBJNklrOGlTTmNGdnl4a0FTRHUwTlZKckF4?=
 =?utf-8?B?ODhTV3NJaXZzeGUxZTVIWUxMcFRpK0NYUXhDR2pDWVlGWGt1RllJdURpbWNk?=
 =?utf-8?B?bkE5OTkwOFQxcnpXaXdUcFQ5TmFSanB4YmhkWlNXd2hJc0d5Ny8yZkQyemtu?=
 =?utf-8?B?N3FrZW0xNUpXRVpQakI5bUFjb09JbytVeHlsbGhUS2FSbHNlRnYrOHE1anpt?=
 =?utf-8?B?L0pUN3pzbFhPZkp2WHU3UEsxc2VKaGFuMUhPN1BrdW4ydUl4bDZpSENnWElV?=
 =?utf-8?B?TW1tMXU3WkV1S3RXVlRoRDJldENSOUxqakVlUDNjT0VWUHBTaC9paStIc2FO?=
 =?utf-8?B?VDdPWXVqQ0lRS0dJWUFOQkRGOEtpREU5aHlqZjVOVGI1c2RnejZqVGhwSzlI?=
 =?utf-8?Q?Ljqw=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4098a2e6-ced5-4bc9-c9e7-08dc871df11f
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Jun 2024 18:16:25.0189
 (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: xF9WYZdSCXvHDalecCfJskXa3MpyG841W3YiRg/nCgOKl/XZzkYQpcXd4F7RJvjN
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6530

Hi Reinette,

On 6/6/2024 6:58 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/6/24 4:09 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>>
>> On 6/6/2024 3:33 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 6/5/24 2:36 PM, Babu Moger wrote:
>>>> The selftest noncont_cat_run_test fails on AMD with the warnings. 
>>>> Reason
>>>> is, AMD supports non contiguous CBM masks but does not report it via 
>>>> CPUID.
>>>>
>>>> Update noncont_cat_run_test to check for the vendor when verifying 
>>>> CPUID.
>>>>
>>>> Fixes: ae638551ab64 ("selftests/resctrl: Add non-contiguous CBMs CAT 
>>>> test")
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>>> This was part of the series
>>>> https://lore.kernel.org/lkml/cover.1708637563.git.babu.moger@amd.com/
>>>> Sending this as a separate fix per review comments.
>>>> ---
>>>>   tools/testing/selftests/resctrl/cat_test.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c 
>>>> b/tools/testing/selftests/resctrl/cat_test.c
>>>> index d4dffc934bc3..b2988888786e 100644
>>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>>> @@ -308,7 +308,7 @@ static int noncont_cat_run_test(const struct 
>>>> resctrl_test *test,
>>>>       else
>>>>           return -EINVAL;
>>>> -    if (sparse_masks != ((ecx >> 3) & 1)) {
>>>> +    if ((get_vendor() == ARCH_INTEL) && sparse_masks != ((ecx >> 3) 
>>>> & 1)) {
>>>>           ksft_print_msg("CPUID output doesn't match 'sparse_masks' 
>>>> file content!\n");
>>>>           return 1;
>>>>       }
>>>
>>> Since AMD does not report this support via CPUID it does not seem
>>> appropriate to use CPUID at all on AMD when doing the hardware check.
>>> I think the above check makes it difficult to understand what is 
>>> different
>>> on AMD.
>>>
>>> What if instead there is a new function, for example,
>>> "static bool arch_supports_noncont_cat(const struct resctrl_test *test)"
>>> that returns true if the hardware supports non-contiguous CBM?
>>
>> Sure.
>>
>>>
>>> The vendor check can be in there to make it obvious what is going on:
>>>
>>>      /* AMD always supports non-contiguous CBM. */
>>>      if (get_vendor() == AMD)
>>>          return true;
>>>
>>>      /* CPUID check for Intel here. */
>>>
>>> The "sparse_masks" from kernel can then be checked against
>>> hardware support with an appropriate (no mention of CPUID)
>>> error message if this fails.
>>>
>>
>> Something like this?
>>
>>
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c 
>> b/tools/testing/selftests/resctrl/cat_test.c
>> index d4dffc934bc3..b75d220f29f6 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -288,11 +288,30 @@ static int cat_run_test(const struct 
>> resctrl_test *test, const struct user_param
>>          return ret;
>>   }
>>
>> +static bool arch_supports_noncont_cat(const struct resctrl_test *test)
>> +{
>> +       unsigned int eax, ebx, ecx, edx;
>> +
>> +       /* AMD always supports non-contiguous CBM. */
>> +       if (get_vendor() == ARCH_AMD) {
>> +               return true;
>> +       } else {
> 
> The else can be dropped since it follows a return.
Sure

> The rest of the code can be prefixed with a matching
> comment like:
>      /* Intel support for non-contiguous CBM needs to be discovered. */

Sure
> 
> (please feel free to improve)
> 
>> +               if (!strcmp(test->resource, "L3"))
>> +                       __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>> +               else if (!strcmp(test->resource, "L2"))
>> +                       __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>> +               else
>> +                       return false;
>> +
>> +               return ((ecx >> 3) & 1);
>> +       }
>> +}
>> +
>>   static int noncont_cat_run_test(const struct resctrl_test *test,
>>                                  const struct user_params *uparams)
>>   {
>>          unsigned long full_cache_mask, cont_mask, noncont_mask;
>> -       unsigned int eax, ebx, ecx, edx, sparse_masks;
>> +       unsigned int sparse_masks;
>>          int bit_center, ret;
>>          char schemata[64];
>>
>> @@ -301,15 +320,8 @@ static int noncont_cat_run_test(const struct 
>> resctrl_test *test,
>>          if (ret)
>>                  return ret;
>>
>> -       if (!strcmp(test->resource, "L3"))
>> -               __cpuid_count(0x10, 1, eax, ebx, ecx, edx);
>> -       else if (!strcmp(test->resource, "L2"))
>> -               __cpuid_count(0x10, 2, eax, ebx, ecx, edx);
>> -       else
>> -               return -EINVAL;
>> -
>> -       if (sparse_masks != ((ecx >> 3) & 1)) {
>> -               ksft_print_msg("CPUID output doesn't match 
>> 'sparse_masks' file content!\n");
>> +       if (!(arch_supports_noncont_cat(test) && sparse_masks)) {
>> +               ksft_print_msg("Hardware does not support 
>> non-contiguous CBM!\n");
> 
> Please fix the test as well as the message. It is not an error if 
> hardware does
> not support non-contiguous CBM. It is an error if the hardware and 
> kernel disagrees whether
> non-contiguous CBM is supported.

Not sure about this comment.

Did you mean?

  if (!arch_supports_noncont_cat(test)) {
                 ksft_print_msg("Hardware does not support 
non-contiguous CBM!\n");
                 return 0;
          } else if (arch_supports_noncont_cat(test) && !sparse_masks)) {
                 ksft_print_msg("Hardware and kernel support for 
non-contiguous CBM does not match!\n");
                 return 1;
         }


-- 
- Babu Moger