[PATCH] arm64/mm: use correct operators for string comparison in cache.S

[PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

The GAS directives that are currently being used in dcache_by_line_op
rely on assembler behavior that is not documented, and probably not
guaranteed to produce the correct behavior going forward.

Currently, we end up with some undefined symbols in cache.o:

$ nm arch/arm64/mm/cache.o
	 ...
	 U civac
	 ...
	 U cvac
	 U cvap
	 U cvau

This is due to the fact that the comparisons used to select the
operation type in the dcache_by_line_op macro are comparing symbols
not strings, and even though it seems that GAS is doing the right
thing here (undefined symbols by the same name are equal to each
other), it seems unwise to rely on this.

So let's provide some definitions that are guaranteed to be distinct,
and make them local so they don't pollute the gobal symbol space.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 arch/arm64/include/asm/assembler.h | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
index 6142402c2eb4..d11c32df85c2 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -377,19 +377,24 @@ alternative_endif
  * 	size:		size of the region
  * 	Corrupts:	kaddr, size, tmp1, tmp2
  */
+	.set	.Lcvau,  0
+	.set	.Lcvac,  1
+	.set	.Lcvap,  2
+	.set	.Lcivac, 3
+
 	.macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
 	dcache_line_size \tmp1, \tmp2
 	add	\size, \kaddr, \size
 	sub	\tmp2, \tmp1, #1
 	bic	\kaddr, \kaddr, \tmp2
 9998:
-	.if	(\op == cvau || \op == cvac)
+	.if	(.L\op == .Lcvau || .L\op == .Lcvac)
 alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
 	dc	\op, \kaddr
 alternative_else
 	dc	civac, \kaddr
 alternative_endif
-	.elseif	(\op == cvap)
+	.elseif	(.L\op == .Lcvap)
 alternative_if ARM64_HAS_DCPOP
 	sys 3, c7, c12, 1, \kaddr	// dc cvap
 alternative_else
-- 
2.19.2


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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Robin Murphy]

Hi Ard,

On 01/12/2018 11:01, Ard Biesheuvel wrote:
> The GAS directives that are currently being used in dcache_by_line_op
> rely on assembler behavior that is not documented, and probably not
> guaranteed to produce the correct behavior going forward.
> 
> Currently, we end up with some undefined symbols in cache.o:
> 
> $ nm arch/arm64/mm/cache.o
> 	 ...
> 	 U civac
> 	 ...
> 	 U cvac
> 	 U cvap
> 	 U cvau
> 
> This is due to the fact that the comparisons used to select the
> operation type in the dcache_by_line_op macro are comparing symbols
> not strings, and even though it seems that GAS is doing the right
> thing here (undefined symbols by the same name are equal to each
> other), it seems unwise to rely on this.
> 
> So let's provide some definitions that are guaranteed to be distinct,
> and make them local so they don't pollute the gobal symbol space.

Rather than making the unintended symbol comparisons work properly, can 
we not just implement the string comparisons that were supposed to be? 
Superficially, the diff below seems to still generate the desired output 
(although as always there's probably some subtlety I'm missing).

Robin.

----->8-----

diff --git a/arch/arm64/include/asm/assembler.h 
b/arch/arm64/include/asm/assembler.h
index 6142402c2eb4..2c5f4825fee3 100644
--- a/arch/arm64/include/asm/assembler.h
+++ b/arch/arm64/include/asm/assembler.h
@@ -383,13 +383,13 @@ alternative_endif
  	sub	\tmp2, \tmp1, #1
  	bic	\kaddr, \kaddr, \tmp2
  9998:
-	.if	(\op == cvau || \op == cvac)
+	.if	("\op" == "cvau" || "\op" == "cvac")
  alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
  	dc	\op, \kaddr
  alternative_else
  	dc	civac, \kaddr
  alternative_endif
-	.elseif	(\op == cvap)
+	.elseif	("\op" == "cvap")
  alternative_if ARM64_HAS_DCPOP
  	sys 3, c7, c12, 1, \kaddr	// dc cvap
  alternative_else

> 
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
>   arch/arm64/include/asm/assembler.h | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> index 6142402c2eb4..d11c32df85c2 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -377,19 +377,24 @@ alternative_endif
>    * 	size:		size of the region
>    * 	Corrupts:	kaddr, size, tmp1, tmp2
>    */
> +	.set	.Lcvau,  0
> +	.set	.Lcvac,  1
> +	.set	.Lcvap,  2
> +	.set	.Lcivac, 3
> +
>   	.macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
>   	dcache_line_size \tmp1, \tmp2
>   	add	\size, \kaddr, \size
>   	sub	\tmp2, \tmp1, #1
>   	bic	\kaddr, \kaddr, \tmp2
>   9998:
> -	.if	(\op == cvau || \op == cvac)
> +	.if	(.L\op == .Lcvau || .L\op == .Lcvac)
>   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
>   	dc	\op, \kaddr
>   alternative_else
>   	dc	civac, \kaddr
>   alternative_endif
> -	.elseif	(\op == cvap)
> +	.elseif	(.L\op == .Lcvap)
>   alternative_if ARM64_HAS_DCPOP
>   	sys 3, c7, c12, 1, \kaddr	// dc cvap
>   alternative_else
> 

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
>
> Hi Ard,
>
> On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > The GAS directives that are currently being used in dcache_by_line_op
> > rely on assembler behavior that is not documented, and probably not
> > guaranteed to produce the correct behavior going forward.
> >
> > Currently, we end up with some undefined symbols in cache.o:
> >
> > $ nm arch/arm64/mm/cache.o
> >        ...
> >        U civac
> >        ...
> >        U cvac
> >        U cvap
> >        U cvau
> >
> > This is due to the fact that the comparisons used to select the
> > operation type in the dcache_by_line_op macro are comparing symbols
> > not strings, and even though it seems that GAS is doing the right
> > thing here (undefined symbols by the same name are equal to each
> > other), it seems unwise to rely on this.
> >
> > So let's provide some definitions that are guaranteed to be distinct,
> > and make them local so they don't pollute the gobal symbol space.
>
> Rather than making the unintended symbol comparisons work properly, can
> we not just implement the string comparisons that were supposed to be?
> Superficially, the diff below seems to still generate the desired output
> (although as always there's probably some subtlety I'm missing).
>
> Robin.
>
> ----->8-----
>
> diff --git a/arch/arm64/include/asm/assembler.h
> b/arch/arm64/include/asm/assembler.h
> index 6142402c2eb4..2c5f4825fee3 100644
> --- a/arch/arm64/include/asm/assembler.h
> +++ b/arch/arm64/include/asm/assembler.h
> @@ -383,13 +383,13 @@ alternative_endif
>         sub     \tmp2, \tmp1, #1
>         bic     \kaddr, \kaddr, \tmp2
>   9998:
> -       .if     (\op == cvau || \op == cvac)
> +       .if     ("\op" == "cvau" || "\op" == "cvac")
>   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
>         dc      \op, \kaddr
>   alternative_else
>         dc      civac, \kaddr
>   alternative_endif
> -       .elseif (\op == cvap)
> +       .elseif ("\op" == "cvap")
>   alternative_if ARM64_HAS_DCPOP
>         sys 3, c7, c12, 1, \kaddr       // dc cvap
>   alternative_else
>

Looking at the GAS info pages, I find

"Operators" are arithmetic functions, like '+' or '%'.
"Arguments" are symbols, numbers or subexpressions.
An "expression" specifies an address or numeric value.

so even if the comparison works as expected, I'm hesitant to rely on
it to work as expected on any version of GAS or any other assembler
claiming to implement the GAS asm dialect.

We could change the logic to .ifc, which is defined to operate on string, i.e.,

    .ifnc \op, civac
    .ifnc \op, cvap
alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
    dc \op, \kaddr
alternative_else
    dc civac, \kaddr
alternative_endif
    .else
alternative_if ARM64_HAS_DCPOP
    sys 3, c7, c12, 1, \kaddr // dc cvap
alternative_else
    dc cvac, \kaddr
alternative_endif
    .endif
    .else
     dc      civac, \kaddr
    .endif

> >
> > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> > ---
> >   arch/arm64/include/asm/assembler.h | 9 +++++++--
> >   1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/include/asm/assembler.h b/arch/arm64/include/asm/assembler.h
> > index 6142402c2eb4..d11c32df85c2 100644
> > --- a/arch/arm64/include/asm/assembler.h
> > +++ b/arch/arm64/include/asm/assembler.h
> > @@ -377,19 +377,24 @@ alternative_endif
> >    *  size:           size of the region
> >    *  Corrupts:       kaddr, size, tmp1, tmp2
> >    */
> > +     .set    .Lcvau,  0
> > +     .set    .Lcvac,  1
> > +     .set    .Lcvap,  2
> > +     .set    .Lcivac, 3
> > +
> >       .macro dcache_by_line_op op, domain, kaddr, size, tmp1, tmp2
> >       dcache_line_size \tmp1, \tmp2
> >       add     \size, \kaddr, \size
> >       sub     \tmp2, \tmp1, #1
> >       bic     \kaddr, \kaddr, \tmp2
> >   9998:
> > -     .if     (\op == cvau || \op == cvac)
> > +     .if     (.L\op == .Lcvau || .L\op == .Lcvac)
> >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> >       dc      \op, \kaddr
> >   alternative_else
> >       dc      civac, \kaddr
> >   alternative_endif
> > -     .elseif (\op == cvap)
> > +     .elseif (.L\op == .Lcvap)
> >   alternative_if ARM64_HAS_DCPOP
> >       sys 3, c7, c12, 1, \kaddr       // dc cvap
> >   alternative_else
> >

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Will Deacon]

On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > The GAS directives that are currently being used in dcache_by_line_op
> > > rely on assembler behavior that is not documented, and probably not
> > > guaranteed to produce the correct behavior going forward.
> > >
> > > Currently, we end up with some undefined symbols in cache.o:
> > >
> > > $ nm arch/arm64/mm/cache.o
> > >        ...
> > >        U civac
> > >        ...
> > >        U cvac
> > >        U cvap
> > >        U cvau
> > >
> > > This is due to the fact that the comparisons used to select the
> > > operation type in the dcache_by_line_op macro are comparing symbols
> > > not strings, and even though it seems that GAS is doing the right
> > > thing here (undefined symbols by the same name are equal to each
> > > other), it seems unwise to rely on this.
> > >
> > > So let's provide some definitions that are guaranteed to be distinct,
> > > and make them local so they don't pollute the gobal symbol space.
> >
> > Rather than making the unintended symbol comparisons work properly, can
> > we not just implement the string comparisons that were supposed to be?
> > Superficially, the diff below seems to still generate the desired output
> > (although as always there's probably some subtlety I'm missing).
> >
> > Robin.
> >
> > ----->8-----
> >
> > diff --git a/arch/arm64/include/asm/assembler.h
> > b/arch/arm64/include/asm/assembler.h
> > index 6142402c2eb4..2c5f4825fee3 100644
> > --- a/arch/arm64/include/asm/assembler.h
> > +++ b/arch/arm64/include/asm/assembler.h
> > @@ -383,13 +383,13 @@ alternative_endif
> >         sub     \tmp2, \tmp1, #1
> >         bic     \kaddr, \kaddr, \tmp2
> >   9998:
> > -       .if     (\op == cvau || \op == cvac)
> > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> >         dc      \op, \kaddr
> >   alternative_else
> >         dc      civac, \kaddr
> >   alternative_endif
> > -       .elseif (\op == cvap)
> > +       .elseif ("\op" == "cvap")
> >   alternative_if ARM64_HAS_DCPOP
> >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> >   alternative_else
> >
> 
> Looking at the GAS info pages, I find
> 
> "Operators" are arithmetic functions, like '+' or '%'.
> "Arguments" are symbols, numbers or subexpressions.
> An "expression" specifies an address or numeric value.
> 
> so even if the comparison works as expected, I'm hesitant to rely on
> it to work as expected on any version of GAS or any other assembler
> claiming to implement the GAS asm dialect.
> 
> We could change the logic to .ifc, which is defined to operate on string, i.e.,

That looks better to me, although I'm not sure why you're inverted the logic
here:

>     .ifnc \op, civac
>     .ifnc \op, cvap

What am I missing?

Will

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

On Mon, 3 Dec 2018 at 18:44, Will Deacon <will.deacon@arm.com> wrote:
>
> On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > rely on assembler behavior that is not documented, and probably not
> > > > guaranteed to produce the correct behavior going forward.
> > > >
> > > > Currently, we end up with some undefined symbols in cache.o:
> > > >
> > > > $ nm arch/arm64/mm/cache.o
> > > >        ...
> > > >        U civac
> > > >        ...
> > > >        U cvac
> > > >        U cvap
> > > >        U cvau
> > > >
> > > > This is due to the fact that the comparisons used to select the
> > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > not strings, and even though it seems that GAS is doing the right
> > > > thing here (undefined symbols by the same name are equal to each
> > > > other), it seems unwise to rely on this.
> > > >
> > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > and make them local so they don't pollute the gobal symbol space.
> > >
> > > Rather than making the unintended symbol comparisons work properly, can
> > > we not just implement the string comparisons that were supposed to be?
> > > Superficially, the diff below seems to still generate the desired output
> > > (although as always there's probably some subtlety I'm missing).
> > >
> > > Robin.
> > >
> > > ----->8-----
> > >
> > > diff --git a/arch/arm64/include/asm/assembler.h
> > > b/arch/arm64/include/asm/assembler.h
> > > index 6142402c2eb4..2c5f4825fee3 100644
> > > --- a/arch/arm64/include/asm/assembler.h
> > > +++ b/arch/arm64/include/asm/assembler.h
> > > @@ -383,13 +383,13 @@ alternative_endif
> > >         sub     \tmp2, \tmp1, #1
> > >         bic     \kaddr, \kaddr, \tmp2
> > >   9998:
> > > -       .if     (\op == cvau || \op == cvac)
> > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > >         dc      \op, \kaddr
> > >   alternative_else
> > >         dc      civac, \kaddr
> > >   alternative_endif
> > > -       .elseif (\op == cvap)
> > > +       .elseif ("\op" == "cvap")
> > >   alternative_if ARM64_HAS_DCPOP
> > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > >   alternative_else
> > >
> >
> > Looking at the GAS info pages, I find
> >
> > "Operators" are arithmetic functions, like '+' or '%'.
> > "Arguments" are symbols, numbers or subexpressions.
> > An "expression" specifies an address or numeric value.
> >
> > so even if the comparison works as expected, I'm hesitant to rely on
> > it to work as expected on any version of GAS or any other assembler
> > claiming to implement the GAS asm dialect.
> >
> > We could change the logic to .ifc, which is defined to operate on string, i.e.,
>
> That looks better to me, although I'm not sure why you're inverted the logic
> here:
>
> >     .ifnc \op, civac
> >     .ifnc \op, cvap
>
> What am I missing?
>

.ifc does not permit '\op equals string1 or \op equals string2'

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Will Deacon]

On Mon, Dec 03, 2018 at 06:54:35PM +0100, Ard Biesheuvel wrote:
> On Mon, 3 Dec 2018 at 18:44, Will Deacon <will.deacon@arm.com> wrote:
> >
> > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > > rely on assembler behavior that is not documented, and probably not
> > > > > guaranteed to produce the correct behavior going forward.
> > > > >
> > > > > Currently, we end up with some undefined symbols in cache.o:
> > > > >
> > > > > $ nm arch/arm64/mm/cache.o
> > > > >        ...
> > > > >        U civac
> > > > >        ...
> > > > >        U cvac
> > > > >        U cvap
> > > > >        U cvau
> > > > >
> > > > > This is due to the fact that the comparisons used to select the
> > > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > > not strings, and even though it seems that GAS is doing the right
> > > > > thing here (undefined symbols by the same name are equal to each
> > > > > other), it seems unwise to rely on this.
> > > > >
> > > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > > and make them local so they don't pollute the gobal symbol space.
> > > >
> > > > Rather than making the unintended symbol comparisons work properly, can
> > > > we not just implement the string comparisons that were supposed to be?
> > > > Superficially, the diff below seems to still generate the desired output
> > > > (although as always there's probably some subtlety I'm missing).
> > > >
> > > > Robin.
> > > >
> > > > ----->8-----
> > > >
> > > > diff --git a/arch/arm64/include/asm/assembler.h
> > > > b/arch/arm64/include/asm/assembler.h
> > > > index 6142402c2eb4..2c5f4825fee3 100644
> > > > --- a/arch/arm64/include/asm/assembler.h
> > > > +++ b/arch/arm64/include/asm/assembler.h
> > > > @@ -383,13 +383,13 @@ alternative_endif
> > > >         sub     \tmp2, \tmp1, #1
> > > >         bic     \kaddr, \kaddr, \tmp2
> > > >   9998:
> > > > -       .if     (\op == cvau || \op == cvac)
> > > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > > >         dc      \op, \kaddr
> > > >   alternative_else
> > > >         dc      civac, \kaddr
> > > >   alternative_endif
> > > > -       .elseif (\op == cvap)
> > > > +       .elseif ("\op" == "cvap")
> > > >   alternative_if ARM64_HAS_DCPOP
> > > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > > >   alternative_else
> > > >
> > >
> > > Looking at the GAS info pages, I find
> > >
> > > "Operators" are arithmetic functions, like '+' or '%'.
> > > "Arguments" are symbols, numbers or subexpressions.
> > > An "expression" specifies an address or numeric value.
> > >
> > > so even if the comparison works as expected, I'm hesitant to rely on
> > > it to work as expected on any version of GAS or any other assembler
> > > claiming to implement the GAS asm dialect.
> > >
> > > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> >
> > That looks better to me, although I'm not sure why you're inverted the logic
> > here:
> >
> > >     .ifnc \op, civac
> > >     .ifnc \op, cvap
> >
> > What am I missing?
> >
> 
> .ifc does not permit '\op equals string1 or \op equals string2'

Thanks. Then I guess we invert the logic as you suggest, or we duplicate the
alternative code. Looking at this some more, I think what we currently have
is broken because on a system with ARM64_WORKAROUND_CLEAN_CACHE but not
ARM64_HAS_DCPOP, you'll get DC CVAC for __clean_dcache_area_pop, which
would be broken on that CPU.

Will

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

On Mon, 3 Dec 2018 at 19:10, Will Deacon <will.deacon@arm.com> wrote:
>
> On Mon, Dec 03, 2018 at 06:54:35PM +0100, Ard Biesheuvel wrote:
> > On Mon, 3 Dec 2018 at 18:44, Will Deacon <will.deacon@arm.com> wrote:
> > >
> > > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > > > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > > > rely on assembler behavior that is not documented, and probably not
> > > > > > guaranteed to produce the correct behavior going forward.
> > > > > >
> > > > > > Currently, we end up with some undefined symbols in cache.o:
> > > > > >
> > > > > > $ nm arch/arm64/mm/cache.o
> > > > > >        ...
> > > > > >        U civac
> > > > > >        ...
> > > > > >        U cvac
> > > > > >        U cvap
> > > > > >        U cvau
> > > > > >
> > > > > > This is due to the fact that the comparisons used to select the
> > > > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > > > not strings, and even though it seems that GAS is doing the right
> > > > > > thing here (undefined symbols by the same name are equal to each
> > > > > > other), it seems unwise to rely on this.
> > > > > >
> > > > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > > > and make them local so they don't pollute the gobal symbol space.
> > > > >
> > > > > Rather than making the unintended symbol comparisons work properly, can
> > > > > we not just implement the string comparisons that were supposed to be?
> > > > > Superficially, the diff below seems to still generate the desired output
> > > > > (although as always there's probably some subtlety I'm missing).
> > > > >
> > > > > Robin.
> > > > >
> > > > > ----->8-----
> > > > >
> > > > > diff --git a/arch/arm64/include/asm/assembler.h
> > > > > b/arch/arm64/include/asm/assembler.h
> > > > > index 6142402c2eb4..2c5f4825fee3 100644
> > > > > --- a/arch/arm64/include/asm/assembler.h
> > > > > +++ b/arch/arm64/include/asm/assembler.h
> > > > > @@ -383,13 +383,13 @@ alternative_endif
> > > > >         sub     \tmp2, \tmp1, #1
> > > > >         bic     \kaddr, \kaddr, \tmp2
> > > > >   9998:
> > > > > -       .if     (\op == cvau || \op == cvac)
> > > > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > > > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > > > >         dc      \op, \kaddr
> > > > >   alternative_else
> > > > >         dc      civac, \kaddr
> > > > >   alternative_endif
> > > > > -       .elseif (\op == cvap)
> > > > > +       .elseif ("\op" == "cvap")
> > > > >   alternative_if ARM64_HAS_DCPOP
> > > > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > > > >   alternative_else
> > > > >
> > > >
> > > > Looking at the GAS info pages, I find
> > > >
> > > > "Operators" are arithmetic functions, like '+' or '%'.
> > > > "Arguments" are symbols, numbers or subexpressions.
> > > > An "expression" specifies an address or numeric value.
> > > >
> > > > so even if the comparison works as expected, I'm hesitant to rely on
> > > > it to work as expected on any version of GAS or any other assembler
> > > > claiming to implement the GAS asm dialect.
> > > >
> > > > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> > >
> > > That looks better to me, although I'm not sure why you're inverted the logic
> > > here:
> > >
> > > >     .ifnc \op, civac
> > > >     .ifnc \op, cvap
> > >
> > > What am I missing?
> > >
> >
> > .ifc does not permit '\op equals string1 or \op equals string2'
>
> Thanks. Then I guess we invert the logic as you suggest, or we duplicate the
> alternative code. Looking at this some more, I think what we currently have
> is broken because on a system with ARM64_WORKAROUND_CLEAN_CACHE but not
> ARM64_HAS_DCPOP, you'll get DC CVAC for __clean_dcache_area_pop, which
> would be broken on that CPU.
>

Can we just fallback to civac instead? Or do we need to add logic to
combine the two feature flags?

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Dave Martin]

On Mon, Dec 03, 2018 at 05:45:06PM +0000, Will Deacon wrote:
> On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > rely on assembler behavior that is not documented, and probably not
> > > > guaranteed to produce the correct behavior going forward.
> > > >
> > > > Currently, we end up with some undefined symbols in cache.o:
> > > >
> > > > $ nm arch/arm64/mm/cache.o
> > > >        ...
> > > >        U civac
> > > >        ...
> > > >        U cvac
> > > >        U cvap
> > > >        U cvau
> > > >
> > > > This is due to the fact that the comparisons used to select the
> > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > not strings, and even though it seems that GAS is doing the right
> > > > thing here (undefined symbols by the same name are equal to each
> > > > other), it seems unwise to rely on this.
> > > >
> > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > and make them local so they don't pollute the gobal symbol space.
> > >
> > > Rather than making the unintended symbol comparisons work properly, can
> > > we not just implement the string comparisons that were supposed to be?
> > > Superficially, the diff below seems to still generate the desired output
> > > (although as always there's probably some subtlety I'm missing).
> > >
> > > Robin.
> > >
> > > ----->8-----
> > >
> > > diff --git a/arch/arm64/include/asm/assembler.h
> > > b/arch/arm64/include/asm/assembler.h
> > > index 6142402c2eb4..2c5f4825fee3 100644
> > > --- a/arch/arm64/include/asm/assembler.h
> > > +++ b/arch/arm64/include/asm/assembler.h
> > > @@ -383,13 +383,13 @@ alternative_endif
> > >         sub     \tmp2, \tmp1, #1
> > >         bic     \kaddr, \kaddr, \tmp2
> > >   9998:
> > > -       .if     (\op == cvau || \op == cvac)
> > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > >         dc      \op, \kaddr
> > >   alternative_else
> > >         dc      civac, \kaddr
> > >   alternative_endif
> > > -       .elseif (\op == cvap)
> > > +       .elseif ("\op" == "cvap")

(Whatever this looks like, it's still comparing symbols someone probably
already pointed that out.  Oh, and the () are redundant.  This isn't C,
but, meh.)

> > >   alternative_if ARM64_HAS_DCPOP
> > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > >   alternative_else
> > >
> > 
> > Looking at the GAS info pages, I find
> > 
> > "Operators" are arithmetic functions, like '+' or '%'.
> > "Arguments" are symbols, numbers or subexpressions.
> > An "expression" specifies an address or numeric value.
> > 
> > so even if the comparison works as expected, I'm hesitant to rely on
> > it to work as expected on any version of GAS or any other assembler
> > claiming to implement the GAS asm dialect.
> > 
> > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> 
> That looks better to me, although I'm not sure why you're inverted the logic
> here:
> 
> >     .ifnc \op, civac
> >     .ifnc \op, cvap
> 
> What am I missing?

I vote for the .ifc approach.

Note, the current works-by-accident approach using == has the odd side-
effect of spitting out undefined symbol references in the .o file.  It
seems that isn't breaking the link, but I wonder whether there are any
side-effects we're not aware of.

If we don't like the inverted logic, there is always

	.set .L__foo_\@, 0
	.ifc \op, civac
	.set .L__foo_\@, 1
	.endif
	.ifc \op, cvap
	.set .L__foo_\@, 1
	.endif

	.if .L__foo_\@
	// ...
	.endif

which is ugly.  Either way, the logic could be wrapped as a helper:

	.macro if_string_is_either str, cmp1, cmp2, insn:vararg
		.ifnc "\str","\cmp1"
		.ifnc "\str","\cmp2"
		.exitm
		.endif
		.endif

		\insn
	.endm

Cheers
---Dave

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

On Thu, 6 Dec 2018 at 12:20, Dave Martin <Dave.Martin@arm.com> wrote:
>
> On Mon, Dec 03, 2018 at 05:45:06PM +0000, Will Deacon wrote:
> > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > > rely on assembler behavior that is not documented, and probably not
> > > > > guaranteed to produce the correct behavior going forward.
> > > > >
> > > > > Currently, we end up with some undefined symbols in cache.o:
> > > > >
> > > > > $ nm arch/arm64/mm/cache.o
> > > > >        ...
> > > > >        U civac
> > > > >        ...
> > > > >        U cvac
> > > > >        U cvap
> > > > >        U cvau
> > > > >
> > > > > This is due to the fact that the comparisons used to select the
> > > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > > not strings, and even though it seems that GAS is doing the right
> > > > > thing here (undefined symbols by the same name are equal to each
> > > > > other), it seems unwise to rely on this.
> > > > >
> > > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > > and make them local so they don't pollute the gobal symbol space.
> > > >
> > > > Rather than making the unintended symbol comparisons work properly, can
> > > > we not just implement the string comparisons that were supposed to be?
> > > > Superficially, the diff below seems to still generate the desired output
> > > > (although as always there's probably some subtlety I'm missing).
> > > >
> > > > Robin.
> > > >
> > > > ----->8-----
> > > >
> > > > diff --git a/arch/arm64/include/asm/assembler.h
> > > > b/arch/arm64/include/asm/assembler.h
> > > > index 6142402c2eb4..2c5f4825fee3 100644
> > > > --- a/arch/arm64/include/asm/assembler.h
> > > > +++ b/arch/arm64/include/asm/assembler.h
> > > > @@ -383,13 +383,13 @@ alternative_endif
> > > >         sub     \tmp2, \tmp1, #1
> > > >         bic     \kaddr, \kaddr, \tmp2
> > > >   9998:
> > > > -       .if     (\op == cvau || \op == cvac)
> > > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > > >         dc      \op, \kaddr
> > > >   alternative_else
> > > >         dc      civac, \kaddr
> > > >   alternative_endif
> > > > -       .elseif (\op == cvap)
> > > > +       .elseif ("\op" == "cvap")
>
> (Whatever this looks like, it's still comparing symbols someone probably
> already pointed that out.  Oh, and the () are redundant.  This isn't C,
> but, meh.)
>
> > > >   alternative_if ARM64_HAS_DCPOP
> > > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > > >   alternative_else
> > > >
> > >
> > > Looking at the GAS info pages, I find
> > >
> > > "Operators" are arithmetic functions, like '+' or '%'.
> > > "Arguments" are symbols, numbers or subexpressions.
> > > An "expression" specifies an address or numeric value.
> > >
> > > so even if the comparison works as expected, I'm hesitant to rely on
> > > it to work as expected on any version of GAS or any other assembler
> > > claiming to implement the GAS asm dialect.
> > >
> > > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> >
> > That looks better to me, although I'm not sure why you're inverted the logic
> > here:
> >
> > >     .ifnc \op, civac
> > >     .ifnc \op, cvap
> >
> > What am I missing?
>
> I vote for the .ifc approach.
>
> Note, the current works-by-accident approach using == has the odd side-
> effect of spitting out undefined symbol references in the .o file.  It
> seems that isn't breaking the link, but I wonder whether there are any
> side-effects we're not aware of.
>

Did you read the commit log at all? :-)

> If we don't like the inverted logic, there is always
>
>         .set .L__foo_\@, 0
>         .ifc \op, civac
>         .set .L__foo_\@, 1
>         .endif
>         .ifc \op, cvap
>         .set .L__foo_\@, 1
>         .endif
>
>         .if .L__foo_\@
>         // ...
>         .endif
>
> which is ugly.  Either way, the logic could be wrapped as a helper:
>
>         .macro if_string_is_either str, cmp1, cmp2, insn:vararg
>                 .ifnc "\str","\cmp1"
>                 .ifnc "\str","\cmp2"
>                 .exitm
>                 .endif
>                 .endif
>
>                 \insn
>         .endm
>

Yeah, I don't think this is an improvement over using inverted logic.

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Will Deacon]

On Tue, Dec 04, 2018 at 01:44:01AM +0100, Ard Biesheuvel wrote:
> On Mon, 3 Dec 2018 at 19:10, Will Deacon <will.deacon@arm.com> wrote:
> >
> > On Mon, Dec 03, 2018 at 06:54:35PM +0100, Ard Biesheuvel wrote:
> > > On Mon, 3 Dec 2018 at 18:44, Will Deacon <will.deacon@arm.com> wrote:
> > > >
> > > > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > > > > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > > > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > > > > rely on assembler behavior that is not documented, and probably not
> > > > > > > guaranteed to produce the correct behavior going forward.
> > > > > > >
> > > > > > > Currently, we end up with some undefined symbols in cache.o:
> > > > > > >
> > > > > > > $ nm arch/arm64/mm/cache.o
> > > > > > >        ...
> > > > > > >        U civac
> > > > > > >        ...
> > > > > > >        U cvac
> > > > > > >        U cvap
> > > > > > >        U cvau
> > > > > > >
> > > > > > > This is due to the fact that the comparisons used to select the
> > > > > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > > > > not strings, and even though it seems that GAS is doing the right
> > > > > > > thing here (undefined symbols by the same name are equal to each
> > > > > > > other), it seems unwise to rely on this.
> > > > > > >
> > > > > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > > > > and make them local so they don't pollute the gobal symbol space.
> > > > > >
> > > > > > Rather than making the unintended symbol comparisons work properly, can
> > > > > > we not just implement the string comparisons that were supposed to be?
> > > > > > Superficially, the diff below seems to still generate the desired output
> > > > > > (although as always there's probably some subtlety I'm missing).
> > > > > >
> > > > > > Robin.
> > > > > >
> > > > > > ----->8-----
> > > > > >
> > > > > > diff --git a/arch/arm64/include/asm/assembler.h
> > > > > > b/arch/arm64/include/asm/assembler.h
> > > > > > index 6142402c2eb4..2c5f4825fee3 100644
> > > > > > --- a/arch/arm64/include/asm/assembler.h
> > > > > > +++ b/arch/arm64/include/asm/assembler.h
> > > > > > @@ -383,13 +383,13 @@ alternative_endif
> > > > > >         sub     \tmp2, \tmp1, #1
> > > > > >         bic     \kaddr, \kaddr, \tmp2
> > > > > >   9998:
> > > > > > -       .if     (\op == cvau || \op == cvac)
> > > > > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > > > > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > > > > >         dc      \op, \kaddr
> > > > > >   alternative_else
> > > > > >         dc      civac, \kaddr
> > > > > >   alternative_endif
> > > > > > -       .elseif (\op == cvap)
> > > > > > +       .elseif ("\op" == "cvap")
> > > > > >   alternative_if ARM64_HAS_DCPOP
> > > > > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > > > > >   alternative_else
> > > > > >
> > > > >
> > > > > Looking at the GAS info pages, I find
> > > > >
> > > > > "Operators" are arithmetic functions, like '+' or '%'.
> > > > > "Arguments" are symbols, numbers or subexpressions.
> > > > > An "expression" specifies an address or numeric value.
> > > > >
> > > > > so even if the comparison works as expected, I'm hesitant to rely on
> > > > > it to work as expected on any version of GAS or any other assembler
> > > > > claiming to implement the GAS asm dialect.
> > > > >
> > > > > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> > > >
> > > > That looks better to me, although I'm not sure why you're inverted the logic
> > > > here:
> > > >
> > > > >     .ifnc \op, civac
> > > > >     .ifnc \op, cvap
> > > >
> > > > What am I missing?
> > > >
> > >
> > > .ifc does not permit '\op equals string1 or \op equals string2'
> >
> > Thanks. Then I guess we invert the logic as you suggest, or we duplicate the
> > alternative code. Looking at this some more, I think what we currently have
> > is broken because on a system with ARM64_WORKAROUND_CLEAN_CACHE but not
> > ARM64_HAS_DCPOP, you'll get DC CVAC for __clean_dcache_area_pop, which
> > would be broken on that CPU.
> >
> 
> Can we just fallback to civac instead? Or do we need to add logic to
> combine the two feature flags?

I guess this could introduce a performance regression for CPUs without
either DCPOP or ARM64_WORKAROUND_CLEAN_CACHE, since we're effectively
forcing a hefty cache miss on a subsequent access to the persisted data.

So I'd prefer not to make the CIVAC unconditional unless we have to.

Will

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Ard Biesheuvel]

On Thu, 6 Dec 2018 at 12:51, Will Deacon <will.deacon@arm.com> wrote:
>
> On Tue, Dec 04, 2018 at 01:44:01AM +0100, Ard Biesheuvel wrote:
> > On Mon, 3 Dec 2018 at 19:10, Will Deacon <will.deacon@arm.com> wrote:
> > >
> > > On Mon, Dec 03, 2018 at 06:54:35PM +0100, Ard Biesheuvel wrote:
> > > > On Mon, 3 Dec 2018 at 18:44, Will Deacon <will.deacon@arm.com> wrote:
> > > > >
> > > > > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:
> > > > > > On Mon, 3 Dec 2018 at 14:11, Robin Murphy <robin.murphy@arm.com> wrote:
> > > > > > > On 01/12/2018 11:01, Ard Biesheuvel wrote:
> > > > > > > > The GAS directives that are currently being used in dcache_by_line_op
> > > > > > > > rely on assembler behavior that is not documented, and probably not
> > > > > > > > guaranteed to produce the correct behavior going forward.
> > > > > > > >
> > > > > > > > Currently, we end up with some undefined symbols in cache.o:
> > > > > > > >
> > > > > > > > $ nm arch/arm64/mm/cache.o
> > > > > > > >        ...
> > > > > > > >        U civac
> > > > > > > >        ...
> > > > > > > >        U cvac
> > > > > > > >        U cvap
> > > > > > > >        U cvau
> > > > > > > >
> > > > > > > > This is due to the fact that the comparisons used to select the
> > > > > > > > operation type in the dcache_by_line_op macro are comparing symbols
> > > > > > > > not strings, and even though it seems that GAS is doing the right
> > > > > > > > thing here (undefined symbols by the same name are equal to each
> > > > > > > > other), it seems unwise to rely on this.
> > > > > > > >
> > > > > > > > So let's provide some definitions that are guaranteed to be distinct,
> > > > > > > > and make them local so they don't pollute the gobal symbol space.
> > > > > > >
> > > > > > > Rather than making the unintended symbol comparisons work properly, can
> > > > > > > we not just implement the string comparisons that were supposed to be?
> > > > > > > Superficially, the diff below seems to still generate the desired output
> > > > > > > (although as always there's probably some subtlety I'm missing).
> > > > > > >
> > > > > > > Robin.
> > > > > > >
> > > > > > > ----->8-----
> > > > > > >
> > > > > > > diff --git a/arch/arm64/include/asm/assembler.h
> > > > > > > b/arch/arm64/include/asm/assembler.h
> > > > > > > index 6142402c2eb4..2c5f4825fee3 100644
> > > > > > > --- a/arch/arm64/include/asm/assembler.h
> > > > > > > +++ b/arch/arm64/include/asm/assembler.h
> > > > > > > @@ -383,13 +383,13 @@ alternative_endif
> > > > > > >         sub     \tmp2, \tmp1, #1
> > > > > > >         bic     \kaddr, \kaddr, \tmp2
> > > > > > >   9998:
> > > > > > > -       .if     (\op == cvau || \op == cvac)
> > > > > > > +       .if     ("\op" == "cvau" || "\op" == "cvac")
> > > > > > >   alternative_if_not ARM64_WORKAROUND_CLEAN_CACHE
> > > > > > >         dc      \op, \kaddr
> > > > > > >   alternative_else
> > > > > > >         dc      civac, \kaddr
> > > > > > >   alternative_endif
> > > > > > > -       .elseif (\op == cvap)
> > > > > > > +       .elseif ("\op" == "cvap")
> > > > > > >   alternative_if ARM64_HAS_DCPOP
> > > > > > >         sys 3, c7, c12, 1, \kaddr       // dc cvap
> > > > > > >   alternative_else
> > > > > > >
> > > > > >
> > > > > > Looking at the GAS info pages, I find
> > > > > >
> > > > > > "Operators" are arithmetic functions, like '+' or '%'.
> > > > > > "Arguments" are symbols, numbers or subexpressions.
> > > > > > An "expression" specifies an address or numeric value.
> > > > > >
> > > > > > so even if the comparison works as expected, I'm hesitant to rely on
> > > > > > it to work as expected on any version of GAS or any other assembler
> > > > > > claiming to implement the GAS asm dialect.
> > > > > >
> > > > > > We could change the logic to .ifc, which is defined to operate on string, i.e.,
> > > > >
> > > > > That looks better to me, although I'm not sure why you're inverted the logic
> > > > > here:
> > > > >
> > > > > >     .ifnc \op, civac
> > > > > >     .ifnc \op, cvap
> > > > >
> > > > > What am I missing?
> > > > >
> > > >
> > > > .ifc does not permit '\op equals string1 or \op equals string2'
> > >
> > > Thanks. Then I guess we invert the logic as you suggest, or we duplicate the
> > > alternative code. Looking at this some more, I think what we currently have
> > > is broken because on a system with ARM64_WORKAROUND_CLEAN_CACHE but not
> > > ARM64_HAS_DCPOP, you'll get DC CVAC for __clean_dcache_area_pop, which
> > > would be broken on that CPU.
> > >
> >
> > Can we just fallback to civac instead? Or do we need to add logic to
> > combine the two feature flags?
>
> I guess this could introduce a performance regression for CPUs without
> either DCPOP or ARM64_WORKAROUND_CLEAN_CACHE, since we're effectively
> forcing a hefty cache miss on a subsequent access to the persisted data.
>
> So I'd prefer not to make the CIVAC unconditional unless we have to.
>

OK

I have some patches that extend alternative patching via callbacks for
this. I will send them out shortly.

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

Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S

[Dave Martin]

On Thu, Dec 06, 2018 at 12:47:16PM +0100, Ard Biesheuvel wrote:
> On Thu, 6 Dec 2018 at 12:20, Dave Martin <Dave.Martin@arm.com> wrote:
> >
> > On Mon, Dec 03, 2018 at 05:45:06PM +0000, Will Deacon wrote:
> > > On Mon, Dec 03, 2018 at 02:22:14PM +0100, Ard Biesheuvel wrote:

[...]

> > > That looks better to me, although I'm not sure why you're inverted the logic
> > > here:
> > >
> > > >     .ifnc \op, civac
> > > >     .ifnc \op, cvap
> > >
> > > What am I missing?
> >
> > I vote for the .ifc approach.
> >
> > Note, the current works-by-accident approach using == has the odd side-
> > effect of spitting out undefined symbol references in the .o file.  It
> > seems that isn't breaking the link, but I wonder whether there are any
> > side-effects we're not aware of.
> >
> 
> Did you read the commit log at all? :-)

Due to a combination of a mutt snafu and laziness, no.  (However did you
guess? ;)

> > If we don't like the inverted logic, there is always
> >
> >         .set .L__foo_\@, 0
> >         .ifc \op, civac
> >         .set .L__foo_\@, 1
> >         .endif
> >         .ifc \op, cvap
> >         .set .L__foo_\@, 1
> >         .endif
> >
> >         .if .L__foo_\@
> >         // ...
> >         .endif
> >
> > which is ugly.  Either way, the logic could be wrapped as a helper:
> >
> >         .macro if_string_is_either str, cmp1, cmp2, insn:vararg
> >                 .ifnc "\str","\cmp1"
> >                 .ifnc "\str","\cmp2"
> >                 .exitm
> >                 .endif
> >                 .endif
> >
> >                 \insn
> >         .endm
> >
> 
> Yeah, I don't think this is an improvement over using inverted logic.

As a single instance, no.  But if this issue comes up in multiple places
it could be worth wrapping it up.

Cheers
---Dave

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