Re: [PATCH v1 2/3] kselftest/arm64: Validate vector lengths are set in sve-probe-vls

On Wed, Jul 28, 2021 at 12:07:01PM +0100, Mark Brown wrote:

On Wed, Jul 28, 2021 at 10:41:51AM +0100, Dave Martin wrote:

quoted

This test was originally a diagnostic tool, so the way VLs are probed
aims to be efficient, rather than being defensive against the kernel
doing weird stuff.

Yeah, the whole probing thing doesn't really fit into kselftest's idea
of what a test is - I just put this in here since it seemed like a cheap
extra validation that we could add in with little bother rather than
because it's a complete and thorough validation of every possible thing
that could go wrong.  I'd be just as happy to not modify this at all but
since it does try the intermediate VLs it didn't seem like a terrible
idea to throw in some validation while we're at it.

quoted

If the kernel returns a vl > than the one we tried to set, we'll end
up in an infinite loop because of the way the loop deliberately uses the
kernel's return value to skip unsupported VLs.  So, it might be better
to probe every single architecturally possible VL and sanity check
everything instead.

Yup, that'd obviously be a complete rewrite though.  We'd not only need
to probe every possible vector length, but also validate that any
unsupported vector lengths report the expected vector length instead
when we try them.

Fair enough, but is it worth dropping a comment in to clarify what this
does and doesn't test?  This could be an area for future improvement.

quoted

quoted

+		if (rdvl_sve() != vl)
+			ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
+					   vl, rdvl_sve());

quoted

If this fails, the VL vl wasn't "Set" at all.  I found this a bit
confusing from just looking at this hunk.

quoted

Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?

Sure.

quoted

I'm not sure of the correct option for forcing SVE off against the
compiler default -- check with the tools folks.  If there isn't a
proper way to do this, it's a toolchain defect so we should flag it up,
but -mgeneral-regs-only might work for us even though it's a bit of a
sledgehammer.

-march should do the trick (if it doesn't the base kernel already has
issues).  This is a separate issue that affects all the arm64 selftests
I think.

OK, if there's already precedent then I guess we can go with that.

Cheers
---Dave

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