Re: [PATCH v4 3/8] arm64/fpsimdmacros: Allow the macro "for" to be used in more cases

From: Dave Martin <Dave.Martin@arm.com>
To: Will Deacon <will@kernel.org>
Cc: Julien Grall <julien@xen.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Zhang Lei <zhang.lei@jp.fujitsu.com>,
	Julien Grall <julien.grall@arm.com>,
	Mark Brown <broonie@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Daniel Kiss <Daniel.Kiss@arm.com>
Subject: Re: [PATCH v4 3/8] arm64/fpsimdmacros: Allow the macro "for" to be used in more cases
Date: Mon, 21 Sep 2020 17:53:32 +0100	[thread overview]
Message-ID: <20200921165330.GB6642@arm.com> (raw)
In-Reply-To: <20200921123802.GG2139@willie-the-truck>

On Mon, Sep 21, 2020 at 01:38:03PM +0100, Will Deacon wrote:
> On Fri, Aug 28, 2020 at 07:11:50PM +0100, Mark Brown wrote:
> > From: Julien Grall <julien.grall@arm.com>
> > 
> > The current version of the macro "for" is not able to work when the
> > counter is used to generate registers using mnemonics. This is because
> > gas is not able to evaluate the expression generated if used in
> > register's name (i.e x\n).
> > 
> > Gas offers a way to evaluate macro arguments by using % in front of
> > them under the alternate macro mode.
> 
> altmacro mode doesn't appear to be very widely used in the kernel at all,
> so I'm a bit nervous about this.

Note, altmacro is ancient and doesn't seem to have changed in living
memory, so I'd say it's stable.

I think the idea with this patch was to make sure that .altmacro is
only in effect for the internals of _for: it gets turned off again
before expanding any other macro, and at the end.

> > The implementation of "for" is updated to use the alternate macro mode
> > and %, so we can use the macro in more cases. As the alternate macro
> > mode may have side-effects, this is disabled when expanding the body.
> > 
> > While it is enough to prefix the argument of the macro "__for_body"
> > with %, the arguments of "__for" are also prefixed to get a more
> > bearable value in case of compilation error.
> > 
> > Suggested-by: Dave Martin <dave.martin@arm.com>
> > Signed-off-by: Julien Grall <julien.grall@arm.com>
> > Reviewed-by: Dave Martin <Dave.Martin@arm.com>
> > Signed-off-by: Mark Brown <broonie@kernel.org>
> > ---
> >  arch/arm64/include/asm/fpsimdmacros.h | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/arch/arm64/include/asm/fpsimdmacros.h b/arch/arm64/include/asm/fpsimdmacros.h
> > index 636e9d9c7929..75293f111a6b 100644
> > --- a/arch/arm64/include/asm/fpsimdmacros.h
> > +++ b/arch/arm64/include/asm/fpsimdmacros.h
> > @@ -166,19 +166,23 @@
> >  
> >  .macro __for from:req, to:req
> >  	.if (\from) == (\to)
> > -		_for__body \from
> > +		_for__body %\from
> >  	.else
> > -		__for \from, (\from) + ((\to) - (\from)) / 2
> > -		__for (\from) + ((\to) - (\from)) / 2 + 1, \to
> > +		__for %\from, %((\from) + ((\to) - (\from)) / 2)
> > +		__for %((\from) + ((\to) - (\from)) / 2 + 1), %\to
> >  	.endif
> >  .endm
> >  
> >  .macro _for var:req, from:req, to:req, insn:vararg
> >  	.macro _for__body \var:req
> > +		.noaltmacro
> >  		\insn
> > +		.altmacro
> 
> Why do we need to disable alt macro mode here?

From memory, I think this was down to the principle of least surprise:
\insn may itself be a macro call, and throughout the kernel macros
either don't care what the macro expansion mode is, or expect non-
altmacro mode.

So this sticks to that convention so that we don't leak altmacro mode
into code that isn't expecting it.

> >  	.endm
> >  
> > +	.altmacro
> >  	__for \from, \to
> > +	.noaltmacro
> 
> Why do we enable it here, rather than in the __for macro itself?

Again from memory, I think the mode takes effect when a macro is
expanded.  If .altmacro was put inside __for, the macro may already have
been expanded (in non-altmacro mode) before the .altmacro directive
takes effect.

Cheers
---Dave

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