linux-kernel - Re: [PATCH v2 1/3] selftests/resctrl: Add CPU vendor detection for Hygon

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f2e30c0b-018f-4988-a1ad-9ead6af7994d@open-hieco.net>
Date: Mon, 8 Dec 2025 16:01:05 +0800
From: Xiaochen Shen <shenxiaochen@...n-hieco.net>
To: Fenghua Yu <fenghuay@...dia.com>, tony.luck@...el.com,
 reinette.chatre@...el.com, bp@...en8.de, shuah@...nel.org,
 skhan@...uxfoundation.org
Cc: babu.moger@....com, james.morse@....com, Dave.Martin@....com,
 x86@...nel.org, linux-kernel@...r.kernel.org,
 linux-kselftest@...r.kernel.org, shenxiaochen@...n-hieco.net
Subject: Re: [PATCH v2 1/3] selftests/resctrl: Add CPU vendor detection for
 Hygon

Hi Fenghua,

On 12/6/2025 3:28 AM, Fenghua Yu wrote:
>> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
>> @@ -42,6 +42,8 @@ static int detect_vendor(void)
>>           vendor_id = ARCH_INTEL;
>>       else if (s && !strcmp(s, ": AuthenticAMD\n"))
>>           vendor_id = ARCH_AMD;
>> +    else if (s && !strcmp(s, ": HygonGenuine\n"))
>> +        vendor_id = ARCH_HYGON;
>>   
> Since vendor_id is bitmask now and BIT() is a UL value, it's better to define it as "unsigned int" (unsigned long is a bit overkill). Otherwise, type conversion may be risky.

Thank you for the suggestion. How about using BIT_U8() instead of BIT()?
In my opinion, 8-bits type "unsigned int" is enough for "vendor id".

> 
> Is it better to change vendor_id as "unsigned int", static unsigned int detect_vendor(), and a couple of other places?

Yes. It is better to update the return types of detect_vendor() and get_vendor() from 'int' to 'unsigned int'
to align with their usage as bitmask values and to prevent potentially risky type conversions.

Should I split the code changes (using BIT_xx(), updates of type 'unsigned int') into a separate patch?	

The patch may look like:
-----------------------------
commit baaabb7bd3a3e45a8093422b576383da20488aca
Author: Xiaochen Shen <shenxiaochen@...n-hieco.net>
Date:   Mon Dec 8 14:26:45 2025 +0800

    selftests/resctrl: Improve type definitions of CPU vendor IDs

    In file resctrl.h:
    -----------------
      /*
       * CPU vendor IDs
       *
       * Define as bits because they're used for vendor_specific bitmask in
       * the struct resctrl_test.
       */
      #define ARCH_INTEL     1
      #define ARCH_AMD       2
    -----------------

    The comment before the CPU vendor IDs defines attempts to provide
    guidance but it is clearly still quite subtle that these values are
    required to be unique bits. Consider for example their usage in
    test_vendor_specific_check():
            return get_vendor() & test->vendor_specific

    A clearer and more maintainable approach is to define these CPU vendor
    IDs using BIT(). This ensures each vendor corresponds to a distinct bit
    and makes it obvious when adding new vendor IDs.

    Additionally, update the return types of detect_vendor() and
    get_vendor() from 'int' to 'unsigned int' to align with their usage as
    bitmask values and to prevent potentially risky type conversions.

    Suggested-by: Reinette Chatre <reinette.chatre@...el.com>
    Suggested-by: Fenghua Yu <fenghuay@...dia.com>
    Signed-off-by: Xiaochen Shen <shenxiaochen@...n-hieco.net>

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index cd3adfc14969..2922dfbf9090 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -23,6 +23,7 @@
 #include <asm/unistd.h>
 #include <linux/perf_event.h>
 #include <linux/compiler.h>
+#include <linux/bits.h>
 #include "../kselftest.h"

 #define MB                     (1024 * 1024)
@@ -36,8 +37,8 @@
  * Define as bits because they're used for vendor_specific bitmask in
  * the struct resctrl_test.
  */
-#define ARCH_INTEL     1
-#define ARCH_AMD       2
+#define ARCH_INTEL     BIT_U8(0)
+#define ARCH_AMD       BIT_U8(1)

 #define END_OF_TESTS   1

@@ -163,7 +164,7 @@ extern int snc_unreliable;
 extern char llc_occup_path[1024];

 int snc_nodes_per_l3_cache(void);
-int get_vendor(void);
+unsigned int get_vendor(void);
 bool check_resctrlfs_support(void);
 int filter_dmesg(void);
 int get_domain_id(const char *resource, int cpu_no, int *domain_id);
diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index 5154ffd821c4..0fef2e4171e7 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -23,10 +23,10 @@ static struct resctrl_test *resctrl_tests[] = {
        &l2_noncont_cat_test,
 };

-static int detect_vendor(void)
+static unsigned int detect_vendor(void)
 {
        FILE *inf = fopen("/proc/cpuinfo", "r");
-       int vendor_id = 0;
+       unsigned int vendor_id = 0;
        char *s = NULL;
        char *res;

@@ -48,12 +48,14 @@ static int detect_vendor(void)
        return vendor_id;
 }

-int get_vendor(void)
+unsigned int get_vendor(void)
 {
-       static int vendor = -1;
+       static unsigned int vendor;

-       if (vendor == -1)
+       if (vendor == 0)
                vendor = detect_vendor();
+
+       /* detect_vendor() returns invalid vendor id */
        if (vendor == 0)
                ksft_print_msg("Can not get vendor info...\n");

-----------------------------


Thank you!

Best regards,
Xiaochen Shen