On 16/01/2023 2:17 pm, Jan Beulich wrote: > On 16.01.2023 14:00, Andrew Cooper wrote: >> On 12/01/2023 4:51 pm, Andrew Cooper wrote: >>> On 12/01/2023 1:10 pm, Jan Beulich wrote: >>>> On 10.01.2023 18:18, Andrew Cooper wrote: >>>>> --- a/xen/arch/x86/setup.c >>>>> +++ b/xen/arch/x86/setup.c >>>>> @@ -54,6 +54,7 @@ >>>>> #include <asm/spec_ctrl.h> >>>>> #include <asm/guest.h> >>>>> #include <asm/microcode.h> >>>>> +#include <asm/prot-key.h> >>>>> #include <asm/pv/domain.h> >>>>> >>>>> /* opt_nosmp: If true, secondary processors are ignored. */ >>>>> @@ -1804,6 +1805,9 @@ void __init noreturn __start_xen(unsigned long >>>>> mbi_p) >>>>> if ( opt_invpcid && cpu_has_invpcid ) >>>>> use_invpcid = true; >>>>> >>>>> + if ( cpu_has_pks ) >>>>> + wrpkrs_and_cache(0); /* Must be before setting CR4.PKS */ >>>> Same question here as for PKRU wrt the BSP during S3 resume. >>> I had reasoned not, but it turns out that I'm wrong. >>> >>> It's important to reset the cache back to 0 here. (Handling PKRU is >>> different - I'll follow up on the other email..) >> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c >> index d23335391c67..de9317e8c573 100644 >> --- 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. I will add a comment about resetting the cache, because that is the point of doing this operation. If we do find firmware which really does mess around with MSR_PKRS (and isn't restoring the pre-S3 value), then this clause needs moving down into asm before we restore %cr4 fully, but TBH, I hope I've had time to try and unify the boot and S3 resume paths a bit before then. >> As its the final hunk before the entire series can be committed, I >> shan't bother sending a v3 just for this. > If you're seeing reasons not to be concerned of the unspecified reset > state, then feel free to add my A-b (but not R-b) here. There are several reasons not to be concerned. I guess I'll take your ack then. Thanks. ~Andrew
