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 15:42:51 +0000	[thread overview]
Message-ID: <20210210154249.GK21837@arm.com> (raw)
In-Reply-To: <20210210145452.GA4748@sirena.org.uk>

On Wed, Feb 10, 2021 at 02:54:52PM +0000, Mark Brown wrote:
> On Wed, Feb 10, 2021 at 10:56:55AM +0000, Dave Martin wrote:
> > On Mon, Feb 01, 2021 at 12:29:00PM +0000, Mark Brown wrote:
> 
> > > +	if (test_thread_flag(TIF_SVE_EXEC)) {
> > > +		vl = sve_vq_from_vl(current->thread.sve_vl) - 1;
> 
> > One more nit: because of the confusion that can arises from "vl" being a
> > somewhat overloaded term in the architecture, I was trying to avoid
> > using the name "vl" for anything that isn't the vector length in bytes.
> 
> > Can this instead be renamed to vq_minus_1 to match the function
> > arguments it's passed for?
> 
> 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.

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.

> 
> > (You could save a couple of lines by moving the declaration here and
> > combining it with this assignment too.)
> 
> Not really the coding style in the file though.

I'm not sure there's really a rigid convention in this file, other than
keeping declarations at the start of blocks.  I tend to push
declarations down when it doesn't harm readability -- i.e., when the
function is more than a screenful and/or the relevant block already has
braces enclosing several lines.  But it's a moot point.

Cheers
---Dave

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