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
