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 <jbeul...@suse.com>
Thanks.
~Andrew
