riscv: Move alternative length validation into subsection
PATCH 1/1 riscv: Move alternative length validation into subsection
After commit 49b290e430d3 ("riscv: prevent compressed instructions in alternatives"), builds with LLVM's integrated assembler fail:
In file included from arch/riscv/mm/init.c:10:
In file included from ./include/linux/mm.h:29:
In file included from ./include/linux/pgtable.h:6:
In file included from ./arch/riscv/include/asm/pgtable.h:108:
./arch/riscv/include/asm/tlbflush.h:23:2: error: expected assembly-time absolute expression
ALT_FLUSH_TLB_PAGE(__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory"));
^
./arch/riscv/include/asm/errata_list.h:33:5: note: expanded from macro 'ALT_FLUSH_TLB_PAGE'
asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID, \
^
./arch/riscv/include/asm/alternative-macros.h:187:2: note: expanded from macro 'ALTERNATIVE'
_ALTERNATIVE_CFG(old_content, new_content, vendor_id, errata_id, CONFIG_k)
^
./arch/riscv/include/asm/alternative-macros.h:113:2: note: expanded from macro '_ALTERNATIVE_CFG'
__ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, IS_ENABLED(CONFIG_k))
^
./arch/riscv/include/asm/alternative-macros.h:110:2: note: expanded from macro '__ALTERNATIVE_CFG'
ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c)
^
./arch/riscv/include/asm/alternative-macros.h:99:3: note: expanded from macro 'ALT_NEW_CONTENT'
".org . - (889b - 888b) + (887b - 886b)\n" \
^
<inline asm>:26:6: note: instantiated into assembly here
.org . - (889b - 888b) + (887b - 886b)
^This error happens because LLVM's integrated assembler has a one-pass design, which means it cannot figure out the instruction lengths when the .org directive is outside of the subsection that contains the instructions, which was changed by the .option directives added by the above change.
Move the .org directives before the .previous directive so that these directives are always within the same subsection, which resolves the failures and does not introduce any new issues with GNU as. This was done for arm64 in commit 966a0acce2fc ("arm64/alternatives: move length validation inside the subsection") and commit 22315a2296f4 ("arm64:
alternatives: Move length validation in alternative_{insn, endif}").
While there is no error from the assembly versions of the macro, they appear to have the same problem so just make the same change there as well so that there are no problems in the future.
--- arch/riscv/include/asm/alternative-macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
arch/riscv/include/asm/alternative-macros.h +2 -2
@@ -27,9 +27,9 @@ \new_c .option pop 889 : - .previous .org . - (889b - 888b) + (887b - 886b) .org . - (887b - 886b) + (889b - 888b) + .previous .endif .endm
@@ -94,9 +94,9 @@ new_c"\n" \".option pop\n" \"889 :\n" \ -".previous\n" \".org . - (887b - 886b) + (889b - 888b)\n" \".org . - (889b - 888b) + (887b - 886b)\n" \ +".previous\n" \".endif\n" #define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable) \
base-commit: 93c0651617a62a69717299f1464dda798af8bebb
Signature · 1 line
2.36.1
Hi,
Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
Quoted 2 lines
After commit
49b290e430d3("riscv: prevent compressed instructions in alternatives"), builds with LLVM's integrated assembler fail:
the commit in question didn't change anything there, so I guess the issue itself was present before that already and the commit only triggered the different buildbots?
Quoted 39 lines
In file included from
arch/riscv/mm/init.c:10:
In file included from./include/linux/mm.h:29:
In file included from./include/linux/pgtable.h:6:
In file included from./arch/riscv/include/asm/pgtable.h:108:
./arch/riscv/include/asm/tlbflush.h:23:2: error: expected assembly-time absolute expressionALT_FLUSH_TLB_PAGE(__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory")); ^ ./arch/riscv/include/asm/errata_list.h:33:5: note: expanded from macro 'ALT_FLUSH_TLB_PAGE' asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID, \ ^ ./arch/riscv/include/asm/alternative-macros.h:187:2: note: expanded from macro 'ALTERNATIVE' _ALTERNATIVE_CFG(old_content, new_content, vendor_id, errata_id, CONFIG_k) ^ ./arch/riscv/include/asm/alternative-macros.h:113:2: note: expanded from macro '_ALTERNATIVE_CFG' __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, IS_ENABLED(CONFIG_k)) ^ ./arch/riscv/include/asm/alternative-macros.h:110:2: note: expanded from macro '__ALTERNATIVE_CFG' ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c) ^ ./arch/riscv/include/asm/alternative-macros.h:99:3: note: expanded from macro 'ALT_NEW_CONTENT' ".org . - (889b - 888b) + (887b - 886b)\n" \ ^ <inline asm>:26:6: note: instantiated into assembly here .org . - (889b - 888b) + (887b - 886b) ^This error happens because LLVM's integrated assembler has a one-pass design, which means it cannot figure out the instruction lengths when the .org directive is outside of the subsection that contains the instructions, which was changed by the .option directives added by the above change.
Move the .org directives before the .previous directive so that these directives are always within the same subsection, which resolves the failures and does not introduce any new issues with GNU as. This was done for arm64 in commit
966a0acce2fc("arm64/alternatives: move length validation inside the subsection") and commit22315a2296f4("arm64:
alternatives: Move length validation in alternative_{insn, endif}").
Quoted 7 lines
While there is no error from the assembly versions of the macro, they appear to have the same problem so just make the same change there as well so that there are no problems in the future.
In any case, on my svpbmt testcases (qemu + d1-nezha):
Thanks for looking into that
Heiko
Quoted 33 lines
--- arch/riscv/include/asm/alternative-macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/riscv/include/asm/alternative-macros.h +2
-2@@ -27,9 +27,9 @@ \new_c .option pop 889 : - .previous .org . - (889b - 888b) + (887b - 886b) .org . - (887b - 886b) + (889b - 888b) + .previous .endif .endm@@ -94,9 +94,9 @@ new_c"\n" \".option pop\n" \"889 :\n" \ -".previous\n" \".org . - (887b - 886b) + (889b - 888b)\n" \".org . - (889b - 888b) + (887b - 886b)\n" \ +".previous\n" \".endif\n" #define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable) \base-commit:
93c0651617a62a69717299f1464dda798af8bebb
Hi Heiko,
Quoted 9 lines · Heiko Stübner
Hi,
Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
Quoted 2 lines
After commit
49b290e430d3("riscv: prevent compressed instructions in alternatives"), builds with LLVM's integrated assembler fail:the commit in question didn't change anything there, so I guess the issue itself was present before that already and the commit only triggered the different buildbots?
Sort of. I am not super knowledgeable on the internals of LLVM's integrated assembler but as far as I understand it, the use of the '.option' directive creates a new fragment within this subsection, which messes up the integrated assembler's ability to resolve the differences between these labels outside of the subsection. Moving the directives into the subsection allows it to do this. So your patch did not really do anything wrong, it just exposed existing brittleness (hence why I did not add a Fixes tag).
Quoted 47 lines
Quoted 39 lines
In file included from
arch/riscv/mm/init.c:10:
In file included from./include/linux/mm.h:29:
In file included from./include/linux/pgtable.h:6:
In file included from./arch/riscv/include/asm/pgtable.h:108:
./arch/riscv/include/asm/tlbflush.h:23:2: error: expected assembly-time absolute expressionALT_FLUSH_TLB_PAGE(__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory")); ^ ./arch/riscv/include/asm/errata_list.h:33:5: note: expanded from macro 'ALT_FLUSH_TLB_PAGE' asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID, \ ^ ./arch/riscv/include/asm/alternative-macros.h:187:2: note: expanded from macro 'ALTERNATIVE' _ALTERNATIVE_CFG(old_content, new_content, vendor_id, errata_id, CONFIG_k) ^ ./arch/riscv/include/asm/alternative-macros.h:113:2: note: expanded from macro '_ALTERNATIVE_CFG' __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, IS_ENABLED(CONFIG_k)) ^ ./arch/riscv/include/asm/alternative-macros.h:110:2: note: expanded from macro '__ALTERNATIVE_CFG' ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c) ^ ./arch/riscv/include/asm/alternative-macros.h:99:3: note: expanded from macro 'ALT_NEW_CONTENT' ".org . - (889b - 888b) + (887b - 886b)\n" \ ^ <inline asm>:26:6: note: instantiated into assembly here .org . - (889b - 888b) + (887b - 886b) ^This error happens because LLVM's integrated assembler has a one-pass design, which means it cannot figure out the instruction lengths when the .org directive is outside of the subsection that contains the instructions, which was changed by the .option directives added by the above change.
Move the .org directives before the .previous directive so that these directives are always within the same subsection, which resolves the failures and does not introduce any new issues with GNU as. This was done for arm64 in commit
966a0acce2fc("arm64/alternatives: move length validation inside the subsection") and commit22315a2296f4("arm64:
alternatives: Move length validation in alternative_{insn, endif}").Quoted 7 lines
While there is no error from the assembly versions of the macro, they appear to have the same problem so just make the same change there as well so that there are no problems in the future.
Quoted 6 lines
In any case, on my svpbmt testcases (qemu + d1-nezha):
Tested-by Heiko Stuebner <[email protected]>Reviewed-by Heiko Stuebner <[email protected]>Thanks for looking into that
Heiko
Thank you a lot for the quick testing and review!
Cheers,
Nathan
Quoted 37 lines
Quoted 33 lines
--- arch/riscv/include/asm/alternative-macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/riscv/include/asm/alternative-macros.h +2
-2@@ -27,9 +27,9 @@ \new_c .option pop 889 : - .previous .org . - (889b - 888b) + (887b - 886b) .org . - (887b - 886b) + (889b - 888b) + .previous .endif .endm@@ -94,9 +94,9 @@ new_c"\n" \".option pop\n" \"889 :\n" \ -".previous\n" \".org . - (887b - 886b) + (889b - 888b)\n" \".org . - (889b - 888b) + (887b - 886b)\n" \ +".previous\n" \".endif\n" #define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable) \base-commit:
93c0651617a62a69717299f1464dda798af8bebb
Palmer Dabbelt
maintainerQuoted 80 lines · On Mon, 16 May 2022 14:45:21 PDT (-0700), [email protected] wrote:
After commit
49b290e430d3("riscv: prevent compressed instructions in alternatives"), builds with LLVM's integrated assembler fail:In file included from
arch/riscv/mm/init.c:10:
In file included from./include/linux/mm.h:29:
In file included from./include/linux/pgtable.h:6:
In file included from./arch/riscv/include/asm/pgtable.h:108:
./arch/riscv/include/asm/tlbflush.h:23:2: error: expected assembly-time absolute expressionALT_FLUSH_TLB_PAGE(__asm__ __volatile__ ("sfence.vma %0" : : "r" (addr) : "memory")); ^ ./arch/riscv/include/asm/errata_list.h:33:5: note: expanded from macro 'ALT_FLUSH_TLB_PAGE' asm(ALTERNATIVE("sfence.vma %0", "sfence.vma", SIFIVE_VENDOR_ID, \ ^ ./arch/riscv/include/asm/alternative-macros.h:187:2: note: expanded from macro 'ALTERNATIVE' _ALTERNATIVE_CFG(old_content, new_content, vendor_id, errata_id, CONFIG_k) ^ ./arch/riscv/include/asm/alternative-macros.h:113:2: note: expanded from macro '_ALTERNATIVE_CFG' __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, IS_ENABLED(CONFIG_k)) ^ ./arch/riscv/include/asm/alternative-macros.h:110:2: note: expanded from macro '__ALTERNATIVE_CFG' ALT_NEW_CONTENT(vendor_id, errata_id, enable, new_c) ^ ./arch/riscv/include/asm/alternative-macros.h:99:3: note: expanded from macro 'ALT_NEW_CONTENT' ".org . - (889b - 888b) + (887b - 886b)\n" \ ^ <inline asm>:26:6: note: instantiated into assembly here .org . - (889b - 888b) + (887b - 886b) ^This error happens because LLVM's integrated assembler has a one-pass design, which means it cannot figure out the instruction lengths when the .org directive is outside of the subsection that contains the instructions, which was changed by the .option directives added by the above change.
Move the .org directives before the .previous directive so that these directives are always within the same subsection, which resolves the failures and does not introduce any new issues with GNU as. This was done for arm64 in commit
966a0acce2fc("arm64/alternatives: move length validation inside the subsection") and commit22315a2296f4("arm64:
alternatives: Move length validation in alternative_{insn, endif}").While there is no error from the assembly versions of the macro, they appear to have the same problem so just make the same change there as well so that there are no problems in the future.
Reported-by kernel test robot <[email protected]>Signed-off-by Nathan Chancellor <[email protected]>--- arch/riscv/include/asm/alternative-macros.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)arch/riscv/include/asm/alternative-macros.h +2
-2@@ -27,9 +27,9 @@ \new_c .option pop 889 : - .previous .org . - (889b - 888b) + (887b - 886b) .org . - (887b - 886b) + (889b - 888b) + .previous .endif .endm@@ -94,9 +94,9 @@ new_c"\n" \".option pop\n" \"889 :\n" \ -".previous\n" \".org . - (887b - 886b) + (889b - 888b)\n" \".org . - (889b - 888b) + (887b - 886b)\n" \ +".previous\n" \".endif\n" #define __ALTERNATIVE_CFG(old_c, new_c, vendor_id, errata_id, enable) \
Quoted 1 line
base-commit:
93c0651617a62a69717299f1464dda798af8bebb
Thanks, this in on for-next (still for 5.19). I'm going to CC stable so it'll get backported, as it seems pretty harmless.