Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[gengdongjiu]

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.

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[gengdongjiu]

> 
> 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
> > 
> > b.eqx
> > 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.


Yes, you are right. It is "HINT #16"

So in fact it can be written below:

+   .macro  error_synchronize
+alternative_if_not ARM64_HAS_IESB
+   esb
+alternative_else_nop_endif
+   .endm

If written to that, whether it will be strange? although ESB should behave as a
NOP anyway if the CPU doesn't have RAS. 

> 
> 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.


Sorry for that I do not push the dependent patch[1].
The "ESB" is defined as a macro 

/*
+ * RAS Error Synchronization barrier
+ */
+   .macro  esb
+   hint#16
+   .endm
+
+/*

[1]
https://www.spinics.net/lists/arm-kernel/msg612884.html

> 
> Robin.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[Mark Rutland]

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.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[Robin Murphy]

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
>   
> b.eqx
> 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.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[gengdongjiu]

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

b.eqx
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 */

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization

[Robin Murphy]

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 
> ---
>  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 */
> 

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[PATCH v1 1/3] arm64: add a macro for SError synchronization

[Dongjiu Geng]

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 
---
 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
 #endif /* __ASM_ASSEMBLER_H */
-- 
1.9.1

___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm