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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 21.08.2025 18:20, Andrew Cooper wrote:
> 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.

Well, okay, slightly hesitantly
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
preferably with the paragraph in the description that I commented on
slightly expanded to cover the "why" aspect.

Jan