[PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled

Maciej Wieczor-Retman posted 3 patches 1 year, 2 months ago
There is a newer version of this series
[PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled
Posted by Maciej Wieczor-Retman 1 year, 2 months ago
Until LASS is merged into the kernel [1], LAM is left disabled in the
config file. Running the LAM selftest with disabled LAM only results in
unhelpful output.

Use one of LAM syscalls() to determine whether the kernel was compiled
with LAM support (CONFIG_ADDRESS_MASKING) or not. Skip running the tests
in the latter case.

[1] https://lore.kernel.org/all/20241028160917.1380714-1-alexander.shishkin@linux.intel.com/

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 v4:
- Add this patch to the series.

 tools/testing/selftests/x86/lam.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/tools/testing/selftests/x86/lam.c b/tools/testing/selftests/x86/lam.c
index 93f2f762d6b5..4ec37a4be007 100644
--- a/tools/testing/selftests/x86/lam.c
+++ b/tools/testing/selftests/x86/lam.c
@@ -124,6 +124,14 @@ static inline int cpu_has_lam(void)
 	return (cpuinfo[0] & (1 << 26));
 }
 
+static inline int kernel_has_lam(void)
+{
+	unsigned long bits;
+
+	syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
+	return !!bits;
+}
+
 static inline int cpu_has_la57(void)
 {
 	return !system("grep -wq la57 /proc/cpuinfo");
@@ -1181,6 +1189,11 @@ int main(int argc, char **argv)
 		return KSFT_SKIP;
 	}
 
+	if (!kernel_has_lam()) {
+		ksft_print_msg("LAM is disabled in the kernel!\n");
+		return KSFT_SKIP;
+	}
+
 	while ((c = getopt(argc, argv, "ht:")) != -1) {
 		switch (c) {
 		case 't':
-- 
2.47.1
Re: [PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled
Posted by Dave Hansen 1 year ago
On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
> +static inline int kernel_has_lam(void)
> +{
> +	unsigned long bits;
> +
> +	syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
> +	return !!bits;
> +}

Generally, I'm less picky about selftest/ code than in-kernel code. But
people really do take selftest code and use it as a starting point for
production code.

I'd much rather have overly verbose, obviously correct code:

	err = syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);

	/* Handle syscall failure, like pre-LAM kernels: */
	if (err)
		return 0

	/* Tag bits are empty on non-LAM systems: */
	return !!bits;

Actually, I was going to argue for that^ just on style and writing good
code. But then I spotted a bug. What happens if the kernel has
CONFIG_ADDRESS_MASKING=n, either because it is config'd off or it's old?
The:

	put_user(0, (unsigned long __user *)arg2);

won't ever get run and 'bits' will be uninitialized.

So, I think this code was trying to be compact, fast and clever. But it
really just turns out to be buggy.
Re: [PATCH v5 2/3] selftests/lam: Skip test if LAM is disabled
Posted by Maciej Wieczor-Retman 1 year ago
On 2025-01-24 at 08:23:09 -0800, Dave Hansen wrote:
>On 11/27/24 09:35, Maciej Wieczor-Retman wrote:
>> +static inline int kernel_has_lam(void)
>> +{
>> +	unsigned long bits;
>> +
>> +	syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>> +	return !!bits;
>> +}
>
>Generally, I'm less picky about selftest/ code than in-kernel code. But
>people really do take selftest code and use it as a starting point for
>production code.
>
>I'd much rather have overly verbose, obviously correct code:
>
>	err = syscall(SYS_arch_prctl, ARCH_GET_MAX_TAG_BITS, &bits);
>
>	/* Handle syscall failure, like pre-LAM kernels: */
>	if (err)
>		return 0
>
>	/* Tag bits are empty on non-LAM systems: */
>	return !!bits;
>

Sure, more comments is always good :)

>Actually, I was going to argue for that^ just on style and writing good
>code. But then I spotted a bug. What happens if the kernel has
>CONFIG_ADDRESS_MASKING=n, either because it is config'd off or it's old?
>The:
>
>	put_user(0, (unsigned long __user *)arg2);
>
>won't ever get run and 'bits' will be uninitialized.

Huh, yeah, you're right. I tested it with both CONFIG_ADDRESS_MASKING=n and =y,
and on systems with it both available and not available but must've been a
coincidence it worked.

I'll fix the checks and init bits for the next version.

>
>So, I think this code was trying to be compact, fast and clever. But it
>really just turns out to be buggy.
>

-- 
Kind regards
Maciej Wieczór-Retman