Re: [PATCH] RFC x86/msr: Use WRMSRNS $imm when available

[Jan Beulich <jbeulich@suse.com>]

On 11.08.2025 11:50, Andrew Cooper wrote:
> On 11/08/2025 9:16 am, Jan Beulich wrote:
>> On 09.08.2025 00:20, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/include/asm/msr.h
>>> +++ b/xen/arch/x86/include/asm/msr.h
>>> @@ -38,9 +38,46 @@ static inline void wrmsrl(unsigned int msr, uint64_t val)
>>>          wrmsr(msr, lo, hi);
>>>  }
>>>  
>>> +/*
>>> + * Non-serialising WRMSR with a compile-time constant index, when available.
>>> + * Falls back to plain WRMSRNS, or to a serialising WRMSR.
>>> + */
>>> +static always_inline void __wrmsrns_imm(uint32_t msr, uint64_t val)
>>> +{
>>> +    /*
>>> +     * For best performance, WRMSRNS %r64, $msr is recommended.  For
>>> +     * compatibility, we need to fall back to plain WRMSRNS, or to WRMSR.
>>> +     *
>>> +     * The combined ABI is awkward, because WRMSRNS $imm takes a single r64,
>>> +     * whereas WRMSR{,NS} takes a split edx:eax pair.
>>> +     *
>>> +     * Always use WRMSRNS %rax, $imm, because it has the most in common with
>>> +     * the legacy forms.  When MSR_IMM isn't available, emit setup logic for
>>> +     * %ecx and %edx too.
>>> +     */
>>> +    alternative_input_2(
>>> +        "mov $%c[msr], %%ecx\n\t"
>> Simply %[msr] here?
>>
>> And then, might it make sense to pull out this and ...
>>
>>> +        "mov %%rax, %%rdx\n\t"
>>> +        "shr $32, %%rdx\n\t"
>>> +        ".byte 0x2e; wrmsr",
>>> +
>>> +        /* WRMSRNS %rax, $msr */
>>> +        ".byte 0xc4,0xe7,0x7a,0xf6,0xc0; .long %c[msr]", X86_FEATURE_MSR_IMM,
>>> +
>>> +        "mov $%c[msr], %%ecx\n\t"
>> ... this, to ...
>>
>>> +        "mov %%rax, %%rdx\n\t"
>>> +        "shr $32, %%rdx\n\t"
>>> +        ".byte 0x0f,0x01,0xc6", X86_FEATURE_WRMSRNS,
>>> +
>>> +        [msr] "i" (msr), "a" (val) : "rcx", "rdx");
>>         [msr] "i" (msr), "a" (val), "c" (msr) : "rdx");
>>
>> allowing the compiler to actually know what's put in %ecx? That'll make
>> original and 2nd replacement code 10 bytes, better balancing with the 9
>> bytes of the 1st replacement. And I'd guess that the potentially dead
>> MOV to %ecx would be hidden in the noise as well.
> 
> I considered that, but what can the compiler do as a result of knowing %ecx?

For example ...

> That said, we do need an RDMSR form (which I desperately want to make
> foo = rdmsr(MSR_BAR) but my cleanup series from 2019 got nowhere), and
> in a read+write case I suppose the compiler could deduplicate the setup
> of %ecx.

... this. But also simply to use a good pattern (exposing as much as possible
to the compiler), so there are more good instances of code for future cloning
from. (In size-optimizing builds, the compiler could further favor ADD/SUB
over MOV when the two MSRs accessed are relatively close together.)

Jan