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:
>
> > 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.
>
> > 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.
> > > + if (rdvl_sve() != vl)
> > > + ksft_exit_fail_msg("Set VL %d, RDVL %d\n",
> > > + vl, rdvl_sve());
>
> > If this fails, the VL vl wasn't "Set" at all. I found this a bit
> > confusing from just looking at this hunk.
>
> > Can we write something like "PR_SVE_SET_VL reports VL %d, RDVL %d"?
>
> Sure.
>
> > 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
