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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 18/08/2025 12:27 pm, Jan Beulich wrote:
> 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.

I didn't realise I'd made a change at first, but it's a consequence of
the compiler's ability to rearrange basic blocks.

It can be fixed with ...

>
>> --- 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;
>> +    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:

    *val = 0;

here, but I don't want to do this.  Because val is by pointer and
generally spilled to the stack, the compiler can't optimise away the store.

I'd far rather get a real compiler error, than to have logic relying on
the result of a faulting MSR read.

>> +    return -EFAULT;
>> +#else
>>      int rc;
>>      uint64_t lo, hi;
> ... the same being needed here?

Fixed.

~Andrew