[PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)

Yian Chen posted 7 patches 2 years, 8 months ago
[PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Yian Chen 2 years, 8 months ago
LASS is enabled via setting a CR4 bit if the platform
supports the feature.

LASS may be disabled in early boot time, for example,
by command line parameter clearcpuid=lass/390 or
vsyscall flag. In such cases, the CPU feature and CR4
bits will be cleared.

Signed-off-by: Yian Chen <yian.chen@intel.com>
Reviewed-by: Tony Luck <tony.luck@intel.com>
---
 arch/x86/kernel/cpu/common.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 9cfca3d7d0e2..efc7c7623968 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -412,6 +412,23 @@ static __always_inline void setup_umip(struct cpuinfo_x86 *c)
 	cr4_clear_bits(X86_CR4_UMIP);
 }
 
+static __always_inline void setup_lass(struct cpuinfo_x86 *c)
+{
+	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
+		cr4_set_bits(X86_CR4_LASS);
+	} else {
+		/*
+		 * only clear the feature and cr4 bits when hardware
+		 * supports LASS, in case it was enabled in a previous
+		 * boot (e.g., via kexec)
+		 */
+		if (cpu_has(c, X86_FEATURE_LASS)) {
+			cr4_clear_bits(X86_CR4_LASS);
+			clear_cpu_cap(c, X86_FEATURE_LASS);
+		}
+	}
+}
+
 /* These bits should not change their value after CPU init is finished. */
 static const unsigned long cr4_pinned_mask =
 	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
@@ -1848,6 +1865,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
 	setup_smep(c);
 	setup_smap(c);
 	setup_umip(c);
+	setup_lass(c);
 
 	/* Enable FSGSBASE instructions if available. */
 	if (cpu_has(c, X86_FEATURE_FSGSBASE)) {
-- 
2.34.1
Re: [PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Dave Hansen 2 years, 8 months ago
On 1/9/23 21:52, Yian Chen wrote:
> +static __always_inline void setup_lass(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> +		cr4_set_bits(X86_CR4_LASS);
> +	} else {
> +		/*
> +		 * only clear the feature and cr4 bits when hardware
> +		 * supports LASS, in case it was enabled in a previous
> +		 * boot (e.g., via kexec)
> +		 */
> +		if (cpu_has(c, X86_FEATURE_LASS)) {
> +			cr4_clear_bits(X86_CR4_LASS);
> +			clear_cpu_cap(c, X86_FEATURE_LASS);
> +		}
> +	}
> +}

Could you try testing this, please?

Please remember that there are three things in play here:
 1. disabled-features.h.  Makes cpu_feature_enabled(X86_FEATURE_LASS)==0
    when CONFIG_X86_LASS=n.
 2. The X86_FEATURE_LASS software feature bit itself.  clearcpuid=lass
    would clear it.
 3. The actual CPU enumeration of X86_FEATURE_LASS

The else{} is handling the case where X86_FEATURE_LASS is compiled out
but where the CPU supports LASS.  It doesn't do anything when the CPU
lacks LASS support *OR* when clearcpuid=lass is used.

In the end, want X86_CR4_LASS set when the kernel wants LASS and clear
in *ALL* other cases.  That would be simply:

	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
		cr4_set_bits(X86_CR4_LASS);
	} else {
		cr4_clear_bits(X86_CR4_LASS);
	}

The cr4_clear_bits(X86_CR4_LASS) should be safe regardless of CPU or
kernel support for LASS.

As for the:

	clear_cpu_cap(c, X86_FEATURE_LASS);

It really only matters for kernels where CONFIG_X86_LASS=n but where the
CPU supports it.  I'm not clear on what specifically you expect to gain
from it, though.

I'm also wondering if we even want a Kconfig option.  Is anyone
realistically going to be compiling this support out?
Re: [PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Sohil Mehta 2 years, 8 months ago
On 1/12/2023 10:17 AM, Dave Hansen wrote:

> In the end, want X86_CR4_LASS set when the kernel wants LASS and clear
> in *ALL* other cases.  That would be simply:
> 
> 	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> 		cr4_set_bits(X86_CR4_LASS);
> 	} else {
> 		cr4_clear_bits(X86_CR4_LASS);
> 	}
> 

Thanks for the explanation. This is very helpful.

> I'm also wondering if we even want a Kconfig option.  Is anyone
> realistically going to be compiling this support out?

I was initially thinking we should leave the Kconfig option for users
that really *need* vsyscall support. But, thinking about it more, we
don't need an LASS Kconfig option for that.

We can make CONFIG_LEGACY_VSYSCALL_NONE as the default instead of the
current CONFIG_LEGACY_VSYSCALL_XONLY. The kernel can disable LASS at
boot time if a user/admin has shown explicit preference by selecting
CONFIG_LEGACY_VSYSCALL_XONLY or by providing command line parameter
vsyscall=xonly/emulate.

Thoughts?
Re: [PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Sohil Mehta 2 years, 8 months ago
Adding folks from the previous discussion when a similar suggestion was
made by Kees. There didn't seem to be any major objection that time.

https://lore.kernel.org/lkml/202205111424.DEB0E3418@keescook/

On 1/12/2023 5:17 PM, Sohil Mehta wrote:
> We can make CONFIG_LEGACY_VSYSCALL_NONE as the default instead of the
> current CONFIG_LEGACY_VSYSCALL_XONLY. The kernel can disable LASS at
> boot time if a user/admin has shown explicit preference by selecting
> CONFIG_LEGACY_VSYSCALL_XONLY or by providing command line parameter
> vsyscall=xonly/emulate.
> 
> Thoughts?
Re: [PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Sohil Mehta 2 years, 8 months ago
> +static __always_inline void setup_lass(struct cpuinfo_x86 *c)
> +{
> +	if (cpu_feature_enabled(X86_FEATURE_LASS)) {
> +		cr4_set_bits(X86_CR4_LASS);
> +	} else {
> +		/*
> +		 * only clear the feature and cr4 bits when hardware
> +		 * supports LASS, in case it was enabled in a previous
> +		 * boot (e.g., via kexec)
> +		 */
> +		if (cpu_has(c, X86_FEATURE_LASS)) {
> +			cr4_clear_bits(X86_CR4_LASS);
> +			clear_cpu_cap(c, X86_FEATURE_LASS);
> +		}
> +	}
> +}

I am quite confused by the "else" code flow. Can you please help 
understand how this code path would be exercised?

Also, why don't other features such as SMAP or SMEP need this type of 
handling? I see something on similar lines for UMIP.

Also, how does the CR4 pinning code in the following patch play into 
this? Could it flag a warning when cr4_clear_bits() is called above?

> +
>   /* These bits should not change their value after CPU init is finished. */
>   static const unsigned long cr4_pinned_mask =
>   	X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
> @@ -1848,6 +1865,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>   	setup_smep(c);
>   	setup_smap(c);
>   	setup_umip(c);
> +	setup_lass(c);
>
Re: [PATCH 5/7] x86/cpu: Enable LASS (Linear Address Space Separation)
Posted by Chen, Yian 2 years, 8 months ago

On 1/11/2023 2:22 PM, Sohil Mehta wrote:
>> +static __always_inline void setup_lass(struct cpuinfo_x86 *c)
>> +{
>> +    if (cpu_feature_enabled(X86_FEATURE_LASS)) {
>> +        cr4_set_bits(X86_CR4_LASS);
>> +    } else {
>> +        /*
>> +         * only clear the feature and cr4 bits when hardware
>> +         * supports LASS, in case it was enabled in a previous
>> +         * boot (e.g., via kexec)
>> +         */
>> +        if (cpu_has(c, X86_FEATURE_LASS)) {
>> +            cr4_clear_bits(X86_CR4_LASS);
>> +            clear_cpu_cap(c, X86_FEATURE_LASS);
>> +        }
>> +    }
>> +}
> 
> I am quite confused by the "else" code flow. Can you please help 
> understand how this code path would be exercised?
>
The "else" branch is to disable LASS for every cpu. Addition to clear 
the CR4 bit, it would be better to clear cpu feature id for consistent 
result in every CPU as well, otherwise,  we could see the feature id is 
cleared in the boot-cpu only, for example, if user enables vsyscall.

> Also, why don't other features such as SMAP or SMEP need this type of 
> handling? 
It seems there is no need to have such if-else for SMAP/SMEP since the 
X86_SMAP/SMEP was gone from arch/x86/Kconfig.

I see something on similar lines for UMIP.
> 
> Also, how does the CR4 pinning code in the following patch play into 
> this?
I did not test if pinning code works as commented:  "/* These bits 
should not change their value after CPU init is finished. */"

Could it flag a warning when cr4_clear_bits() is called above?
I did not observed a warning for cr4_clear_bits(), this should be okay 
since it is called during init.
> 



>> +
>>   /* These bits should not change their value after CPU init is 
>> finished. */
>>   static const unsigned long cr4_pinned_mask =
>>       X86_CR4_SMEP | X86_CR4_SMAP | X86_CR4_UMIP |
>> @@ -1848,6 +1865,7 @@ static void identify_cpu(struct cpuinfo_x86 *c)
>>       setup_smep(c);
>>       setup_smap(c);
>>       setup_umip(c);
>> +    setup_lass(c);