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

[Will Deacon <will.deacon@arm.com>]

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