Re: [PATCH v2 4/8] x86: Initial support for WRMSRNS

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 12/01/2023 12:58 pm, Jan Beulich wrote:
> On 10.01.2023 18:18, Andrew Cooper wrote:
>> WRMSR Non-Serialising is an optimisation intended for cases where an MSR 
>> needs
>> updating, but architectural serialising properties are not needed.
>>
>> In is anticipated that this will apply to most if not all MSRs modified on
>> context switch paths.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
>
> This will allow me to drop half of what the respective emulator patch
> consists of, which I'm yet to post (but which in turn is sitting on
> top of many other already posted emulator patches). Comparing with
> that patch, one nit though:

I did wonder if you had some stuff queued up.  I do need to get back to
reviewing.

>
>> --- a/tools/misc/xen-cpuid.c
>> +++ b/tools/misc/xen-cpuid.c
>> @@ -189,6 +189,7 @@ static const char *const str_7a1[32] =
>>  
>>      [10] = "fzrm",          [11] = "fsrs",
>>      [12] = "fsrcs",
>> +    /* 18 */                [19] = "wrmsrns",
>>  };
> We commonly leave a blank line to indicate dis-contiguous entries.

Oops yes.  Will fix.

>
>> --- a/xen/arch/x86/include/asm/msr.h
>> +++ b/xen/arch/x86/include/asm/msr.h
>> @@ -38,6 +38,18 @@ static inline void wrmsrl(unsigned int msr, __u64 val)
>>          wrmsr(msr, lo, hi);
>>  }
>>  
>> +/* Non-serialising WRMSR, when available.  Falls back to a serialising 
>> WRMSR. */
>> +static inline void wrmsr_ns(uint32_t msr, uint32_t lo, uint32_t hi)
>> +{
>> +    /*
>> +     * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant CS
>> +     * prefix to avoid a trailing NOP.
>> +     */
>> +    alternative_input(".byte 0x2e; wrmsr",
>> +                      ".byte 0x0f,0x01,0xc6", X86_FEATURE_WRMSRNS,
>> +                      "c" (msr), "a" (lo), "d" (hi));
>> +}
> No wrmsrl_ns() and/or wrmsr_ns_safe() variants right away?

I still have a branch cleaning up MSR handling, which has been pending
since the Nanjing XenSummit, which makes some of those disappear.

But no - I wasn't planning to introduce helpers ahead of them being needed.

> Do you have any indications towards a CS prefix being the least risky
> one to use here (or in general)?

Yes.

Remember it's the prefix recommended for, and used by,
-mbranches-within-32B-boundaries to work around the Skylake jmp errata.

And based on this justification, its also the prefix we use for padding
on various jmp/call's for retpoline inlining purposes.

~Andrew