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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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