* [PATCH] arm64/mm: use correct operators for string comparison in cache.S
@ 2018-12-01 11:01 Ard Biesheuvel
2018-12-03 13:11 ` Robin Murphy
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-01 11:01 UTC (permalink / raw)
To: linux-arm-kernel; +Cc: catalin.marinas, will.deacon, 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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-01 11:01 [PATCH] arm64/mm: use correct operators for string comparison in cache.S Ard Biesheuvel
@ 2018-12-03 13:11 ` Robin Murphy
2018-12-03 13:22 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Robin Murphy @ 2018-12-03 13:11 UTC (permalink / raw)
To: Ard Biesheuvel, linux-arm-kernel; +Cc: catalin.marinas, will.deacon
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
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 13:11 ` Robin Murphy
@ 2018-12-03 13:22 ` Ard Biesheuvel
2018-12-03 17:45 ` Will Deacon
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-03 13:22 UTC (permalink / raw)
To: Robin Murphy; +Cc: Catalin Marinas, Will Deacon, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 13:22 ` Ard Biesheuvel
@ 2018-12-03 17:45 ` Will Deacon
2018-12-03 17:54 ` Ard Biesheuvel
2018-12-06 11:20 ` Dave Martin
0 siblings, 2 replies; 12+ messages in thread
From: Will Deacon @ 2018-12-03 17:45 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 17:45 ` Will Deacon
@ 2018-12-03 17:54 ` Ard Biesheuvel
2018-12-03 18:11 ` Will Deacon
2018-12-06 11:20 ` Dave Martin
1 sibling, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-03 17:54 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 17:54 ` Ard Biesheuvel
@ 2018-12-03 18:11 ` Will Deacon
2018-12-04 0:44 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2018-12-03 18:11 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 18:11 ` Will Deacon
@ 2018-12-04 0:44 ` Ard Biesheuvel
2018-12-06 11:51 ` Will Deacon
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-04 0:44 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-03 17:45 ` Will Deacon
2018-12-03 17:54 ` Ard Biesheuvel
@ 2018-12-06 11:20 ` Dave Martin
2018-12-06 11:47 ` Ard Biesheuvel
1 sibling, 1 reply; 12+ messages in thread
From: Dave Martin @ 2018-12-06 11:20 UTC (permalink / raw)
To: Will Deacon
Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel, Ard Biesheuvel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-06 11:20 ` Dave Martin
@ 2018-12-06 11:47 ` Ard Biesheuvel
2018-12-06 12:02 ` Dave Martin
0 siblings, 1 reply; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 11:47 UTC (permalink / raw)
To: Dave Martin; +Cc: Catalin Marinas, Will Deacon, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-04 0:44 ` Ard Biesheuvel
@ 2018-12-06 11:51 ` Will Deacon
2018-12-06 11:59 ` Ard Biesheuvel
0 siblings, 1 reply; 12+ messages in thread
From: Will Deacon @ 2018-12-06 11:51 UTC (permalink / raw)
To: Ard Biesheuvel; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-06 11:51 ` Will Deacon
@ 2018-12-06 11:59 ` Ard Biesheuvel
0 siblings, 0 replies; 12+ messages in thread
From: Ard Biesheuvel @ 2018-12-06 11:59 UTC (permalink / raw)
To: Will Deacon; +Cc: Catalin Marinas, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
2018-12-06 11:47 ` Ard Biesheuvel
@ 2018-12-06 12:02 ` Dave Martin
0 siblings, 0 replies; 12+ messages in thread
From: Dave Martin @ 2018-12-06 12:02 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Catalin Marinas, Will Deacon, Robin Murphy, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2018-12-06 12:02 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-01 11:01 [PATCH] arm64/mm: use correct operators for string comparison in cache.S Ard Biesheuvel
2018-12-03 13:11 ` Robin Murphy
2018-12-03 13:22 ` Ard Biesheuvel
2018-12-03 17:45 ` Will Deacon
2018-12-03 17:54 ` Ard Biesheuvel
2018-12-03 18:11 ` Will Deacon
2018-12-04 0:44 ` Ard Biesheuvel
2018-12-06 11:51 ` Will Deacon
2018-12-06 11:59 ` Ard Biesheuvel
2018-12-06 11:20 ` Dave Martin
2018-12-06 11:47 ` Ard Biesheuvel
2018-12-06 12:02 ` Dave Martin
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.