Re: [PATCH 02/22] x86/msr: Rename wrmsr_ns() to wrmsrns(), and take 64bit value

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.08.2025 08:36, Andrew Cooper wrote:
> On 08/08/2025 9:22 pm, Andrew Cooper wrote:
>> In hindsight, having the wrapper name not be the instruction mnemonic was a
>> poor choice.  Also, PKS turns out to be quite rare in wanting a split value.
>>
>> Switch to using a single 64bit value in preparation for new users.
>>
>> No functional change.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> ---
>> CC: Jan Beulich <JBeulich@xxxxxxxx>
>> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
>> ---
>>  xen/arch/x86/include/asm/msr.h      | 4 ++--
>>  xen/arch/x86/include/asm/prot-key.h | 4 ++--
>>  2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
>> index 4c4f18b3a54d..b6b85b04c3fd 100644
>> --- a/xen/arch/x86/include/asm/msr.h
>> +++ b/xen/arch/x86/include/asm/msr.h
>> @@ -39,7 +39,7 @@ static inline void wrmsrl(unsigned int msr, uint64_t val)
>>  }
>>  
>>  /* 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)
>> +static inline void wrmsrns(uint32_t msr, uint64_t val)
>>  {
>>      /*
>>       * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant CS
>> @@ -47,7 +47,7 @@ static inline void wrmsr_ns(uint32_t msr, uint32_t lo, 
>> uint32_t hi)
>>       */
>>      alternative_input(".byte 0x2e; wrmsr",
>>                        ".byte 0x0f,0x01,0xc6", X86_FEATURE_WRMSRNS,
>> -                      "c" (msr), "a" (lo), "d" (hi));
>> +                      "c" (msr), "a" (val), "d" (val >> 32));
>>  }
> 
> It turns out this is the case poor code generation for MSR_STAR.
> 
> I've adjusted it to:
> 
> @@ -39,8 +39,10 @@ static inline void wrmsrl(unsigned int msr, uint64_t val)
>  }
>  
>  /* 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)
> +static inline void wrmsrns(uint32_t msr, uint64_t val)
>  {
> +    uint32_t lo = val, hi = val >> 32;
> +
>      /*
>       * WRMSR is 2 bytes.  WRMSRNS is 3 bytes.  Pad WRMSR with a redundant CS
>       * prefix to avoid a trailing NOP.
> 
> 
> which stops the compiler from loading the high half of %rax too.

Acked-by: Jan Beulich <jbeulich@xxxxxxxx>