[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/boot: Further simplify CR4 handling in dom0_construct_pv()
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |