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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 19.08.2025 15:52, Andrew Cooper wrote:
> 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.

But the compiler is dealing with such indirection in inline functions just
fine. I don't expect it would typically spill val to the stack. Is there
anything specific here that you think would make this more likely?

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

A compiler error? (Hmm, perhaps you think of uninitialized variable
diagnostics. That may or may not trigger, depending on how else the
caller's variable is used.)

Jan