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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 19/08/2025 5:23 pm, Jan Beulich wrote:
> 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?

Yes.  The design of the functions they're used in.  Adding this line
results in:

add/remove: 0/0 grow/shrink: 7/2 up/down: 109/-36 (73)
Function                                     old     new   delta
read_msr                                    1243    1307     +64
resource_access                              326     341     +15
hwp_init_msrs.cold                           297     308     +11
probe_cpuid_faulting                         168     175      +7
svm_msr_read_intercept                      1034    1039      +5
hwp_write_request                            113     117      +4
hwp_init_msrs                                371     374      +3
amd_log_freq                                 844     828     -16
guest_rdmsr                                 2168    2148     -20

Taking read_msr() as a concrete example, this is because it's a store
into a parent functions variable, not into a local variable, and cannot
be elided.


>
>> 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.)

Yes I was referring to the uninitialised variable diagnostic.  *_safe()
are fairly rare, and we've got plenty of coverage in CI.

~Andrew