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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 12/01/2023 1:10 pm, Jan Beulich wrote:
> On 10.01.2023 18:18, Andrew Cooper wrote:
>> +static inline void wrpkrs(uint32_t pkrs)
>> +{
>> +    uint32_t *this_pkrs = &this_cpu(pkrs);
>> +
>> +    if ( *this_pkrs != pkrs )
>> +    {
>> +        *this_pkrs = pkrs;
>> +
>> +        wrmsr_ns(MSR_PKRS, pkrs, 0);
>> +    }
>> +}
>> +
>> +static inline void wrpkrs_and_cache(uint32_t pkrs)
>> +{
>> +    this_cpu(pkrs) = pkrs;
>> +    wrmsr_ns(MSR_PKRS, pkrs, 0);
>> +}
> Just to confirm - there's no anticipation of uses of this in async
> contexts, i.e. there's no concern about the ordering of cache vs hardware
> writes?

No.  The only thing modifying MSR_PKRS does is change how the pagewalk
works for the current thread (specifically, the determination of Access
Rights).  Their is no relevance outside of the core, especially for
Xen's local copy of the register value.

What WRMSRNS does guarantee is that older instructions will complete
before the MSR gets updated, and that subsequent instructions won't
start, so WRMSRNS acts "atomically" with respect to instruction order.

Also remember that not all WRMSRs are serialising.  e.g. the X2APIC MSRs
are explicitly not, and this is an oversight in practice for
MSR_X2APIC_ICR at least.

>> --- 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..)

~Andrew