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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.09.2024 13:23, Andrew Cooper wrote:
> --- 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;
>      int rc;
>  
>      /*
> -     * Clear SMAP in CR4 to allow user-accesses in construct_dom0().  This
> -     * prevents us needing to write construct_dom0() in terms of
> +     * Clear SMAP/LASS in CR4 to allow user-accesses in construct_dom0().
> +     * This prevents us needing to write construct_dom0() in terms of
>       * copy_{to,from}_user().
>       */
> -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +    if ( cr4 & mask )
>      {
>          if ( IS_ENABLED(CONFIG_PV32) )
> -            cr4_pv32_mask &= ~X86_CR4_SMAP;
> +            cr4_pv32_mask &= ~mask;
>  
> -        write_cr4(read_cr4() & ~X86_CR4_SMAP);
> +        write_cr4(cr4 & ~mask);
>      }
>  
>      rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
>  
> -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +    if ( cr4 & mask )
>      {
> -        write_cr4(read_cr4() | X86_CR4_SMAP);
> +        write_cr4(cr4);
>  
>          if ( IS_ENABLED(CONFIG_PV32) )
> -            cr4_pv32_mask |= X86_CR4_SMAP;
> +            cr4_pv32_mask |= mask;

You may end up setting a bit here which wasn't previously set, and which
might then fault when cr4_pv32_restore tries to OR this into %cr4. Aiui
you must have tested this on LASS-capable hardware, for it to have worked.

Jan