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

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

[Andrew Cooper]

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.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

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;
     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;
     }
 
     return rc;
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 8fa3fb711a8d..cbc0a3a8aa2b 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -303,6 +303,7 @@ XEN_CPUFEATURE(SM3,          10*32+ 1) /*A  SM3 Instructions */
 XEN_CPUFEATURE(SM4,          10*32+ 2) /*A  SM4 Instructions */
 XEN_CPUFEATURE(AVX_VNNI,     10*32+ 4) /*A  AVX-VNNI Instructions */
 XEN_CPUFEATURE(AVX512_BF16,  10*32+ 5) /*A  AVX512 BFloat16 Instructions */
+XEN_CPUFEATURE(LASS,         10*32+ 6) /*   Linear Address Space Separation */
 XEN_CPUFEATURE(CMPCCXADD,    10*32+ 7) /*a  CMPccXADD Instructions */
 XEN_CPUFEATURE(FZRM,         10*32+10) /*A  Fast Zero-length REP MOVSB */
 XEN_CPUFEATURE(FSRS,         10*32+11) /*A  Fast Short REP STOSB */
-- 
2.39.5

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

[Roger Pau Monné]

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@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> 
> 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.

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

[Andrew Cooper]

On 24/09/2024 1:30 pm, Roger Pau Monné wrote:
> 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.

Hmm.  LASS looks to have no CR4 dependencies on SMAP or SMEP, and the
ISE does suggest they can be used independently.

However, I see no connection to paging (beyond LMA), and that is going
to become a problem in due course.

Anyway - I'll drop the LASS aspect for now.  It can be left to whomever
gets some working real hardware first.

~Andrew

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

[Jan Beulich]

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

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

[Roger Pau Monné]

On Tue, Sep 24, 2024 at 03:44:07PM +0200, Jan Beulich wrote:
> 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.

Possibly also needs X86_CR4_LASS adding to the XEN_CR4_PV32_BITS
define, as otherwise it won't end up in cr4_pv32_mask in the first
place AFAICT.

Thanks, Roger.

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

[Andrew Cooper]

The logic would be more robust disabling SMAP based on its precense in CR4,
rather than SMAP's accociation with a synthetic feature.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Strip LASS changes back out.
---
 xen/arch/x86/pv/dom0_build.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/pv/dom0_build.c b/xen/arch/x86/pv/dom0_build.c
index 262edb6bf2f0..ee9ecdc2abbf 100644
--- a/xen/arch/x86/pv/dom0_build.c
+++ b/xen/arch/x86/pv/dom0_build.c
@@ -1057,6 +1057,7 @@ int __init dom0_construct_pv(struct domain *d,
                              module_t *initrd,
                              const char *cmdline)
 {
+    unsigned long cr4 = read_cr4();
     int rc;
 
     /*
@@ -1064,19 +1065,19 @@ int __init dom0_construct_pv(struct domain *d,
      * prevents us needing to write construct_dom0() in terms of
      * copy_{to,from}_user().
      */
-    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
+    if ( cr4 & X86_CR4_SMAP )
     {
         if ( IS_ENABLED(CONFIG_PV32) )
             cr4_pv32_mask &= ~X86_CR4_SMAP;
 
-        write_cr4(read_cr4() & ~X86_CR4_SMAP);
+        write_cr4(cr4 & ~X86_CR4_SMAP);
     }
 
     rc = dom0_construct(d, image, image_headroom, initrd, cmdline);
 
-    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
+    if ( cr4 & X86_CR4_SMAP )
     {
-        write_cr4(read_cr4() | X86_CR4_SMAP);
+        write_cr4(cr4);
 
         if ( IS_ENABLED(CONFIG_PV32) )
             cr4_pv32_mask |= X86_CR4_SMAP;
-- 
2.39.5

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

[Jan Beulich]

On 03.10.2024 01:20, Andrew Cooper wrote:
> The logic would be more robust disabling SMAP based on its precense in CR4,
> rather than SMAP's accociation with a synthetic feature.

It's hard to tell what's more robust without knowing what future changes
there might be. In particular ...

> @@ -1064,19 +1065,19 @@ int __init dom0_construct_pv(struct domain *d,
>       * prevents us needing to write construct_dom0() in terms of
>       * copy_{to,from}_user().
>       */
> -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> +    if ( cr4 & X86_CR4_SMAP )

... with this adjustment ...

>      {
>          if ( IS_ENABLED(CONFIG_PV32) )
>              cr4_pv32_mask &= ~X86_CR4_SMAP;

... this update of a global no longer occurs. Playing games with CR4
elsewhere might run into issues with this lack of updating.

As the future is unknown, I'm really fine either way, so if you continue
to think this way is strictly better:
Acked-by: Jan Beulich <jbeulich@suse.com>

Jan

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

[Roger Pau Monné]

On Fri, Oct 04, 2024 at 08:52:52AM +0200, Jan Beulich wrote:
> On 03.10.2024 01:20, Andrew Cooper wrote:
> > The logic would be more robust disabling SMAP based on its precense in CR4,
> > rather than SMAP's accociation with a synthetic feature.
> 
> It's hard to tell what's more robust without knowing what future changes
> there might be. In particular ...
> 
> > @@ -1064,19 +1065,19 @@ int __init dom0_construct_pv(struct domain *d,
> >       * prevents us needing to write construct_dom0() in terms of
> >       * copy_{to,from}_user().
> >       */
> > -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
> > +    if ( cr4 & X86_CR4_SMAP )
> 
> ... with this adjustment ...
> 
> >      {
> >          if ( IS_ENABLED(CONFIG_PV32) )
> >              cr4_pv32_mask &= ~X86_CR4_SMAP;
> 
> ... this update of a global no longer occurs. Playing games with CR4
> elsewhere might run into issues with this lack of updating.

Maybe we should assert the state of cr4 is as expected?

ASSERT(!boot_cpu_has(X86_FEATURE_XEN_SMAP) || (cr4 & X86_CR4_SMAP));

Thanks, Roger.

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

[Andrew Cooper]

On 04/10/2024 7:52 am, Jan Beulich wrote:
> On 03.10.2024 01:20, Andrew Cooper wrote:
>> The logic would be more robust disabling SMAP based on its precense in CR4,
>> rather than SMAP's accociation with a synthetic feature.
> It's hard to tell what's more robust without knowing what future changes
> there might be. In particular ...
>
>> @@ -1064,19 +1065,19 @@ int __init dom0_construct_pv(struct domain *d,
>>       * prevents us needing to write construct_dom0() in terms of
>>       * copy_{to,from}_user().
>>       */
>> -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
>> +    if ( cr4 & X86_CR4_SMAP )
> ... with this adjustment ...
>
>>      {
>>          if ( IS_ENABLED(CONFIG_PV32) )
>>              cr4_pv32_mask &= ~X86_CR4_SMAP;
> ... this update of a global no longer occurs. Playing games with CR4
> elsewhere might run into issues with this lack of updating.

We don't know the future, but I'm confused by your reasoning here. 
Right now there's an expectation/assumption that FEAT_XEN_SMAP == CR4.SMAP.

In fact, the logic in staging right now is wonky if FEAT_XEN_SMAP=1 but
CR4.SMAP=1.  In this case, we'll do nothing on the way in, and then
activate SMAP on the way out.

construct_dom0() will definitely crash if SMAP is active.  So looking at
CR4 is strictly better than accidentally falling into a FEAT_XEN_SMAP=0
but CR4.SMAP=1 case.

Needing to play with the global cr4_pv32_mask is a consequence of
choosing to disabling SMAP, rather than using STAC and/or rewriting
using copy_*_user().  If you want to avoid playing with cr4_pv32_mask,
we'll need to revisit this decision.

While the APs are active/working at this point in boot, they're not
running guests (32bit PV or otherwise), so alterations to cr4_pv32_mask
don't really matter.


But, as to the robustness.  The code is now written in terms of it's
actual requirements, not its assumptions, and no longer malfunctions
when it's assumptions are violated.

> As the future is unknown, I'm really fine either way, so if you continue
> to think this way is strictly better:
> Acked-by: Jan Beulich <jbeulich@suse.com>

Thanks.

~Andrew

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

[Jan Beulich]

On 04.10.2024 20:49, Andrew Cooper wrote:
> On 04/10/2024 7:52 am, Jan Beulich wrote:
>> On 03.10.2024 01:20, Andrew Cooper wrote:
>>> The logic would be more robust disabling SMAP based on its precense in CR4,
>>> rather than SMAP's accociation with a synthetic feature.
>> It's hard to tell what's more robust without knowing what future changes
>> there might be. In particular ...
>>
>>> @@ -1064,19 +1065,19 @@ int __init dom0_construct_pv(struct domain *d,
>>>       * prevents us needing to write construct_dom0() in terms of
>>>       * copy_{to,from}_user().
>>>       */
>>> -    if ( boot_cpu_has(X86_FEATURE_XEN_SMAP) )
>>> +    if ( cr4 & X86_CR4_SMAP )
>> ... with this adjustment ...
>>
>>>      {
>>>          if ( IS_ENABLED(CONFIG_PV32) )
>>>              cr4_pv32_mask &= ~X86_CR4_SMAP;
>> ... this update of a global no longer occurs. Playing games with CR4
>> elsewhere might run into issues with this lack of updating.
> 
> We don't know the future, but I'm confused by your reasoning here. 
> Right now there's an expectation/assumption that FEAT_XEN_SMAP == CR4.SMAP.
> 
> In fact, the logic in staging right now is wonky if FEAT_XEN_SMAP=1 but
> CR4.SMAP=1.  In this case, we'll do nothing on the way in, and then
> activate SMAP on the way out.

I assume you meant "but CR4.SMAP=0". In that case yes, the logic here would
(kind of as a side effect) correct the wrong combination of state.

> construct_dom0() will definitely crash if SMAP is active.  So looking at
> CR4 is strictly better than accidentally falling into a FEAT_XEN_SMAP=0
> but CR4.SMAP=1 case.

It's better when taking one possible perspective, yes. Otoh CR4.SMAP=1 when
FEAT_XEN_SMAP=0 is a bug, and hence deserves being noticed (if nothing
else then by Xen crashing).

> Needing to play with the global cr4_pv32_mask is a consequence of
> choosing to disabling SMAP, rather than using STAC and/or rewriting
> using copy_*_user().  If you want to avoid playing with cr4_pv32_mask,
> we'll need to revisit this decision.
> 
> While the APs are active/working at this point in boot, they're not
> running guests (32bit PV or otherwise), so alterations to cr4_pv32_mask
> don't really matter.

I didn't really think of APs, but of the BSP itself.

Jan