All of lore.kernel.org
 help / color / mirror / Atom feed
* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 14:13 ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-01 14:13 UTC (permalink / raw)
  To: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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


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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 14:13 ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-01 14:13 UTC (permalink / raw)
  To: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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


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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 14:16         ` Mark Rutland
  (?)
@ 2017-11-02  8:52           ` gengdongjiu
  -1 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-02  8:52 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, ard.biesheuvel, cov, Dave.Martin,
	suzuki.poulose, linux-arm-kernel, linux-kernel, kvmarm


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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-02  8:52           ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-02  8:52 UTC (permalink / raw)
  To: Mark Rutland
  Cc: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, ard.biesheuvel, cov, Dave.Martin,
	suzuki.poulose, linux-arm-kernel, linux-kernel, kvmarm


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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-02  8:52           ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-02  8:52 UTC (permalink / raw)
  To: linux-arm-kernel


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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 19:14 [PATCH v1 0/3] manually add Error Synchronization Barrier at exception handler entry and exit Dongjiu Geng
  2017-11-01 19:14   ` Dongjiu Geng
@ 2017-11-01 19:14   ` Dongjiu Geng
  0 siblings, 0 replies; 18+ messages in thread
From: Dongjiu Geng @ 2017-11-01 19:14 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, marc.zyngier, christoffer.dall,
	james.morse, mark.rutland, ard.biesheuvel, robin.murphy, cov,
	Dave.Martin, gengdongjiu, suzuki.poulose, linux-arm-kernel,
	linux-kernel, kvmarm

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 19:14   ` Dongjiu Geng
  0 siblings, 0 replies; 18+ messages in thread
From: Dongjiu Geng @ 2017-11-01 19:14 UTC (permalink / raw)
  To: catalin.marinas, will.deacon, marc.zyngier, christoffer.dall,
	james.morse, mark.rutland, ard.biesheuvel, robin.murphy, cov,
	Dave.Martin, gengdongjiu, suzuki.poulose, linux-arm-kernel,
	linux-kernel, kvmarm

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 19:14   ` Dongjiu Geng
  0 siblings, 0 replies; 18+ messages in thread
From: Dongjiu Geng @ 2017-11-01 19:14 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 12:54       ` gengdongjiu
  (?)
@ 2017-11-01 14:16         ` Mark Rutland
  -1 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2017-11-01 14:16 UTC (permalink / raw)
  To: gengdongjiu
  Cc: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, ard.biesheuvel, cov, Dave.Martin,
	suzuki.poulose, linux-arm-kernel, linux-kernel, kvmarm

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 14:16         ` Mark Rutland
  0 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2017-11-01 14:16 UTC (permalink / raw)
  To: gengdongjiu
  Cc: linux-arm-kernel, ard.biesheuvel, marc.zyngier, catalin.marinas,
	will.deacon, linux-kernel, kvmarm, cov, Robin Murphy,
	Dave.Martin

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 14:16         ` Mark Rutland
  0 siblings, 0 replies; 18+ messages in thread
From: Mark Rutland @ 2017-11-01 14:16 UTC (permalink / raw)
  To: linux-arm-kernel

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 12:54       ` gengdongjiu
@ 2017-11-01 13:31         ` Robin Murphy
  -1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-11-01 13:31 UTC (permalink / raw)
  To: gengdongjiu, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 13:31         ` Robin Murphy
  0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-11-01 13:31 UTC (permalink / raw)
  To: linux-arm-kernel

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.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 11:24     ` Robin Murphy
  (?)
@ 2017-11-01 12:54       ` gengdongjiu
  -1 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-01 12:54 UTC (permalink / raw)
  To: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 12:54       ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-01 12:54 UTC (permalink / raw)
  To: Robin Murphy, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 12:54       ` gengdongjiu
  0 siblings, 0 replies; 18+ messages in thread
From: gengdongjiu @ 2017-11-01 12:54 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v1 1/3] arm64: add a macro for SError synchronization
  2017-11-01 19:14   ` Dongjiu Geng
@ 2017-11-01 11:24     ` Robin Murphy
  -1 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-11-01 11:24 UTC (permalink / raw)
  To: Dongjiu Geng, catalin.marinas, will.deacon, marc.zyngier,
	christoffer.dall, james.morse, mark.rutland, ard.biesheuvel, cov,
	Dave.Martin, suzuki.poulose, linux-arm-kernel, linux-kernel,
	kvmarm

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v1 1/3] arm64: add a macro for SError synchronization
@ 2017-11-01 11:24     ` Robin Murphy
  0 siblings, 0 replies; 18+ messages in thread
From: Robin Murphy @ 2017-11-01 11:24 UTC (permalink / raw)
  To: linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2017-11-02  8:58 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-01 14:13 [PATCH v1 1/3] arm64: add a macro for SError synchronization gengdongjiu
2017-11-01 14:13 ` gengdongjiu
2017-11-01 19:14 [PATCH v1 0/3] manually add Error Synchronization Barrier at exception handler entry and exit Dongjiu Geng
2017-11-01 19:14 ` [PATCH v1 1/3] arm64: add a macro for SError synchronization Dongjiu Geng
2017-11-01 19:14   ` Dongjiu Geng
2017-11-01 19:14   ` Dongjiu Geng
2017-11-01 11:24   ` Robin Murphy
2017-11-01 11:24     ` Robin Murphy
2017-11-01 12:54     ` gengdongjiu
2017-11-01 12:54       ` gengdongjiu
2017-11-01 12:54       ` gengdongjiu
2017-11-01 13:31       ` Robin Murphy
2017-11-01 13:31         ` Robin Murphy
2017-11-01 14:16       ` Mark Rutland
2017-11-01 14:16         ` Mark Rutland
2017-11-01 14:16         ` Mark Rutland
2017-11-02  8:52         ` gengdongjiu
2017-11-02  8:52           ` gengdongjiu
2017-11-02  8:52           ` gengdongjiu

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.