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
