Re: [PATCH v2 02/16] x86/msr: Rework rdmsr_safe() using asm goto()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.08.2025 22:41, Andrew Cooper wrote:
> ... on capable toolchains.
> 
> This avoids needing to hold rc in a register across the RDMSR, and in most
> cases removes direct testing and branching based on rc, as the fault label can
> be rearranged to directly land on the out-of-line block.
> 
> There is a subtle difference in behaviour.  The old behaviour would, on fault,
> still produce 0's and write to val.
> 
> The new behaviour only writes val on success, and write_msr() is the only
> place where this matters.  Move temp out of switch() scope and initialise it
> to 0.

But what's the motivation behind making this behavioral change? At least in
the cases where the return value isn't checked, it would feel safer if we
continued clearing the value. Even if in all cases where this could matter
(besides the one you cover here) one can prove correctness by looking at
surrounding code.

> --- a/xen/arch/x86/include/asm/msr.h
> +++ b/xen/arch/x86/include/asm/msr.h
> @@ -55,6 +55,24 @@ static inline void wrmsrns(uint32_t msr, uint64_t val)
>  /* rdmsr with exception handling */
>  static inline int rdmsr_safe(unsigned int msr, uint64_t *val)
>  {
> +#ifdef CONFIG_CC_HAS_ASM_GOTO_OUTPUT
> +    uint64_t lo, hi;

Could at least this line move ahead of the #ifdef, to also cover ...

> +    asm_inline goto (
> +        "1: rdmsr\n\t"
> +        _ASM_EXTABLE(1b, %l[fault])
> +        : "=a" (lo), "=d" (hi)
> +        : "c" (msr)
> +        :
> +        : fault );
> +
> +    *val = lo | (hi << 32);
> +
> +    return 0;
> +
> + fault:
> +    return -EFAULT;
> +#else
>      int rc;
>      uint64_t lo, hi;

... the same being needed here?

Jan