| Message ID | 1509563697-6359-2-git-send-email-gengdongjiu@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/11/17 19:14, Dongjiu Geng wrote: > ARMv8.2 adds a control bit to each SCTLR_ELx to insert implicit > Error Synchronization Barrier(IESB) operations at exception handler entry > and exit. But not all hardware platform which support RAS Extension > can support IESB. So for this case, software needs to manually insert > Error Synchronization Barrier(ESB) operations. > > In this macros, if system supports RAS Extensdddon instead of IESB, > it will insert an ESB instruction. > > Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> > --- > arch/arm64/include/asm/assembler.h | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h > index d4c0adf..e6c79c4 100644 > --- a/arch/arm64/include/asm/assembler.h > +++ b/arch/arm64/include/asm/assembler.h > @@ -517,4 +517,13 @@ > #endif > .endm > > + .macro error_synchronize > +alternative_if ARM64_HAS_IESB > + b 1f > +alternative_else_nop_endif > +alternative_if ARM64_HAS_RAS_EXTN > + esb > +alternative_else_nop_endif > +1: > + .endm Having a branch in here is pretty horrible, and furthermore using label number 1 has a pretty high chance of subtly breaking code where this macro is inserted. Can we not somehow nest or combine the alternative conditions here? Robin. > #endif /* __ASM_ASSEMBLER_H */ >
Hi Robin, On 2017/11/1 19:24, Robin Murphy wrote: >> + esb >> +alternative_else_nop_endif >> +1: >> + .endm > Having a branch in here is pretty horrible, and furthermore using label > number 1 has a pretty high chance of subtly breaking code where this > macro is inserted. > > Can we not somehow nest or combine the alternative conditions here? I found it will report error if combine the alternative conditions here. For example: + .macro error_synchronize +alternative_if ARM64_HAS_IESB +alternative_if ARM64_HAS_RAS_EXTN + esb +alternative_else_nop_endif +alternative_else_nop_endif + .endm And even using b.eq/cbz instruction in the alternative instruction in arch/arm64/kernel/entry.S, it will report Error. For example below alternative_if ARM64_HAS_PAN xxxxxxxxxxxxxxxxxxxx b.eq xxxxx alternative_else_nop_endif I do not dig it deeply, do you know the reason about it or good suggestion about that? Thanks a lot in advance. > > Robin. > >> #endif /* __ASM_ASSEMBLER_H */
On 01/11/17 12:54, gengdongjiu wrote: > Hi Robin, > > On 2017/11/1 19:24, Robin Murphy wrote: >>> + esb >>> +alternative_else_nop_endif >>> +1: >>> + .endm >> Having a branch in here is pretty horrible, and furthermore using label >> number 1 has a pretty high chance of subtly breaking code where this >> macro is inserted. >> >> Can we not somehow nest or combine the alternative conditions here? > > I found it will report error if combine the alternative conditions here. > > For example: > > + .macro error_synchronize > +alternative_if ARM64_HAS_IESB > +alternative_if ARM64_HAS_RAS_EXTN > + esb > +alternative_else_nop_endif > +alternative_else_nop_endif > + .endm > > And even using b.eq/cbz instruction in the alternative instruction in arch/arm64/kernel/entry.S, > it will report Error. > > For example below > > alternative_if ARM64_HAS_PAN > xxxxxxxxxxxxxxxxxxxx > b.eq xxxxx > alternative_else_nop_endif > > I do not dig it deeply, do you know the reason about it or good suggestion about that? > Thanks a lot in advance. Actually, on second look ARM64_HAS_RAS_EXTN doesn't even matter - ESB is a hint, so if the CPU doesn't have RAS it should behave as a NOP anyway. On which note, since I don't see one here - are any of those other patches defining an "esb" assembly macro similar to the inline asm case? If not then this isn't going to build with older toolchains - perhaps we should just use the raw hint syntax directly. Robin.
On Wed, Nov 01, 2017 at 08:54:44PM +0800, gengdongjiu wrote: > On 2017/11/1 19:24, Robin Murphy wrote: > >> + esb > >> +alternative_else_nop_endif > >> +1: > >> + .endm > > Having a branch in here is pretty horrible, and furthermore using label > > number 1 has a pretty high chance of subtly breaking code where this > > macro is inserted. > > > > Can we not somehow nest or combine the alternative conditions here? > > I found it will report error if combine the alternative conditions here. > > For example: > > + .macro error_synchronize > +alternative_if ARM64_HAS_IESB > +alternative_if ARM64_HAS_RAS_EXTN > + esb > +alternative_else_nop_endif > +alternative_else_nop_endif > + .endm > > And even using b.eq/cbz instruction in the alternative instruction in arch/arm64/kernel/entry.S, > it will report Error. Alternatives cannot be nested. You need to define a cap like: ARM64_HAS_RAS_NOT_IESB ... which is set when ARM64_HAS_RAS_EXTN && !ARM64_HAS_IESB. Then you can do: alternative_if ARM64_HAS_RAS_NOT_IESB esb alternative_else_nop_endif See ARM64_ALT_PAN_NOT_UAO for an example. That said, as Robin points out we may not even need the alternative. Thanks, Mark.
On 2017/11/1 22:16, Mark Rutland wrote: >> it will report Error. > Alternatives cannot be nested. You need to define a cap like: > > ARM64_HAS_RAS_NOT_IESB > > ... which is set when ARM64_HAS_RAS_EXTN && !ARM64_HAS_IESB. > > Then you can do: > > alternative_if ARM64_HAS_RAS_NOT_IESB > esb > alternative_else_nop_endif > > See ARM64_ALT_PAN_NOT_UAO for an example. > > That said, as Robin points out we may not even need the alternative. Ok, got it. thank you very much for your point and opinion > > Thanks, > Mark.
diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h index d4c0adf..e6c79c4 100644 --- a/arch/arm64/include/asm/assembler.h +++ b/arch/arm64/include/asm/assembler.h @@ -517,4 +517,13 @@ #endif .endm + .macro error_synchronize +alternative_if ARM64_HAS_IESB + b 1f +alternative_else_nop_endif +alternative_if ARM64_HAS_RAS_EXTN + esb +alternative_else_nop_endif +1: + .endm #endif /* __ASM_ASSEMBLER_H */
ARMv8.2 adds a control bit to each SCTLR_ELx to insert implicit Error Synchronization Barrier(IESB) operations at exception handler entry and exit. But not all hardware platform which support RAS Extension can support IESB. So for this case, software needs to manually insert Error Synchronization Barrier(ESB) operations. In this macros, if system supports RAS Extensdddon instead of IESB, it will insert an ESB instruction. Signed-off-by: Dongjiu Geng <gengdongjiu@huawei.com> --- arch/arm64/include/asm/assembler.h | 9 +++++++++ 1 file changed, 9 insertions(+)