| Message ID | 20181201110144.15970-1-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64/mm: use correct operators for string comparison in cache.S | expand |
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 >
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 > >
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
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'
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
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?
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
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.
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
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.
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
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
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(-)