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
