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

[Jan Beulich <jbeulich@xxxxxxxx>]

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