Re: [PATCH v2 2/2] x86/dom0: only disable SMAP for the PV dom0 build

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.07.2024 18:47, Andrew Cooper wrote:
> On 30/07/2024 4:28 pm, Roger Pau Monne wrote:
>> The PVH dom0 builder doesn't switch page tables and has no need to run with
>> SMAP disabled.
>>
>> Use stac() and clac(), as that's safe to use even if events would hit in the
>> middle of the region with SMAP disabled.  Nesting stac() and clac() calls is
>> not safe, so the stac() call is done strictly after elf_load_binary() because
>> that uses raw_{copy_to,clear}_guest() accessors which toggle SMAP.
>>
>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> Summarising a discussion on Matrix.
> 
> There are 3 options.
> 
> 1) Simply reposition the write_cr4()/cr4_pv32_mask adjustments.
> 2) This form (use stac/clac instead of playing with cr4).
> 3) Delay the original set_in_cr4(SMAP).
> 
> As proved by the confusion thus far, cr4_pv32_mask adjustments are
> fragile and can go unnoticed in the general case (need a lucky watchdog
> NMI hit to trigger).  Hence I'd prefer to remove the adjustments.
> 
> Using stac()/clac() is much easier.  It is fragile because of nesting
> (no AC save/restore infrastructure), but any mistake here will be picked
> up reliably in Gitlab CI because both adl-* and zen3p-* support SMAP.
> 
> Personally I think option 2 is better than 1, hence why I suggested it. 
> It's got a fragile corner case but will be spotted reliably.

... when code paths in question are always taken. Any such operation on
a rarely taken code path quite likely won't be spotted by mere testing.

Jan

> However, it occurs to me that option 3 exists as well... just delay
> setting SMAP until after dom0 is made.  We have form now with only
> activating CET-SS on the way out of __start_xen().
> 
> Furthermore, option 3 would allow us to move the cr4_pv32_mask
> adjustment into set_in_cr4() and never need to opencode it.
> 
> (Although this is a bit tricky given the current code layout. 
> cr4_pv32_mask also wants to be __ro_after_init and non-existent in a
> !PV32 build.)
> 
> ~Andrew