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

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

Posted by Andrew Cooper 1 month, 3 weeks ago

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()

Posted by Jan Beulich 1 month, 3 weeks ago

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()

Posted by Andrew Cooper 1 month, 2 weeks ago

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()

Posted by Jan Beulich 1 month, 2 weeks ago

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

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

Posted by Roger Pau Monné 1 month, 3 weeks ago

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.