Re: [PATCH v2 13/16] x86/msr: Use MSR_IMM when available

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2025 22:41, Andrew Cooper wrote:
> Most MSR accesses have compile time constant indexes.  By using the immediate
> form when available, the decoder can start issuing uops directly for the
> relevant MSR, rather than having to issue uops to implement "switch (%ecx)".
> Modern CPUs have tens of thousands of MSRs, so that's quite an if/else chain.
> 
> Create __{rdmsr,wrmsrns}_imm() helpers and use them from {rdmsr,wrmsrns}()
> when the compiler can determine that the msr index is known at compile time.
> 
> At the instruction level, the combined ABI is awkward.  Explain our choices in
> detail.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> 
> The expression wrmsrns(MSR_STAR, rdmsr(MSR_STAR)) now yields:
> 
>   <test_star>:
>       b9 81 00 00 c0          mov    $0xc0000081,%ecx
>       0f 32                   rdmsr
>       48 c1 e2 20             shl    $0x20,%rdx
>       48 09 d0                or     %rdx,%rax
>       48 89 c2                mov    %rax,%rdx
>       48 c1 ea 20             shr    $0x20,%rdx
>       2e 0f 30                cs wrmsr
>       e9 a3 84 e8 ff          jmp    ffff82d040204260 <__x86_return_thunk>
> 
> which is as good as we can manage.  The alternative form of this looks like:
> 
>   <test_star>:
>       b9 81 00 00 c0          mov    $0xc0000081,%ecx
>       c4 e7 7b f6 c0 81 00    rdmsr  $0xc0000081,%rax
>       00 c0
>       2e c4 e7 7a f6 c0 81    cs wrmsrns %rax,$0xc0000081
>       00 00 c0
>       e9 xx xx xx xx          jmp    ffff82d040204260 <__x86_return_thunk>
> 
> Still TBD.  We ought to update the *_safe() forms too.  rdmsr_safe() is easier
> because the potential #GP locations line up, but there need to be two variants
> because of

Because of ...?

> --- a/xen/arch/x86/include/asm/alternative.h
> +++ b/xen/arch/x86/include/asm/alternative.h
> @@ -151,6 +151,13 @@ extern void alternative_instructions(void);
>          ALTERNATIVE(oldinstr, newinstr, feature)                        \
>          :: input )
>  
> +#define alternative_input_2(oldinstr, newinstr1, feature1,              \
> +                            newinstr2, feature2, input...)              \
> +    asm_inline volatile (                                               \
> +        ALTERNATIVE_2(oldinstr, newinstr1, feature1,                    \
> +                      newinstr2, feature2)                              \
> +        :: input )
> +
>  /* Like alternative_input, but with a single output argument */
>  #define alternative_io(oldinstr, newinstr, feature, output, input...)   \
>      asm_inline volatile (                                               \
> diff --git a/xen/arch/x86/include/asm/msr.h b/xen/arch/x86/include/asm/msr.h
> index 1bd27b989a4d..2ceff6cca8bb 100644
> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -29,10 +29,52 @@
>   *  wrmsrl(MSR_FOO, val);
>   */
>  
> -static inline uint64_t rdmsr(unsigned int msr)
> +/*
> + * RDMSR with a compile-time constant index, when available.  Falls back to
> + * plain RDMSR.
> + */
> +static always_inline uint64_t __rdmsr_imm(uint32_t msr)
> +{
> +    uint64_t val;
> +
> +    /*
> +     * For best performance, RDMSR $msr, %r64 is recommended.  For
> +     * compatibility, we need to fall back to plain RDMSR.
> +     *
> +     * The combined ABI is awkward, because RDMSR $imm produces an r64,
> +     * whereas WRMSR{,NS} produces a split edx:eax pair.
> +     *
> +     * Always use RDMSR $imm, %rax, because it has the most in common with 
> the
> +     * legacy form.  When MSR_IMM isn't available, emit logic to fold %edx
> +     * back into %rax.
> +     *
> +     * Let the compiler do %ecx setup.  This does mean there's a useless `mov
> +     * $imm, %ecx` in the instruction stream in the MSR_IMM case, but it 
> means
> +     * the compiler can de-duplicate the setup in the common case of reading
> +     * and writing the same MSR.
> +     */
> +    alternative_io(
> +        "rdmsr\n\t"
> +        "shl $32, %%rdx\n\t"
> +        "or %%rdx, %%rax\n\t",
> +
> +        /* RDMSR $msr, %rax */
> +        ".byte 0xc4,0xe7,0x7b,0xf6,0xc0; .long %c[msr]", X86_FEATURE_MSR_IMM,
> +
> +        "=a" (val),

Strictly speaking "=&a". Not that it matters much here; just to not
set a bad precedent.

> @@ -55,11 +97,51 @@ static inline void wrmsr(unsigned int msr, uint64_t val)
>  }
>  #define wrmsrl(msr, val) wrmsr(msr, val)
>  
> +/*
> + * 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
> +     * %edx.
> +     *
> +     * Let the compiler do %ecx setup.  This does mean there's a useless `mov
> +     * $imm, %ecx` in the instruction stream in the MSR_IMM case, but it 
> means
> +     * the compiler can de-duplicate the setup in the common case of reading
> +     * and writing the same MSR.
> +     */
> +    alternative_input_2(
> +        "mov %%rax, %%rdx\n\t"
> +        "shr $32, %%rdx\n\t"
> +        ".byte 0x2e; wrmsr",
> +
> +        /* CS WRMSRNS %rax, $msr */
> +        ".byte 0x2e,0xc4,0xe7,0x7a,0xf6,0xc0; .long %c[msr]", 
> X86_FEATURE_MSR_IMM,
> +
> +        "mov %%rax, %%rdx\n\t"
> +        "shr $32, %%rdx\n\t"
> +        ".byte 0x0f,0x01,0xc6", X86_FEATURE_WRMSRNS,

Isn't this the wrong way round for hardware which has both features? The
last active alternative wins, iirc.

Jan