All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] riscv: Move alternative length validation into subsection
@ 2022-05-16 21:45 ` Nathan Chancellor
  0 siblings, 0 replies; 8+ messages in thread
From: Nathan Chancellor @ 2022-05-16 21:45 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou
  Cc: Nick Desaulniers, Heiko Stuebner, linux-riscv, linux-kernel,
	llvm, patches, Nathan Chancellor, kernel test robot

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.

Link: https://github.com/ClangBuiltLinux/linux/issues/1640
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 arch/riscv/include/asm/alternative-macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index e13b1f6bb400..ec2f3f1b836f 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -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
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 8+ messages in thread
* [PATCH] riscv: Move alternative length validation into subsection
@ 2022-05-16 21:45 ` Nathan Chancellor
  0 siblings, 0 replies; 8+ messages in thread
From: Nathan Chancellor @ 2022-05-16 21:45 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou
  Cc: Nick Desaulniers, Heiko Stuebner, linux-riscv, linux-kernel,
	llvm, patches, Nathan Chancellor, kernel test robot

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.

Link: https://github.com/ClangBuiltLinux/linux/issues/1640
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Nathan Chancellor <nathan@kernel.org>
---
 arch/riscv/include/asm/alternative-macros.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index e13b1f6bb400..ec2f3f1b836f 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -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
-- 
2.36.1


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply related	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
  2022-05-16 21:45 ` Nathan Chancellor
@ 2022-05-16 21:55   ` Heiko Stübner
  -1 siblings, 0 replies; 8+ messages in thread
From: Heiko Stübner @ 2022-05-16 21:55 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Nathan Chancellor
  Cc: Nick Desaulniers, linux-riscv, linux-kernel, llvm, patches,
	Nathan Chancellor, kernel test robot

Hi,

Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
> 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?


>   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.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

In any case, on my svpbmt testcases (qemu + d1-nezha):
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks for looking into that
Heiko


> ---
>  arch/riscv/include/asm/alternative-macros.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index e13b1f6bb400..ec2f3f1b836f 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -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
> 





^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
@ 2022-05-16 21:55   ` Heiko Stübner
  0 siblings, 0 replies; 8+ messages in thread
From: Heiko Stübner @ 2022-05-16 21:55 UTC (permalink / raw)
  To: Paul Walmsley, Palmer Dabbelt, Albert Ou, Nathan Chancellor
  Cc: Nick Desaulniers, linux-riscv, linux-kernel, llvm, patches,
	Nathan Chancellor, kernel test robot

Hi,

Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
> 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?


>   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.
> 
> Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>

In any case, on my svpbmt testcases (qemu + d1-nezha):
Tested-by: Heiko Stuebner <heiko@sntech.de>
Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Thanks for looking into that
Heiko


> ---
>  arch/riscv/include/asm/alternative-macros.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index e13b1f6bb400..ec2f3f1b836f 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -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
> 





_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
  2022-05-16 21:55   ` Heiko Stübner
@ 2022-05-16 22:38     ` Nathan Chancellor
  -1 siblings, 0 replies; 8+ messages in thread
From: Nathan Chancellor @ 2022-05-16 22:38 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Nick Desaulniers,
	linux-riscv, linux-kernel, llvm, patches, kernel test robot

Hi Heiko,

On Mon, May 16, 2022 at 11:55:47PM +0200, Heiko Stübner wrote:
> Hi,
> 
> Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
> > 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).

> >   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.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> In any case, on my svpbmt testcases (qemu + d1-nezha):
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> Thanks for looking into that
> Heiko

Thank you a lot for the quick testing and review!

Cheers,
Nathan

> > ---
> >  arch/riscv/include/asm/alternative-macros.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> > index e13b1f6bb400..ec2f3f1b836f 100644
> > --- a/arch/riscv/include/asm/alternative-macros.h
> > +++ b/arch/riscv/include/asm/alternative-macros.h
> > @@ -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
> > 
> 
> 
> 
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
@ 2022-05-16 22:38     ` Nathan Chancellor
  0 siblings, 0 replies; 8+ messages in thread
From: Nathan Chancellor @ 2022-05-16 22:38 UTC (permalink / raw)
  To: Heiko Stübner
  Cc: Paul Walmsley, Palmer Dabbelt, Albert Ou, Nick Desaulniers,
	linux-riscv, linux-kernel, llvm, patches, kernel test robot

Hi Heiko,

On Mon, May 16, 2022 at 11:55:47PM +0200, Heiko Stübner wrote:
> Hi,
> 
> Am Montag, 16. Mai 2022, 23:45:21 CEST schrieb Nathan Chancellor:
> > 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).

> >   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.
> > 
> > Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> > Reported-by: kernel test robot <lkp@intel.com>
> > Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> 
> In any case, on my svpbmt testcases (qemu + d1-nezha):
> Tested-by: Heiko Stuebner <heiko@sntech.de>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>
> 
> Thanks for looking into that
> Heiko

Thank you a lot for the quick testing and review!

Cheers,
Nathan

> > ---
> >  arch/riscv/include/asm/alternative-macros.h | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> > index e13b1f6bb400..ec2f3f1b836f 100644
> > --- a/arch/riscv/include/asm/alternative-macros.h
> > +++ b/arch/riscv/include/asm/alternative-macros.h
> > @@ -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
> > 
> 
> 
> 
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
  2022-05-16 21:45 ` Nathan Chancellor
@ 2022-06-03  1:34   ` Palmer Dabbelt
  -1 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2022-06-03  1:34 UTC (permalink / raw)
  To: nathan
  Cc: Paul Walmsley, aou, ndesaulniers, heiko, linux-riscv,
	linux-kernel, llvm, patches, nathan, lkp

On Mon, 16 May 2022 14:45:21 PDT (-0700), nathan@kernel.org 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 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.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  arch/riscv/include/asm/alternative-macros.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index e13b1f6bb400..ec2f3f1b836f 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -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

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.

^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH] riscv: Move alternative length validation into subsection
@ 2022-06-03  1:34   ` Palmer Dabbelt
  0 siblings, 0 replies; 8+ messages in thread
From: Palmer Dabbelt @ 2022-06-03  1:34 UTC (permalink / raw)
  To: nathan
  Cc: Paul Walmsley, aou, ndesaulniers, heiko, linux-riscv,
	linux-kernel, llvm, patches, nathan, lkp

On Mon, 16 May 2022 14:45:21 PDT (-0700), nathan@kernel.org 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 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.
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1640
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Nathan Chancellor <nathan@kernel.org>
> ---
>  arch/riscv/include/asm/alternative-macros.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> index e13b1f6bb400..ec2f3f1b836f 100644
> --- a/arch/riscv/include/asm/alternative-macros.h
> +++ b/arch/riscv/include/asm/alternative-macros.h
> @@ -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

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.

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

^ permalink raw reply	[flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-06-03  1:35 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-16 21:45 [PATCH] riscv: Move alternative length validation into subsection Nathan Chancellor
2022-05-16 21:45 ` Nathan Chancellor
2022-05-16 21:55 ` Heiko Stübner
2022-05-16 21:55   ` Heiko Stübner
2022-05-16 22:38   ` Nathan Chancellor
2022-05-16 22:38     ` Nathan Chancellor
2022-06-03  1:34 ` Palmer Dabbelt
2022-06-03  1:34   ` Palmer Dabbelt

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.