[PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag

Maciej Wieczor-Retman posted 3 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag
Posted by Maciej Wieczor-Retman 1 year, 2 months ago
In current form cpu_has_la57() reports platform's support for LA57
through reading the output of cpuid. A much more useful information is
whether 5-level paging is actually enabled on the running system.

Presence of the la57 flag in /proc/cpuinfo signifies that 5-level paging
was compiled into the kernel, is supported by the platform and wasn't
disabled by kernel command line argument.

Use system() with cat and grep to figure out if la57 is enabled on the
running system.

Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
---
Changelog v5:
- Remove "cat" from system() call and use only "grep".

Changelog v4:
- Add this patch to the series.

 tools/testing/selftests/x86/lam.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
index 0ea4f6813930..93f2f762d6b5 100644
--- a/tools/testing/selftests/x86/lam.c
+++ b/tools/testing/selftests/x86/lam.c
@@ -124,14 +124,9 @@ static inline int cpu_has_lam(void)
 	return (cpuinfo[0] & (1 << 26));
 }
 
-/* Check 5-level page table feature in CPUID.(EAX=07H, ECX=00H):ECX.[bit 16] */
 static inline int cpu_has_la57(void)
 {
-	unsigned int cpuinfo[4];
-
-	__cpuid_count(0x7, 0, cpuinfo[0], cpuinfo[1], cpuinfo[2], cpuinfo[3]);
-
-	return (cpuinfo[2] & (1 << 16));
+	return !system("grep -wq la57 /proc/cpuinfo");
 }
 
 /*
-- 
2.47.1
Re: [PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag
Posted by Dave Hansen 1 year ago
On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
> -/* Check 5-level page table feature in CPUID.(EAX=07H, ECX=00H):ECX.[bit 16] */
>  static inline int cpu_has_la57(void)
>  {
> -	unsigned int cpuinfo[4];
> -
> -	__cpuid_count(0x7, 0, cpuinfo[0], cpuinfo[1], cpuinfo[2], cpuinfo[3]);
> -
> -	return (cpuinfo[2] & (1 << 16));
> +	return !system("grep -wq la57 /proc/cpuinfo");
>  }

I would rather we find another way.

First, we've documented the behavior a bit in here:

	https://docs.kernel.org/arch/x86/cpuinfo.html

The important part is:

	"The absence of a flag in /proc/cpuinfo by itself means almost
	nothing to an end user."

Even worse, let's say there's a CPU bug and we say define a bug bit:

	bugs		: spectre_v1 spectre_v2 ... la57_is_broken

How is that grep going to work out? ;)

Could you poke around and see if there is any existing ABI that we can
use to query LA57 support? Maybe one of the things KVM exports, or some
TASK_SIZE_MAX comparisons?
Re: [PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag
Posted by Maciej Wieczor-Retman 1 year ago
On 2025-01-24 at 08:14:41 -0800, Dave Hansen wrote:
>On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
>> -/* Check 5-level page table feature in CPUID.(EAX=07H, ECX=00H):ECX.[bit 16] */
>>  static inline int cpu_has_la57(void)
>>  {
>> -	unsigned int cpuinfo[4];
>> -
>> -	__cpuid_count(0x7, 0, cpuinfo[0], cpuinfo[1], cpuinfo[2], cpuinfo[3]);
>> -
>> -	return (cpuinfo[2] & (1 << 16));
>> +	return !system("grep -wq la57 /proc/cpuinfo");
>>  }
>
>I would rather we find another way.
>
>First, we've documented the behavior a bit in here:
>
>	https://docs.kernel.org/arch/x86/cpuinfo.html
>
>The important part is:
>
>	"The absence of a flag in /proc/cpuinfo by itself means almost
>	nothing to an end user."
>
>Even worse, let's say there's a CPU bug and we say define a bug bit:
>
>	bugs		: spectre_v1 spectre_v2 ... la57_is_broken
>
>How is that grep going to work out? ;)
>
>Could you poke around and see if there is any existing ABI that we can
>use to query LA57 support? Maybe one of the things KVM exports, or some
>TASK_SIZE_MAX comparisons?

Sure, I'll try to find some other way.

My previous tactic was to munmap() a high address and see if it works. Does that
sound okay in case there isn't anything else would indicate la57 to userspace
reliably?

>

-- 
Kind regards
Maciej Wieczór-Retman
Re: [PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag
Posted by Maciej Wieczor-Retman 1 year ago
On 2025-01-24 at 21:17:22 +0100, Maciej Wieczor-Retman wrote:
>On 2025-01-24 at 08:14:41 -0800, Dave Hansen wrote:
>>On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
>Sure, I'll try to find some other way.
>
>My previous tactic was to munmap() a high address and see if it works. Does that
>sound okay in case there isn't anything else would indicate la57 to userspace
>reliably?

Sorry, meant mmap() not munmap()

>
>>
>
>-- 
>Kind regards
>Maciej Wieczór-Retman

-- 
Kind regards
Maciej Wieczór-Retman
Re: [PATCH v5 1/3] selftests/lam: Move cpu_has_la57() to use cpuinfo flag
Posted by Dave Hansen 1 year ago
On 1/24/25 12:17, Maciej Wieczor-Retman wrote:
>> Could you poke around and see if there is any existing ABI that we can
>> use to query LA57 support? Maybe one of the things KVM exports, or some
>> TASK_SIZE_MAX comparisons?
> Sure, I'll try to find some other way.
> 
> My previous tactic was to munmap() a high address and see if it works. Does that
> sound okay in case there isn't anything else would indicate la57 to userspace
> reliably?

Yeah, I think that's fine. I can't think of any obvious horrible
pitfalls. All I'd ask is that you spend 20 minutes grepping for things
that are conditional on X86_FEATURE_LA57 and see if there's anything
else that's a good candidate.