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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/01/2023 3:04 pm, Jan Beulich wrote:
> 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.

I have a request for an update to table 10-1 already pending.  MSR_PKRS
isn't plausibly different from PKRU.

(And even if it is different, this still won't matter while Xen doesn't
use CR4.PKS itself.)

~Andrew