Re: [PATCH v7 1/2] arm64/sve: Split TIF_SVE into separate execute and register state flags

From: Dave Martin <Dave.Martin@arm.com>
To: Mark Brown <broonie@kernel.org>
Cc: Julien Grall <julien@xen.com>, Julien Grall <julien@xen.org>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Zhang Lei <zhang.lei@jp.fujitsu.com>,
	Will Deacon <will@kernel.org>,
	linux-arm-kernel@lists.infradead.org,
	Daniel Kiss <Daniel.Kiss@arm.com>
Subject: Re: [PATCH v7 1/2] arm64/sve: Split TIF_SVE into separate execute and register state flags
Date: Wed, 10 Feb 2021 18:15:30 +0000	[thread overview]
Message-ID: <20210210181530.GL21837@arm.com> (raw)
In-Reply-To: <20210210171442.GC4748@sirena.org.uk>

On Wed, Feb 10, 2021 at 05:14:42PM +0000, Mark Brown wrote:
> On Wed, Feb 10, 2021 at 03:42:51PM +0000, Dave Martin wrote:
> > On Wed, Feb 10, 2021 at 02:54:52PM +0000, Mark Brown wrote:
> 
> > > Oh, *that's* what that's all about.  I spent quite a bit of time trying
> > > to figure out why we were sometimes using vq_minus_1 but never managed
> > > to get to the bottom of it - it's an awkward name and there's nothing in
> > > the code that explains the logic behind when we use it so it was really
> > > confusing.  We can do the rename but I'm not sure it's achieving the
> > > goal of comprehensibility.
> 
> > Ah, I see.  The reason for the difference is that the vector length is
> > encoded in ZCR_ELx.LEN as the vector length in quadwords ("vq" -- see
> > Documentation/arm64/sve.rst) minus one.  It seemed poor practice to do
> > the conversion in asm where the compiler can't see or optimise it, plus
> > I didn't want the possibility of passing meaningless values at that
> > level.  So the caller has to validate the vector length with
> > sve_vl_valid() where deemed necessary, and then convert explicitly.
> 
> Yeah, it's relatively clear to get to the fact that it's due to the
> ZCR_ELx.LEN - what was not at all clear was what the rule for choosing
> between the two representations was, my instinct would've been to hide
> the different representation, something like a static inline wrapper for
> the assembly function would still let the compiler see what's going on.  

We could certainly do that.  One reason why I didn't go for that
initially is that the ZCR setting is done in low-level places where we
don't really want to have to BUG().  Requiring the caller to do
something explicit reduces the change of someone passing in an unchecked
garbage value for the vl.  But provided the caller checks with
sve_vl_valid(), or we are confident by construction that the value is
valid, this doesn't really matter.

Due to a lot of painful debugging, I became pretty paranoid when
upstreaming the initial code.  We should keep some of that paranoia, but
we probably don't need quite so much of it now.

> > Either way, calling this "vl" is breaking a useful convention that's
> > followed throughout the rest of the kernel, so I'd prefer we call it
> > something else -- but within reason, I don't mind what name is used.
> 
> That's the convention in the arm64 FP code or something else?

It's the convention I made up for the arm64 SVE code and user/kernel
API -- so when I say "the rest of the kernel", I just mean arch/arm64/.
The rest of the kernel doesn't contain any SVE code that doesn't follow
this convention, so I can claim it is followed everywhere ;)

In the architecture, "VL" is a looser concept that in most contexts
means something like "the size of a vector", but it can be a bit
unexpected -- as in the ", MUL VL" addressing mode syntax for example.

Cheers
---Dave

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