xen/arch/x86/include/asm/spec_ctrl.h | 4 ++-- xen/arch/x86/include/asm/spec_ctrl_asm.h | 14 +++++++------- 2 files changed, 9 insertions(+), 9 deletions(-)
https://github.com/llvm/llvm-project/issues/60792
RFC. I very much dislike this patch, but it does work for me.
Why the parameter name of foo? Turns out I found a different Clang-IAS
bug/misfeature when trying to name the parameter uniq.
In file included from arch/x86/asm-macros.c:3:
./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
.L\@\uniq\()fill_rsb_loop:
^
It appears you can't have any macro parameters beginning with a u.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
xen/arch/x86/include/asm/spec_ctrl.h | 4 ++--
xen/arch/x86/include/asm/spec_ctrl_asm.h | 14 +++++++-------
2 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h
index 3cf8a7d304d4..cd727be7c689 100644
--- a/xen/arch/x86/include/asm/spec_ctrl.h
+++ b/xen/arch/x86/include/asm/spec_ctrl.h
@@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
/* (ab)use alternative_input() to specify clobbers. */
- alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
+ alternative_input("", "DO_OVERWRITE_RSB foo=%=", X86_BUG_IBPB_NO_RET,
: "rax", "rcx");
}
@@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
*
* (ab)use alternative_input() to specify clobbers.
*/
- alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
+ alternative_input("", "DO_OVERWRITE_RSB foo=%=", X86_FEATURE_SC_RSB_IDLE,
: "rax", "rcx");
}
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index fab27ff5532b..06455c5663bb 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -117,7 +117,7 @@
.L\@_done:
.endm
-.macro DO_OVERWRITE_RSB tmp=rax
+.macro DO_OVERWRITE_RSB tmp=rax foo=
/*
* Requires nothing
* Clobbers \tmp (%rax by default), %rcx
@@ -136,27 +136,27 @@
mov $16, %ecx /* 16 iterations, two calls per loop */
mov %rsp, %\tmp /* Store the current %rsp */
-.L\@_fill_rsb_loop:
+.L\@\foo\()fill_rsb_loop:
.irp n, 1, 2 /* Unrolled twice. */
- call .L\@_insert_rsb_entry_\n /* Create an RSB entry. */
+ call .L\@\foo\()insert_rsb_entry_\n /* Create an RSB entry. */
int3 /* Halt rogue speculation. */
-.L\@_insert_rsb_entry_\n:
+.L\@\foo\()insert_rsb_entry_\n:
.endr
sub $1, %ecx
- jnz .L\@_fill_rsb_loop
+ jnz .L\@\foo\()fill_rsb_loop
mov %\tmp, %rsp /* Restore old %rsp */
#ifdef CONFIG_XEN_SHSTK
mov $1, %ecx
rdsspd %ecx
cmp $1, %ecx
- je .L\@_shstk_done
+ je .L\@\foo\()shstk_done
mov $64, %ecx /* 64 * 4 bytes, given incsspd */
incsspd %ecx /* Restore old SSP */
-.L\@_shstk_done:
+.L\@\foo\()shstk_done:
#endif
.endm
--
2.30.2
On 17/02/2023 12:19 am, Andrew Cooper wrote: > https://github.com/llvm/llvm-project/issues/60792 > > RFC. I very much dislike this patch, but it does work for me. > > Why the parameter name of foo? Turns out I found a different Clang-IAS > bug/misfeature when trying to name the parameter uniq. > > In file included from arch/x86/asm-macros.c:3: > ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode] > .L\@\uniq\()fill_rsb_loop: > ^ > > It appears you can't have any macro parameters beginning with a u. It's worth saying that I can't repro this in more simple setups, so it likely specific to some of the gymnastics we do in asm-macros.c Also, I was hoping to try and sort out the macro such that it had .macro ... uniq=\@ reducing the internal complexity, but I couldn't find any form of that that GAS was happy with. Suggestions welcome. ~Andrew
https://github.com/llvm/llvm-project/issues/60792
It turns out that Clang-IAS does not expand \@ uniquely in a translaition
unit, and the XSA-426 change tickles this bug:
<instantiation>:4:1: error: invalid symbol redefinition
.L1_fill_rsb_loop:
^
make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
too, which Clang does seem to expand properly.
Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
v1 (vs RFC):
* Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
I originally tried a parameter named uniq but this found a different Clang-IAS
bug:
In file included from arch/x86/asm-macros.c:3:
./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
.L\@\uniq\()fill_rsb_loop:
^
It appears that Clang is looking for unicode escapes before completing
parameter substitution. But the macro didn't originate from a context where a
unicode escape was even applicable to begin with.
The consequence is that you can't use macro prameters beginning with a u.
---
xen/arch/x86/include/asm/spec_ctrl.h | 4 ++--
xen/arch/x86/include/asm/spec_ctrl_asm.h | 18 +++++++++++-------
2 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/include/asm/spec_ctrl.h b/xen/arch/x86/include/asm/spec_ctrl.h
index 3cf8a7d304d4..ddb2247fcb0a 100644
--- a/xen/arch/x86/include/asm/spec_ctrl.h
+++ b/xen/arch/x86/include/asm/spec_ctrl.h
@@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
/* (ab)use alternative_input() to specify clobbers. */
- alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
+ alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
: "rax", "rcx");
}
@@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
*
* (ab)use alternative_input() to specify clobbers.
*/
- alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
+ alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
: "rax", "rcx");
}
diff --git a/xen/arch/x86/include/asm/spec_ctrl_asm.h b/xen/arch/x86/include/asm/spec_ctrl_asm.h
index fab27ff5532b..1220f219f695 100644
--- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
+++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
@@ -117,11 +117,15 @@
.L\@_done:
.endm
-.macro DO_OVERWRITE_RSB tmp=rax
+.macro DO_OVERWRITE_RSB tmp=rax x=
/*
* Requires nothing
* Clobbers \tmp (%rax by default), %rcx
*
+ * x= is an optional parameter to work around
+ * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
+ * expand \@ uniquely, and is intended for muxing %= in too.
+ *
* Requires 256 bytes of {,shadow}stack space, but %rsp/SSP has no net
* change. Based on Google's performance numbers, the loop is unrolled to 16
* iterations and two calls per iteration.
@@ -136,27 +140,27 @@
mov $16, %ecx /* 16 iterations, two calls per loop */
mov %rsp, %\tmp /* Store the current %rsp */
-.L\@_fill_rsb_loop:
+.L\x\@_fill_rsb_loop:
.irp n, 1, 2 /* Unrolled twice. */
- call .L\@_insert_rsb_entry_\n /* Create an RSB entry. */
+ call .L\x\@_insert_rsb_entry_\n /* Create an RSB entry. */
int3 /* Halt rogue speculation. */
-.L\@_insert_rsb_entry_\n:
+.L\x\@_insert_rsb_entry_\n:
.endr
sub $1, %ecx
- jnz .L\@_fill_rsb_loop
+ jnz .L\x\@_fill_rsb_loop
mov %\tmp, %rsp /* Restore old %rsp */
#ifdef CONFIG_XEN_SHSTK
mov $1, %ecx
rdsspd %ecx
cmp $1, %ecx
- je .L\@_shstk_done
+ je .L\x\@_shstk_done
mov $64, %ecx /* 64 * 4 bytes, given incsspd */
incsspd %ecx /* Restore old SSP */
-.L\@_shstk_done:
+.L\x\@_shstk_done:
#endif
.endm
--
2.30.2
On 17.02.2023 13:22, Andrew Cooper wrote:
> https://github.com/llvm/llvm-project/issues/60792
>
> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
> unit, and the XSA-426 change tickles this bug:
>
> <instantiation>:4:1: error: invalid symbol redefinition
> .L1_fill_rsb_loop:
> ^
> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>
> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
> too, which Clang does seem to expand properly.
>
> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Wei Liu <wl@xen.org>
>
> v1 (vs RFC):
> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>
> I originally tried a parameter named uniq but this found a different Clang-IAS
> bug:
>
> In file included from arch/x86/asm-macros.c:3:
> ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
> .L\@\uniq\()fill_rsb_loop:
> ^
>
> It appears that Clang is looking for unicode escapes before completing
> parameter substitution. But the macro didn't originate from a context where a
> unicode escape was even applicable to begin with.
>
> The consequence is that you can't use macro prameters beginning with a u.
How nice. If really needed, I guess we could use -Wno-unicode on the
assumption that we don't use anything that could legitimately trigger that
warning.
> --- a/xen/arch/x86/include/asm/spec_ctrl.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>
> /* (ab)use alternative_input() to specify clobbers. */
> - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
> : "rax", "rcx");
> }
>
> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
> *
> * (ab)use alternative_input() to specify clobbers.
> */
> - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
> : "rax", "rcx");
> }
Since inside the macro you retain the uses of \@, I'd find it desirable
to keep gcc-generated code tidy by omitting the extra argument there.
This could be done by introducing another C macro along the lines of:
#ifdef __clang__
#define CLANG_UNIQUE(name) " " #name "=%="
#else
#define CLANG_UNIQUE(name)
#endif
Besides the less confusing label names this would also have the benefit
of providing a link at the use sites to what the issue is that is being
worked around. Plus, once (if ever) fixed in Clang, we could then adjust
the condition to just the affected versions.
> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
> @@ -117,11 +117,15 @@
> .L\@_done:
> .endm
>
> -.macro DO_OVERWRITE_RSB tmp=rax
> +.macro DO_OVERWRITE_RSB tmp=rax x=
The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
raising the question "Why is this there?" Furthermore I think it would
be good practice if macro parameters were comma-separated. I realize we
don't have this anyway near consistent right now, but still.
> /*
> * Requires nothing
> * Clobbers \tmp (%rax by default), %rcx
> *
> + * x= is an optional parameter to work around
> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
> + * expand \@ uniquely, and is intended for muxing %= in too.
With the above suggestion I'd see this comment move to next to that
helper macro, with a link to there left here.
Just to clarify: I'm not going to insist on any of the suggested
adjustments, but personally I think they would be beneficial. If you
are pretty certain the other way around, please let me know, and I'll
consider ack-ing (largely) as-is.
Jan
On 21/02/2023 10:31 am, Jan Beulich wrote:
> On 17.02.2023 13:22, Andrew Cooper wrote:
>> https://github.com/llvm/llvm-project/issues/60792
>>
>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>> unit, and the XSA-426 change tickles this bug:
>>
>> <instantiation>:4:1: error: invalid symbol redefinition
>> .L1_fill_rsb_loop:
>> ^
>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>
>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
>> too, which Clang does seem to expand properly.
>>
>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Wei Liu <wl@xen.org>
>>
>> v1 (vs RFC):
>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
Sadly, this is not quite correct. There needs to be a non-numeric
character separating \@ and \x or the concatenation of the two can end
up non-unique. So I think we need the \().
>> I originally tried a parameter named uniq but this found a different Clang-IAS
>> bug:
>>
>> In file included from arch/x86/asm-macros.c:3:
>> ./arch/x86/include/asm/spec_ctrl_asm.h:139:5: error: \u used with no following hex digits; treating as '\' followed by identifier [-Werror,-Wunicode]
>> .L\@\uniq\()fill_rsb_loop:
>> ^
>>
>> It appears that Clang is looking for unicode escapes before completing
>> parameter substitution. But the macro didn't originate from a context where a
>> unicode escape was even applicable to begin with.
>>
>> The consequence is that you can't use macro prameters beginning with a u.
> How nice. If really needed, I guess we could use -Wno-unicode on the
> assumption that we don't use anything that could legitimately trigger that
> warning.
It's proving very stubborn to narrow down.
I really can't reproduce it outside of this specific occurrence in the
Xen build system, but my gut feeling is that it's something to do with
the asm level .include.
>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
>> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>
>> /* (ab)use alternative_input() to specify clobbers. */
>> - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>> : "rax", "rcx");
>> }
>>
>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
>> *
>> * (ab)use alternative_input() to specify clobbers.
>> */
>> - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>> : "rax", "rcx");
>> }
> Since inside the macro you retain the uses of \@, I'd find it desirable
> to keep gcc-generated code tidy by omitting the extra argument there.
As I said in the RFC, I tried to have x=\@ but gas really didn't like that.
But given the concatenation observation, we also cannot simply replace
\@ with %= (given the option, which I couldn't figure out), because they
can overlap.
> This could be done by introducing another C macro along the lines of:
>
> #ifdef __clang__
> #define CLANG_UNIQUE(name) " " #name "=%="
> #else
> #define CLANG_UNIQUE(name)
> #endif
>
> Besides the less confusing label names this would also have the benefit
> of providing a link at the use sites to what the issue is that is being
> worked around. Plus, once (if ever) fixed in Clang, we could then adjust
> the condition to just the affected versions.
It's only Clang IAS, but there's no suitable define to figure this out.
And while I appreciate the effort to be more descriptive, this name is
literally longer than the thing it wraps and ...
>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>> @@ -117,11 +117,15 @@
>> .L\@_done:
>> .endm
>>
>> -.macro DO_OVERWRITE_RSB tmp=rax
>> +.macro DO_OVERWRITE_RSB tmp=rax x=
> The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
> raising the question "Why is this there?"
Because I started out with u=\@
>> /*
>> * Requires nothing
>> * Clobbers \tmp (%rax by default), %rcx
>> *
>> + * x= is an optional parameter to work around
>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
>> + * expand \@ uniquely, and is intended for muxing %= in too.
> With the above suggestion I'd see this comment move to next to that
> helper macro, with a link to there left here.
... there's no getting rid of this comment, at least in some form. The
parameter is odd, and needs explaining.
Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this
case, attempts to "declutter" the niche usecase of inspecting the
assembled file comes with a substantial complexity at the C level.
It's really not worth the extra complexity.
~Andrew
On 21.02.2023 13:26, Andrew Cooper wrote:
> On 21/02/2023 10:31 am, Jan Beulich wrote:
>> On 17.02.2023 13:22, Andrew Cooper wrote:
>>> https://github.com/llvm/llvm-project/issues/60792
>>>
>>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>>> unit, and the XSA-426 change tickles this bug:
>>>
>>> <instantiation>:4:1: error: invalid symbol redefinition
>>> .L1_fill_rsb_loop:
>>> ^
>>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>>
>>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
>>> too, which Clang does seem to expand properly.
>>>
>>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Wei Liu <wl@xen.org>
>>>
>>> v1 (vs RFC):
>>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>
> Sadly, this is not quite correct. There needs to be a non-numeric
> character separating \@ and \x or the concatenation of the two can end
> up non-unique.
How that if \@ is always 1?
> So I think we need the \().
Or put one at the start of the label and the other at the end?
>>> --- a/xen/arch/x86/include/asm/spec_ctrl.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl.h
>>> @@ -83,7 +83,7 @@ static always_inline void spec_ctrl_new_guest_context(void)
>>> wrmsrl(MSR_PRED_CMD, PRED_CMD_IBPB);
>>>
>>> /* (ab)use alternative_input() to specify clobbers. */
>>> - alternative_input("", "DO_OVERWRITE_RSB", X86_BUG_IBPB_NO_RET,
>>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_BUG_IBPB_NO_RET,
>>> : "rax", "rcx");
>>> }
>>>
>>> @@ -172,7 +172,7 @@ static always_inline void spec_ctrl_enter_idle(struct cpu_info *info)
>>> *
>>> * (ab)use alternative_input() to specify clobbers.
>>> */
>>> - alternative_input("", "DO_OVERWRITE_RSB", X86_FEATURE_SC_RSB_IDLE,
>>> + alternative_input("", "DO_OVERWRITE_RSB x=%=", X86_FEATURE_SC_RSB_IDLE,
>>> : "rax", "rcx");
>>> }
>> Since inside the macro you retain the uses of \@, I'd find it desirable
>> to keep gcc-generated code tidy by omitting the extra argument there.
>
> As I said in the RFC, I tried to have x=\@ but gas really didn't like that.
>
> But given the concatenation observation, we also cannot simply replace
> \@ with %= (given the option, which I couldn't figure out), because they
> can overlap.
>
>> This could be done by introducing another C macro along the lines of:
>>
>> #ifdef __clang__
>> #define CLANG_UNIQUE(name) " " #name "=%="
>> #else
>> #define CLANG_UNIQUE(name)
>> #endif
>>
>> Besides the less confusing label names this would also have the benefit
>> of providing a link at the use sites to what the issue is that is being
>> worked around. Plus, once (if ever) fixed in Clang, we could then adjust
>> the condition to just the affected versions.
>
> It's only Clang IAS, but there's no suitable define to figure this out.
"this" being what? The case of Clang but with / without integrated as?
Since we have to turn that off, we could as well add a -D option along
with the -no-integrated-as one.
> And while I appreciate the effort to be more descriptive, this name is
> literally longer than the thing it wraps and ...
>
>>> --- a/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> +++ b/xen/arch/x86/include/asm/spec_ctrl_asm.h
>>> @@ -117,11 +117,15 @@
>>> .L\@_done:
>>> .endm
>>>
>>> -.macro DO_OVERWRITE_RSB tmp=rax
>>> +.macro DO_OVERWRITE_RSB tmp=rax x=
>> The "=" in "x=" isn't needed, is it? It looks a little confusing to me,
>> raising the question "Why is this there?"
>
> Because I started out with u=\@
>
>>> /*
>>> * Requires nothing
>>> * Clobbers \tmp (%rax by default), %rcx
>>> *
>>> + * x= is an optional parameter to work around
>>> + * https://github.com/llvm/llvm-project/issues/60792 where Clang-IAS doesn't
>>> + * expand \@ uniquely, and is intended for muxing %= in too.
>> With the above suggestion I'd see this comment move to next to that
>> helper macro, with a link to there left here.
>
> ... there's no getting rid of this comment, at least in some form. The
> parameter is odd, and needs explaining.
Of course, hence my "with a link to there left here".
> Passing %= unconditionally doesn't matter for GCC/Binuitls, and in this
> case, attempts to "declutter" the niche usecase of inspecting the
> assembled file comes with a substantial complexity at the C level.
> It's really not worth the extra complexity.
I wouldn't call that one simple macro "complexity". I'm in particular
not advocating for (conditionally) removing the extra macro parameter.
Jan
On 21/02/2023 12:46 pm, Jan Beulich wrote:
> On 21.02.2023 13:26, Andrew Cooper wrote:
>> On 21/02/2023 10:31 am, Jan Beulich wrote:
>>> On 17.02.2023 13:22, Andrew Cooper wrote:
>>>> https://github.com/llvm/llvm-project/issues/60792
>>>>
>>>> It turns out that Clang-IAS does not expand \@ uniquely in a translaition
>>>> unit, and the XSA-426 change tickles this bug:
>>>>
>>>> <instantiation>:4:1: error: invalid symbol redefinition
>>>> .L1_fill_rsb_loop:
>>>> ^
>>>> make[3]: *** [Rules.mk:247: arch/x86/acpi/cpu_idle.o] Error 1
>>>>
>>>> Extend DO_OVERWRITE_RSB with an optional parameter so C callers can mux %= in
>>>> too, which Clang does seem to expand properly.
>>>>
>>>> Fixes: 63305e5392ec ("x86/spec-ctrl: Mitigate Cross-Thread Return Address Predictions")
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>>> ---
>>>> CC: Jan Beulich <JBeulich@suse.com>
>>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>>> CC: Wei Liu <wl@xen.org>
>>>>
>>>> v1 (vs RFC):
>>>> * Rename \foo to \x, reorder WRT \@ to avoid needing \() too.
>> Sadly, this is not quite correct. There needs to be a non-numeric
>> character separating \@ and \x or the concatenation of the two can end
>> up non-unique.
> How that if \@ is always 1?
It isn't always 1.
Global (file scope) have \@ expand properly from 0 to N.
Function scope have \@ expand properly from 0 to N in a single asm()
statement.
The 1 here is actually because mknops took the 0 and didn't use it. If
instead we had something like:
asm ("DO_OVERWRITE_RSB"); // \@ = 0
asm ("mkops 2;"
"DO_OVERWRITE_RSB"); // \@ = 1
then it would assemble fine, but the way we build alternatives in asm()
statements reliably causes the alternative block to consume \@=1.
>> So I think we need the \().
> Or put one at the start of the label and the other at the end?
We already played that trick with \n for the .irp, which suffers
similarly with concatenation.
~Andrew
© 2016 - 2024 Red Hat, Inc.