Re: [PATCH] x86/boot: Further simplify CR4 handling in dom0_construct_pv()

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Tue, Sep 24, 2024 at 12:23:43PM +0100, Andrew Cooper wrote:
> The logic would be more robust disabling SMAP based on its precense in CR4,
> rather than on certain features.
> 
> A forthcoming feature, LASS, needs the same treatment here.  Introduce minimum
> enumeration information, although it will take a bit more work to get LASS
> fully usable in guests.

Reading the ISA, doesn't LASS require SMAP to be enabled in %cr4, and
hence disabling SMAP already disables LASS? (without having to toggle
the LASS %cr4 bit)

"A supervisor-mode data access causes a LASS violation only if
supervisor-mode access protection is enabled (because CR4.SMAP = 1)
and either RFLAGS.AC = 0 or the access implicitly accesses a system
data structure."

We can consider also disabling it, but I think it would need to be
noted that such disabling is not strictly necessary, as disabling SMAP
already disables LASS.

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> I know LASS can't be used with traditional PV guests, but I have some PV-lite
> plans.  The problem is the PV kernel, in CPL3, accessing addresses in the high
> canonincal half.
> ---
>  xen/arch/x86/include/asm/x86-defns.h        |  1 +
>  xen/arch/x86/pv/dom0_build.c                | 18 ++++++++++--------
>  xen/include/public/arch-x86/cpufeatureset.h |  1 +
>  3 files changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/xen/arch/x86/include/asm/x86-defns.h 
> b/xen/arch/x86/include/asm/x86-defns.h
> index caa92829eaa9..8f97fb1e6a12 100644
> --- a/xen/arch/x86/include/asm/x86-defns.h
> +++ b/xen/arch/x86/include/asm/x86-defns.h
> @@ -75,6 +75,7 @@
>  #define X86_CR4_PKE        0x00400000 /* enable PKE */
>  #define X86_CR4_CET        0x00800000 /* Control-flow Enforcement Technology 
> */
>  #define X86_CR4_PKS        0x01000000 /* Protection Key Supervisor */
> +#define X86_CR4_LASS       0x08000000 /* Linear Address Space Separation */
>  
>  /*
>   * XSTATE component flags in XCR0 | MSR_XSS
> diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
> index 262edb6bf2f0..f5c868df384f 100644
> --- a/xen/arch/x86/pv/dom0_build.c
> +++ b/xen/arch/x86/pv/dom0_build.c
> @@ -1057,29 +1057,31 @@ int __init dom0_construct_pv(struct domain *d,
>                               module_t *initrd,
>                               const char *cmdline)
>  {
> +    unsigned long cr4 = read_cr4();
> +    unsigned long mask = X86_CR4_SMAP | X86_CR4_LASS;

const maybe?  Seeing as it is read-only.

Thanks, Roger.