Re: [PATCH] x86/AMD: Convert rdmsr_amd_safe() to use asm goto()

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 07/04/2025 4:48 pm, Jan Beulich wrote:
> On 07.04.2025 17:35, Andrew Cooper wrote:
>> Unlike the WRMSR side, we can't use asm goto() unconditionally, because our
>> toolchain baseline doesn't support asm goto with outputs.
> Is there actually a benefit we gain from now needing to maintain two different
> pieces of logic fulfilling the same purpose?

IMO, yes.  Besides getting rid of .fixup/unlikely, the code generation
is better-enough to warrant it, including getting the common path
correct (the referenced labels are all considered cold).

e.g. for this change, we go from:

    xor    %esi,%esi
    rdmsr
    test   %esi,%esi
    jne    <init_amd+0x540>
    and    $0xfffffffe,%edx
    wrmsr

(note the forward branch) to simply:

    rdmsr
    and    $0xfffffffe,%edx
    wrmsr

because the exception table redirect is directly into init_amd.cold, and
we don't have to hold `int err` in a register across the asm() block.

This is an intentionally simple example to get the infrastructure in,
but vmread() will definitely benefit.

>
>> Also, there's a different errata workaround we'll need if we want to use asm
>> goto() with "+" constraints:
>>
>> config CC_HAS_ASM_GOTO_TIED_OUTPUT
>>      depends on CC_HAS_ASM_GOTO_OUTPUT
>>      # Detect buggy gcc and clang, fixed in gcc-11 clang-14.
>>      def_bool $(success,echo 'int foo(int *x) { asm goto (".long (%l[bar]) - 
>> .": "+m"(*x) ::: bar); return *x; bar: return 0; }' | $CC -x c - -c -o 
>> /dev/null)
>>
>> I'm tempted to put it in straight away, lest we forget about it.
> Perhaps best if we really want to go this route. Yet then - why "TIED"? Isn't
> "tied" the term they use when referring to an earlier operand by using a
> digit (or the operand's name in square brackets)?

This is straight from Linux.  I've not looked at the issue in detail.

>
>> --- a/xen/Kconfig
>> +++ b/xen/Kconfig
>> @@ -41,6 +41,20 @@ config CC_SPLIT_SECTIONS
>>  config CC_HAS_UBSAN
>>      def_bool $(cc-option,-fsanitize=undefined)
>>  
>> +# Fixed in GCC 14, 13.3, 12.4 and 11.5
>> +# https://gcc.gnu.org/bugzilla/show_bug.cgi?id=113921
>> +config GCC_ASM_GOTO_OUTPUT_BROKEN
>> +    bool
>> +    depends on CC_IS_GCC
>> +    default y if GCC_VERSION < 110500
>> +    default y if GCC_VERSION >= 120000 && GCC_VERSION < 120400
>> +    default y if GCC_VERSION >= 130000 && GCC_VERSION < 130300
> Unlike for pre-release versions (x.0.y) I view this as problematic. Distros
> are likely to have backported the fix before the minor releases took place.
> Or they may have backported without ever meaning to follow later minor
> releases. We'd needlessly exclude them here. Imo ...
>
>> +config CC_HAS_ASM_GOTO_OUTPUT
>> +    def_bool y
>> +    depends on !GCC_ASM_GOTO_OUTPUT_BROKEN
>> +    depends on $(success,echo 'int foo(int x) { asm goto ("": "=r"(x) ::: 
>> bar); return x; bar: return 0; }' | $(CC) -x c - -c -o /dev/null)
> ... the only option is to actually probe for support as well as the (non-)
> buggy-ness.

There is no sensible way to probe.  It compiles fine, but (AIUI) fails
to spill registers correctly on some paths, which also makes it very
sensitive to other optimisations.

~Andrew