* [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO
@ 2019-04-24 16:55 Kees Cook
2019-04-24 17:27 ` Nick Desaulniers
` (2 more replies)
0 siblings, 3 replies; 4+ messages in thread
From: Kees Cook @ 2019-04-24 16:55 UTC (permalink / raw)
To: Catalin Marinas
Cc: Will Deacon, Mark Rutland, Arnd Bergmann, Alex Matveev,
Ard Biesheuvel, Nick Desaulniers, Yury Norov, Matthias Kaehlcke,
Sami Tolvanen, linux-arm-kernel, linux-kernel
Clang's integrated assembler does not allow assembly macros defined
in one inline asm block using the .macro directive to be used across
separate asm blocks. LLVM developers consider this a feature and not a
bug, recommending code refactoring:
https://bugs.llvm.org/show_bug.cgi?id=19749
As binutils doesn't allow macros to be redefined, this change uses
UNDEFINE_MRS_S and UNDEFINE_MSR_S to define corresponding macros
in-place and workaround gcc and clang limitations on redefining macros
across different assembler blocks.
Specifically, the current state after preprocessing looks like this:
asm volatile(".macro mXX_s ... .endm");
void f()
{
asm volatile("mXX_s a, b");
}
With GCC, it gives macro redefinition error because sysreg.h is included
in multiple source files, and assembler code for all of them is later
combined for LTO (I've seen an intermediate file with hundreds of
identical definitions).
With clang, it gives macro undefined error because clang doesn't allow
sharing macros between inline asm statements.
I also seem to remember catching another sort of undefined error with
GCC due to reordering of macro definition asm statement and generated
asm code for function that uses the macro.
The solution with defining and undefining for each use, while certainly
not elegant, satisfies both GCC and clang, LTO and non-LTO.
Co-developed-by: Alex Matveev <alxmtvv@gmail.com>
Co-developed-by: Yury Norov <ynorov@caviumnetworks.com>
Co-developed-by: Sami Tolvanen <samitolvanen@google.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
v5: include register declaration in macro (rutland)
v4: move to using preprocessor entirely, split constraints for "rZ" case.
v3: added more uses in irqflags, updated commit log, based on
discussion in https://lore.kernel.org/patchwork/patch/851580/
---
arch/arm64/include/asm/irqflags.h | 8 +++---
arch/arm64/include/asm/kvm_hyp.h | 4 +--
arch/arm64/include/asm/sysreg.h | 45 ++++++++++++++++++++++---------
3 files changed, 38 insertions(+), 19 deletions(-)
diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
index 43d8366c1e87..629963189085 100644
--- a/arch/arm64/include/asm/irqflags.h
+++ b/arch/arm64/include/asm/irqflags.h
@@ -43,7 +43,7 @@ static inline void arch_local_irq_enable(void)
asm volatile(ALTERNATIVE(
"msr daifclr, #2 // arch_local_irq_enable\n"
"nop",
- "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
+ __msr_s(SYS_ICC_PMR_EL1, "%0")
"dsb sy",
ARM64_HAS_IRQ_PRIO_MASKING)
:
@@ -55,7 +55,7 @@ static inline void arch_local_irq_disable(void)
{
asm volatile(ALTERNATIVE(
"msr daifset, #2 // arch_local_irq_disable",
- "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0",
+ __msr_s(SYS_ICC_PMR_EL1, "%0"),
ARM64_HAS_IRQ_PRIO_MASKING)
:
: "r" ((unsigned long) GIC_PRIO_IRQOFF)
@@ -86,7 +86,7 @@ static inline unsigned long arch_local_save_flags(void)
"mov %0, %1\n"
"nop\n"
"nop",
- "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1) "\n"
+ __mrs_s("%0", SYS_ICC_PMR_EL1)
"ands %1, %1, " __stringify(PSR_I_BIT) "\n"
"csel %0, %0, %2, eq",
ARM64_HAS_IRQ_PRIO_MASKING)
@@ -116,7 +116,7 @@ static inline void arch_local_irq_restore(unsigned long flags)
asm volatile(ALTERNATIVE(
"msr daif, %0\n"
"nop",
- "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n"
+ __msr_s(SYS_ICC_PMR_EL1, "%0")
"dsb sy",
ARM64_HAS_IRQ_PRIO_MASKING)
: "+r" (flags)
diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 4da765f2cca5..c3060833b7a5 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -30,7 +30,7 @@
({ \
u64 reg; \
asm volatile(ALTERNATIVE("mrs %0, " __stringify(r##nvh),\
- "mrs_s %0, " __stringify(r##vh),\
+ __mrs_s("%0", r##vh), \
ARM64_HAS_VIRT_HOST_EXTN) \
: "=r" (reg)); \
reg; \
@@ -40,7 +40,7 @@
do { \
u64 __val = (u64)(v); \
asm volatile(ALTERNATIVE("msr " __stringify(r##nvh) ", %x0",\
- "msr_s " __stringify(r##vh) ", %x0",\
+ __msr_s(r##vh, "%x0"), \
ARM64_HAS_VIRT_HOST_EXTN) \
: : "rZ" (__val)); \
} while (0)
diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
index 5b267dec6194..3b2c09eddd8f 100644
--- a/arch/arm64/include/asm/sysreg.h
+++ b/arch/arm64/include/asm/sysreg.h
@@ -746,20 +746,39 @@
#include <linux/build_bug.h>
#include <linux/types.h>
-asm(
-" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
-" .equ .L__reg_num_x\\num, \\num\n"
-" .endr\n"
+#define __DEFINE_MRS_MSR_S_REGNUM \
+" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n" \
+" .equ .L__reg_num_x\\num, \\num\n" \
+" .endr\n" \
" .equ .L__reg_num_xzr, 31\n"
-"\n"
-" .macro mrs_s, rt, sreg\n"
- __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt))
+
+#define DEFINE_MRS_S \
+ __DEFINE_MRS_MSR_S_REGNUM \
+" .macro mrs_s, rt, sreg\n" \
+ __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt)) \
" .endm\n"
-"\n"
-" .macro msr_s, sreg, rt\n"
- __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
+
+#define DEFINE_MSR_S \
+ __DEFINE_MRS_MSR_S_REGNUM \
+" .macro msr_s, sreg, rt\n" \
+ __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt)) \
" .endm\n"
-);
+
+#define UNDEFINE_MRS_S \
+" .purgem mrs_s\n"
+
+#define UNDEFINE_MSR_S \
+" .purgem msr_s\n"
+
+#define __mrs_s(v, r) \
+ DEFINE_MRS_S \
+" mrs_s " v ", " __stringify(r) "\n" \
+ UNDEFINE_MRS_S
+
+#define __msr_s(r, v) \
+ DEFINE_MSR_S \
+" msr_s " __stringify(r) ", " v "\n" \
+ UNDEFINE_MSR_S
/*
* Unlike read_cpuid, calls to read_sysreg are never expected to be
@@ -787,13 +806,13 @@ asm(
*/
#define read_sysreg_s(r) ({ \
u64 __val; \
- asm volatile("mrs_s %0, " __stringify(r) : "=r" (__val)); \
+ asm volatile(__mrs_s("%0", r) : "=r" (__val)); \
__val; \
})
#define write_sysreg_s(v, r) do { \
u64 __val = (u64)(v); \
- asm volatile("msr_s " __stringify(r) ", %x0" : : "rZ" (__val)); \
+ asm volatile(__msr_s(r, "%x0") : : "rZ" (__val)); \
} while (0)
/*
--
2.17.1
--
Kees Cook
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO
2019-04-24 16:55 [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO Kees Cook
@ 2019-04-24 17:27 ` Nick Desaulniers
2019-04-25 10:22 ` Mark Rutland
2019-04-25 14:00 ` Will Deacon
2 siblings, 0 replies; 4+ messages in thread
From: Nick Desaulniers @ 2019-04-24 17:27 UTC (permalink / raw)
To: Kees Cook
Cc: Catalin Marinas, Will Deacon, Mark Rutland, Arnd Bergmann,
Alex Matveev, Ard Biesheuvel, Yury Norov, Matthias Kaehlcke,
Sami Tolvanen, Linux ARM, LKML
On Wed, Apr 24, 2019 at 9:55 AM Kees Cook <keescook@chromium.org> wrote:
>
> Clang's integrated assembler does not allow assembly macros defined
> in one inline asm block using the .macro directive to be used across
> separate asm blocks. LLVM developers consider this a feature and not a
> bug, recommending code refactoring:
>
> https://bugs.llvm.org/show_bug.cgi?id=19749
>
> As binutils doesn't allow macros to be redefined, this change uses
> UNDEFINE_MRS_S and UNDEFINE_MSR_S to define corresponding macros
> in-place and workaround gcc and clang limitations on redefining macros
> across different assembler blocks.
>
> Specifically, the current state after preprocessing looks like this:
>
> asm volatile(".macro mXX_s ... .endm");
> void f()
> {
> asm volatile("mXX_s a, b");
> }
>
> With GCC, it gives macro redefinition error because sysreg.h is included
> in multiple source files, and assembler code for all of them is later
> combined for LTO (I've seen an intermediate file with hundreds of
> identical definitions).
>
> With clang, it gives macro undefined error because clang doesn't allow
> sharing macros between inline asm statements.
>
> I also seem to remember catching another sort of undefined error with
> GCC due to reordering of macro definition asm statement and generated
> asm code for function that uses the macro.
>
> The solution with defining and undefining for each use, while certainly
> not elegant, satisfies both GCC and clang, LTO and non-LTO.
>
> Co-developed-by: Alex Matveev <alxmtvv@gmail.com>
> Co-developed-by: Yury Norov <ynorov@caviumnetworks.com>
> Co-developed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v5: include register declaration in macro (rutland)
Great! Thanks for following up on the feedback. Happy to see the way
this turned out.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> v4: move to using preprocessor entirely, split constraints for "rZ" case.
> v3: added more uses in irqflags, updated commit log, based on
> discussion in https://lore.kernel.org/patchwork/patch/851580/
> ---
> arch/arm64/include/asm/irqflags.h | 8 +++---
> arch/arm64/include/asm/kvm_hyp.h | 4 +--
> arch/arm64/include/asm/sysreg.h | 45 ++++++++++++++++++++++---------
> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 43d8366c1e87..629963189085 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -43,7 +43,7 @@ static inline void arch_local_irq_enable(void)
> asm volatile(ALTERNATIVE(
> "msr daifclr, #2 // arch_local_irq_enable\n"
> "nop",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
> + __msr_s(SYS_ICC_PMR_EL1, "%0")
> "dsb sy",
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> @@ -55,7 +55,7 @@ static inline void arch_local_irq_disable(void)
> {
> asm volatile(ALTERNATIVE(
> "msr daifset, #2 // arch_local_irq_disable",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0",
> + __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> : "r" ((unsigned long) GIC_PRIO_IRQOFF)
> @@ -86,7 +86,7 @@ static inline unsigned long arch_local_save_flags(void)
> "mov %0, %1\n"
> "nop\n"
> "nop",
> - "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1) "\n"
> + __mrs_s("%0", SYS_ICC_PMR_EL1)
> "ands %1, %1, " __stringify(PSR_I_BIT) "\n"
> "csel %0, %0, %2, eq",
> ARM64_HAS_IRQ_PRIO_MASKING)
> @@ -116,7 +116,7 @@ static inline void arch_local_irq_restore(unsigned long flags)
> asm volatile(ALTERNATIVE(
> "msr daif, %0\n"
> "nop",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n"
> + __msr_s(SYS_ICC_PMR_EL1, "%0")
> "dsb sy",
> ARM64_HAS_IRQ_PRIO_MASKING)
> : "+r" (flags)
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 4da765f2cca5..c3060833b7a5 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -30,7 +30,7 @@
> ({ \
> u64 reg; \
> asm volatile(ALTERNATIVE("mrs %0, " __stringify(r##nvh),\
> - "mrs_s %0, " __stringify(r##vh),\
> + __mrs_s("%0", r##vh), \
> ARM64_HAS_VIRT_HOST_EXTN) \
> : "=r" (reg)); \
> reg; \
> @@ -40,7 +40,7 @@
> do { \
> u64 __val = (u64)(v); \
> asm volatile(ALTERNATIVE("msr " __stringify(r##nvh) ", %x0",\
> - "msr_s " __stringify(r##vh) ", %x0",\
> + __msr_s(r##vh, "%x0"), \
> ARM64_HAS_VIRT_HOST_EXTN) \
> : : "rZ" (__val)); \
> } while (0)
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 5b267dec6194..3b2c09eddd8f 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -746,20 +746,39 @@
> #include <linux/build_bug.h>
> #include <linux/types.h>
>
> -asm(
> -" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> -" .equ .L__reg_num_x\\num, \\num\n"
> -" .endr\n"
> +#define __DEFINE_MRS_MSR_S_REGNUM \
> +" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n" \
> +" .equ .L__reg_num_x\\num, \\num\n" \
> +" .endr\n" \
> " .equ .L__reg_num_xzr, 31\n"
> -"\n"
> -" .macro mrs_s, rt, sreg\n"
> - __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt))
> +
> +#define DEFINE_MRS_S \
> + __DEFINE_MRS_MSR_S_REGNUM \
> +" .macro mrs_s, rt, sreg\n" \
> + __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt)) \
> " .endm\n"
> -"\n"
> -" .macro msr_s, sreg, rt\n"
> - __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
> +
> +#define DEFINE_MSR_S \
> + __DEFINE_MRS_MSR_S_REGNUM \
> +" .macro msr_s, sreg, rt\n" \
> + __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt)) \
> " .endm\n"
> -);
> +
> +#define UNDEFINE_MRS_S \
> +" .purgem mrs_s\n"
> +
> +#define UNDEFINE_MSR_S \
> +" .purgem msr_s\n"
> +
> +#define __mrs_s(v, r) \
> + DEFINE_MRS_S \
> +" mrs_s " v ", " __stringify(r) "\n" \
> + UNDEFINE_MRS_S
> +
> +#define __msr_s(r, v) \
> + DEFINE_MSR_S \
> +" msr_s " __stringify(r) ", " v "\n" \
> + UNDEFINE_MSR_S
>
> /*
> * Unlike read_cpuid, calls to read_sysreg are never expected to be
> @@ -787,13 +806,13 @@ asm(
> */
> #define read_sysreg_s(r) ({ \
> u64 __val; \
> - asm volatile("mrs_s %0, " __stringify(r) : "=r" (__val)); \
> + asm volatile(__mrs_s("%0", r) : "=r" (__val)); \
> __val; \
> })
>
> #define write_sysreg_s(v, r) do { \
> u64 __val = (u64)(v); \
> - asm volatile("msr_s " __stringify(r) ", %x0" : : "rZ" (__val)); \
> + asm volatile(__msr_s(r, "%x0") : : "rZ" (__val)); \
> } while (0)
>
> /*
> --
> 2.17.1
>
>
> --
> Kees Cook
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO
2019-04-24 16:55 [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO Kees Cook
2019-04-24 17:27 ` Nick Desaulniers
@ 2019-04-25 10:22 ` Mark Rutland
2019-04-25 14:00 ` Will Deacon
2 siblings, 0 replies; 4+ messages in thread
From: Mark Rutland @ 2019-04-25 10:22 UTC (permalink / raw)
To: Kees Cook
Cc: Catalin Marinas, Will Deacon, Arnd Bergmann, Alex Matveev,
Ard Biesheuvel, Nick Desaulniers, Yury Norov, Matthias Kaehlcke,
Sami Tolvanen, linux-arm-kernel, linux-kernel
On Wed, Apr 24, 2019 at 09:55:37AM -0700, Kees Cook wrote:
> Clang's integrated assembler does not allow assembly macros defined
> in one inline asm block using the .macro directive to be used across
> separate asm blocks. LLVM developers consider this a feature and not a
> bug, recommending code refactoring:
>
> https://bugs.llvm.org/show_bug.cgi?id=19749
>
> As binutils doesn't allow macros to be redefined, this change uses
> UNDEFINE_MRS_S and UNDEFINE_MSR_S to define corresponding macros
> in-place and workaround gcc and clang limitations on redefining macros
> across different assembler blocks.
>
> Specifically, the current state after preprocessing looks like this:
>
> asm volatile(".macro mXX_s ... .endm");
> void f()
> {
> asm volatile("mXX_s a, b");
> }
>
> With GCC, it gives macro redefinition error because sysreg.h is included
> in multiple source files, and assembler code for all of them is later
> combined for LTO (I've seen an intermediate file with hundreds of
> identical definitions).
>
> With clang, it gives macro undefined error because clang doesn't allow
> sharing macros between inline asm statements.
>
> I also seem to remember catching another sort of undefined error with
> GCC due to reordering of macro definition asm statement and generated
> asm code for function that uses the macro.
>
> The solution with defining and undefining for each use, while certainly
> not elegant, satisfies both GCC and clang, LTO and non-LTO.
>
> Co-developed-by: Alex Matveev <alxmtvv@gmail.com>
> Co-developed-by: Yury Norov <ynorov@caviumnetworks.com>
> Co-developed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
I've given this a spin with the kernel.org crosstool GCC 8.1.0
toolchain, and I can confirm defconfig compiels cleanly and works as
expected. AFAICT, all usage in inline assembly has been updated, and the
removal of explicit stringification makes usage a bit easier to read,
which is a nice bonus!
I don't think we can make this any cleaner short of not having to
manually assemble the instructions, so FWIW:
Reviewed-by: Mark Rutland <mark.rutland@arm.com>
I'll leave this to Catalin and Will.
Thanks,
Mark.
> ---
> v5: include register declaration in macro (rutland)
> v4: move to using preprocessor entirely, split constraints for "rZ" case.
> v3: added more uses in irqflags, updated commit log, based on
> discussion in https://lore.kernel.org/patchwork/patch/851580/
> ---
> arch/arm64/include/asm/irqflags.h | 8 +++---
> arch/arm64/include/asm/kvm_hyp.h | 4 +--
> arch/arm64/include/asm/sysreg.h | 45 ++++++++++++++++++++++---------
> 3 files changed, 38 insertions(+), 19 deletions(-)
>
> diff --git a/arch/arm64/include/asm/irqflags.h b/arch/arm64/include/asm/irqflags.h
> index 43d8366c1e87..629963189085 100644
> --- a/arch/arm64/include/asm/irqflags.h
> +++ b/arch/arm64/include/asm/irqflags.h
> @@ -43,7 +43,7 @@ static inline void arch_local_irq_enable(void)
> asm volatile(ALTERNATIVE(
> "msr daifclr, #2 // arch_local_irq_enable\n"
> "nop",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ",%0\n"
> + __msr_s(SYS_ICC_PMR_EL1, "%0")
> "dsb sy",
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> @@ -55,7 +55,7 @@ static inline void arch_local_irq_disable(void)
> {
> asm volatile(ALTERNATIVE(
> "msr daifset, #2 // arch_local_irq_disable",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0",
> + __msr_s(SYS_ICC_PMR_EL1, "%0"),
> ARM64_HAS_IRQ_PRIO_MASKING)
> :
> : "r" ((unsigned long) GIC_PRIO_IRQOFF)
> @@ -86,7 +86,7 @@ static inline unsigned long arch_local_save_flags(void)
> "mov %0, %1\n"
> "nop\n"
> "nop",
> - "mrs_s %0, " __stringify(SYS_ICC_PMR_EL1) "\n"
> + __mrs_s("%0", SYS_ICC_PMR_EL1)
> "ands %1, %1, " __stringify(PSR_I_BIT) "\n"
> "csel %0, %0, %2, eq",
> ARM64_HAS_IRQ_PRIO_MASKING)
> @@ -116,7 +116,7 @@ static inline void arch_local_irq_restore(unsigned long flags)
> asm volatile(ALTERNATIVE(
> "msr daif, %0\n"
> "nop",
> - "msr_s " __stringify(SYS_ICC_PMR_EL1) ", %0\n"
> + __msr_s(SYS_ICC_PMR_EL1, "%0")
> "dsb sy",
> ARM64_HAS_IRQ_PRIO_MASKING)
> : "+r" (flags)
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 4da765f2cca5..c3060833b7a5 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -30,7 +30,7 @@
> ({ \
> u64 reg; \
> asm volatile(ALTERNATIVE("mrs %0, " __stringify(r##nvh),\
> - "mrs_s %0, " __stringify(r##vh),\
> + __mrs_s("%0", r##vh), \
> ARM64_HAS_VIRT_HOST_EXTN) \
> : "=r" (reg)); \
> reg; \
> @@ -40,7 +40,7 @@
> do { \
> u64 __val = (u64)(v); \
> asm volatile(ALTERNATIVE("msr " __stringify(r##nvh) ", %x0",\
> - "msr_s " __stringify(r##vh) ", %x0",\
> + __msr_s(r##vh, "%x0"), \
> ARM64_HAS_VIRT_HOST_EXTN) \
> : : "rZ" (__val)); \
> } while (0)
> diff --git a/arch/arm64/include/asm/sysreg.h b/arch/arm64/include/asm/sysreg.h
> index 5b267dec6194..3b2c09eddd8f 100644
> --- a/arch/arm64/include/asm/sysreg.h
> +++ b/arch/arm64/include/asm/sysreg.h
> @@ -746,20 +746,39 @@
> #include <linux/build_bug.h>
> #include <linux/types.h>
>
> -asm(
> -" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n"
> -" .equ .L__reg_num_x\\num, \\num\n"
> -" .endr\n"
> +#define __DEFINE_MRS_MSR_S_REGNUM \
> +" .irp num,0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30\n" \
> +" .equ .L__reg_num_x\\num, \\num\n" \
> +" .endr\n" \
> " .equ .L__reg_num_xzr, 31\n"
> -"\n"
> -" .macro mrs_s, rt, sreg\n"
> - __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt))
> +
> +#define DEFINE_MRS_S \
> + __DEFINE_MRS_MSR_S_REGNUM \
> +" .macro mrs_s, rt, sreg\n" \
> + __emit_inst(0xd5200000|(\\sreg)|(.L__reg_num_\\rt)) \
> " .endm\n"
> -"\n"
> -" .macro msr_s, sreg, rt\n"
> - __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt))
> +
> +#define DEFINE_MSR_S \
> + __DEFINE_MRS_MSR_S_REGNUM \
> +" .macro msr_s, sreg, rt\n" \
> + __emit_inst(0xd5000000|(\\sreg)|(.L__reg_num_\\rt)) \
> " .endm\n"
> -);
> +
> +#define UNDEFINE_MRS_S \
> +" .purgem mrs_s\n"
> +
> +#define UNDEFINE_MSR_S \
> +" .purgem msr_s\n"
> +
> +#define __mrs_s(v, r) \
> + DEFINE_MRS_S \
> +" mrs_s " v ", " __stringify(r) "\n" \
> + UNDEFINE_MRS_S
> +
> +#define __msr_s(r, v) \
> + DEFINE_MSR_S \
> +" msr_s " __stringify(r) ", " v "\n" \
> + UNDEFINE_MSR_S
>
> /*
> * Unlike read_cpuid, calls to read_sysreg are never expected to be
> @@ -787,13 +806,13 @@ asm(
> */
> #define read_sysreg_s(r) ({ \
> u64 __val; \
> - asm volatile("mrs_s %0, " __stringify(r) : "=r" (__val)); \
> + asm volatile(__mrs_s("%0", r) : "=r" (__val)); \
> __val; \
> })
>
> #define write_sysreg_s(v, r) do { \
> u64 __val = (u64)(v); \
> - asm volatile("msr_s " __stringify(r) ", %x0" : : "rZ" (__val)); \
> + asm volatile(__msr_s(r, "%x0") : : "rZ" (__val)); \
> } while (0)
>
> /*
> --
> 2.17.1
>
>
> --
> Kees Cook
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO
2019-04-24 16:55 [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO Kees Cook
2019-04-24 17:27 ` Nick Desaulniers
2019-04-25 10:22 ` Mark Rutland
@ 2019-04-25 14:00 ` Will Deacon
2 siblings, 0 replies; 4+ messages in thread
From: Will Deacon @ 2019-04-25 14:00 UTC (permalink / raw)
To: Kees Cook
Cc: Catalin Marinas, Mark Rutland, Arnd Bergmann, Alex Matveev,
Ard Biesheuvel, Nick Desaulniers, Yury Norov, Matthias Kaehlcke,
Sami Tolvanen, linux-arm-kernel, linux-kernel
On Wed, Apr 24, 2019 at 09:55:37AM -0700, Kees Cook wrote:
> Clang's integrated assembler does not allow assembly macros defined
> in one inline asm block using the .macro directive to be used across
> separate asm blocks. LLVM developers consider this a feature and not a
> bug, recommending code refactoring:
>
> https://bugs.llvm.org/show_bug.cgi?id=19749
>
> As binutils doesn't allow macros to be redefined, this change uses
> UNDEFINE_MRS_S and UNDEFINE_MSR_S to define corresponding macros
> in-place and workaround gcc and clang limitations on redefining macros
> across different assembler blocks.
>
> Specifically, the current state after preprocessing looks like this:
>
> asm volatile(".macro mXX_s ... .endm");
> void f()
> {
> asm volatile("mXX_s a, b");
> }
>
> With GCC, it gives macro redefinition error because sysreg.h is included
> in multiple source files, and assembler code for all of them is later
> combined for LTO (I've seen an intermediate file with hundreds of
> identical definitions).
>
> With clang, it gives macro undefined error because clang doesn't allow
> sharing macros between inline asm statements.
>
> I also seem to remember catching another sort of undefined error with
> GCC due to reordering of macro definition asm statement and generated
> asm code for function that uses the macro.
>
> The solution with defining and undefining for each use, while certainly
> not elegant, satisfies both GCC and clang, LTO and non-LTO.
>
> Co-developed-by: Alex Matveev <alxmtvv@gmail.com>
> Co-developed-by: Yury Norov <ynorov@caviumnetworks.com>
> Co-developed-by: Sami Tolvanen <samitolvanen@google.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
> v5: include register declaration in macro (rutland)
Cheers all, I've picked this up for 5.2 with the two Reviewed-by tags.
Will
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-04-25 14:00 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-24 16:55 [PATCH v5] arm64: sysreg: Make mrs_s and msr_s macros work with Clang and LTO Kees Cook
2019-04-24 17:27 ` Nick Desaulniers
2019-04-25 10:22 ` Mark Rutland
2019-04-25 14:00 ` Will Deacon
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).