On 16.01.2023 15:57, Andrew Cooper wrote: > On 16/01/2023 2:17 pm, Jan Beulich wrote: >> On 16.01.2023 14:00, Andrew Cooper wrote: >>> --- a/xen/arch/x86/acpi/power.c >>> +++ b/xen/arch/x86/acpi/power.c >>> @@ -299,6 +299,13 @@ static int enter_state(u32 state) >>> >>> update_mcu_opt_ctrl(); >>> >>> + /* >>> + * Should be before restoring CR4, but that is earlier in asm. We >>> rely on >>> + * MSR_PKRS actually being 0 out of S3 resume. >>> + */ >>> + if ( cpu_has_pks ) >>> + wrpkrs_and_cache(0); >>> + >>> /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */ >>> percpu_traps_init(); >>> >>> >>> I've folded this hunk, to sort out the S3 resume path. >> The comment is a little misleading imo - it looks to justify that nothing >> needs doing. Could you add "..., but our cache needs clearing" to clarify >> why, despite our relying on zero being in the register (which I find >> problematic, considering that the doc doesn't even spell out reset state), >> the write is needed? > > Xen doesn't actually set CR4.PKS at all (yet). > > I'm just trying to do a reasonable job of leaving Xen in a position > where it doesn't explode the instant we want to start using PKS ourselves. > > S3 resume is out of a full core poweroff. Registers (which aren't > modified by firmware) will have their architectural reset values, and > for MSR_PKRS, that's 0.
And where have you found that to be spelled out? It is this lack of specification (afaics) which is concerning me. Jan
