Re: [PATCH v2 5/8] x86/hvm: Context switch MSR_PKRS

[Jan Beulich <jbeulich@xxxxxxxx>]

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