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

From: Will Deacon <will.deacon@arm.com>
To: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Robin Murphy <robin.murphy@arm.com>,
	linux-arm-kernel <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH] arm64/mm: use correct operators for string comparison in cache.S
Date: Mon, 3 Dec 2018 17:45:06 +0000	[thread overview]
Message-ID: <20181203174506.GA28497@arm.com> (raw)
In-Reply-To: <CAKv+Gu88W79sMoCFse4iaXwr=N1JT73-htvmGSQkVHdP1DaAYA@mail.gmail.com>

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