Received-SPF: pass (google.com: domain of linux-kernel+bounces-205174-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Message-ID: <8553ddc2-ee0f-447d-8fe4-5a7dd84375f2@intel.com>
Date: Thu, 6 Jun 2024 16:58:34 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] selftests/resctrl: Fix noncont_cat_run_test for AMD
To: <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>
From: Reinette Chatre <reinette.chatre@intel.com>
Content-Language: en-US
In-Reply-To: <3bd755d7-e5c7-3dc3-9875-4884c857e325@amd.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?c2dyVmxwRFd4UDNtM1hJRHltN3NuS1B3ZXluTmxHSC9pQ3pJTmpzbDhHYjhG?=
 =?utf-8?B?eERESWM5WWhEaktzSjExblh2Q3NiWTNaWk5kWWhVeFJMbDMzK1RkUXBMcDlJ?=
 =?utf-8?B?MThUMEl0azNEeWlzVWF1ZFhaQ1liQlFXQjhFdERIWGNNa21pdmJzQmZkeWxT?=
 =?utf-8?B?ekVDc1RQZkVwaHdNMC9iWEljMDdBeFVSUEN4cDd3TWtkbmxac243VXFBNVJC?=
 =?utf-8?B?ZzhRL3JLUXUxcjNjbi9MRStZaFNjNFVlRGlGYXJWcVEzYWVvcTNldXcxdUJ6?=
 =?utf-8?B?TVd2SWczQUhzYXdZZmJEU2lTeHUxNENaenhIWlczTmxJeEVOeDUwU1RERE8z?=
 =?utf-8?B?Z21ZTndVVnMvVFlRYUtNWS9oaHZQc2gvZkp3ekhoeEl5WUd5QUhIWGV5QjAz?=
 =?utf-8?B?Ukh5ZG5YUkNXd2I2S1NCcndseEJLQjk3M3kyaG1EWk1JQWRYdU9HWm9GYUdC?=
 =?utf-8?B?aDlUOXhGdUVDRnNCaHZKU2hQL0IyckxrdWVVTTRFYzZMdngyREVGY01wZGUr?=
 =?utf-8?B?a01pazN6SVVWNGNoSzVJTWM1dGRJWGN3OTdWL3Vnc0w2TE5zTGswblczZll6?=
 =?utf-8?B?L2dPUHNJdDk0d1lDd1FGcGlJbWZ0L0VlWENETlVJS3k1TTJMQjBVUEdrdGxF?=
 =?utf-8?B?aHFobklGdXJ4eDFmeXRTZ3ZOY2txZDNRY0k5NnNDUlBqbG04cFZtWHNodHBk?=
 =?utf-8?B?OVdDWENITG9ZU0lvQTJaKzBPeDZlTHhFakx1WllJbWRaR0o3STNUUXJkcStM?=
 =?utf-8?B?eTNsS2xhSDhmT0hrSHRqdEpxV215Sm1WRzR0N2Y2c2tNTkJzVmFsNmxvZjVP?=
 =?utf-8?B?ek1RcVFoWW1ScEEwWEpXZUhKc0E3N00wemlOYXZibGMzNGxBODBJQWtheE41?=
 =?utf-8?B?MVF2d2c3ei9VbWh0UE85Y3dxa3FFMWpGUkJVY1NwL3lwYmp1QkxsVmpUWHIw?=
 =?utf-8?B?cmRTM0VsZUQ1bjg4c244NUdOOEhkcmc0QWJKRUNZRlJjOTZaU1BHRHB6S0p5?=
 =?utf-8?B?OUJSZzROTXNIV1BNdktrTE9QbEJHeXNqa0JORGZ6QkNSc2dBSkJlbW4zbDJN?=
 =?utf-8?B?dlhVR29rVGVCV2pzQlEzUUNzelIydkN0KzdhRVZiTkVGazFwVGRWM1VvQnhQ?=
 =?utf-8?B?MkdNZVVLWk9XbEVub0toaXhHQnEyRk1FSjhRbC9uYWhXT3FuRmY4aDg1eVBs?=
 =?utf-8?B?dytMY3VOT25sUjRQL1MwOE10L0h2WllIZmI2QXF3SEtGY2V1a1oybloxM1F0?=
 =?utf-8?B?OHdJd01CSkd5R2Iwd1A5M1ppL1RTUnl0NGJEM08yaXFwbG5RaFBtMmt0YjYx?=
 =?utf-8?B?aWZBLzN0dVpnTkdyVXFEQ2VQQjIzQk1xWTdTOHdiWU5Jb2ZkMnVWanZtcG5q?=
 =?utf-8?B?R1dGbjZJTnRCc1B4VGlvd2p0R3g4b0lrU3lpR1ZJQkhNTGhhOFlFaU5wZjlp?=
 =?utf-8?B?bGdLMzRIUHZvTnp2VE4yRFRmYmhlZ3dSekZQcXMrMGxPZzdXZG9lWXpaVTQv?=
 =?utf-8?B?MStnaDZ2WWhIS0x4S1ZsQ0duZ3lvQ2FzRDJxUmlJU2p3TjlEbDVneVluTDNF?=
 =?utf-8?B?QXhmWGE3dEtET1dGZTJLNjhmOUtVbkd3VDJ3Um13YVJ1RVVGTXVDMGJGaGw1?=
 =?utf-8?B?YTNLempUMWp4NlFVcXNlVHVWT0FOKzJGMENUL2pEZjN3MjI2cHN4NTZBMU5V?=
 =?utf-8?B?L3Q1R3NMUDdwUDhJd0tNdDFqVTRpdTF1WGR1MnNLeHRoQldkZ1IvU2N0bm5O?=
 =?utf-8?B?ZGtGb09RcllWNVBXUjVsNmlTcDB3d3lYK1dJbm4vL3dsMXZmNGJRT1h4bW8x?=
 =?utf-8?B?bFVrTCtiNnJmMS9qTkUrU3ZZd1ZtT2hWZGU1NXUzZ1owSmRvVGcrdkFNNSth?=
 =?utf-8?B?N1VUcnVBdkNJTXp4N3E0RXJEcVo3OEVKbTQyeWZyekp6cHU2VUU0Q1BOaGlw?=
 =?utf-8?B?RWRUcDd0R3pTUjZmU0dZYmJjZXUwRDRjakhhaW9Yc2FLUzQyNGJiUURIdm9p?=
 =?utf-8?B?Sy9pZEVyTnVObVArWHB3cVBuUlpva0ltak1JdklmSm1qTTFiNnIyTmxFMUVn?=
 =?utf-8?B?bGlBMmxNUm0yRmx5R243eXp4STBNWHVSU25BZHJnU2VFUDNzUEZscU9DVGpN?=
 =?utf-8?B?bm5YOERmMUNWSXM5dnZ1bkpndmhsYXBiM2tmNGE1amVwOUVJRWZ3a2dkZlVV?=
 =?utf-8?B?UUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b12ccc43-ec4d-493b-2b7b-08dc86849455
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Jun 2024 23:58:36.3241
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: umj8PD7ETOln4OPF/PMQ27fpJgAuTyjVHIJBtmI9pK8ej5W7XsxbtVrMO27ZyYcOqB2CPFR9U3i+aAPgqcMQUAWeGYguiikjRtTZ0fv43fw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB8098
X-OriginatorOrg: intel.com

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

(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.

Reinette