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
