Re: [PATCH v1 3/3] kselftest/arm64: Add tests for SVE vector configuration

[Dave Martin <Dave.Martin@arm.com>]

On Wed, Jul 28, 2021 at 01:59:18PM +0100, Mark Brown wrote:
> On Wed, Jul 28, 2021 at 10:41:58AM +0100, Dave Martin wrote:
> > On Tue, Jul 27, 2021 at 07:06:49PM +0100, Mark Brown wrote:
> 
> > > We provide interfaces for configuring the SVE vector length seen by
> > > processes using prctl and also via /proc for configuring the default
> > > values. Provide tests that exercise all these interfaces and verify that
> > > they take effect as expected, though at present no test fully enumerates
> > > all the possible vector lengths.
> 
> > Does "at present" mean that this test doesn't do it either?
> 
> > (It doesn't seem to try every VL, unless I've missed something?  Is this
> > planned?)
> 
> Nothing currently does it, and nor does this patch.  Planned is a strong
> term but yes, ideally we should probe all the VLs.
> 
> > > +#include <stddef.h>
> > > +#include <stdio.h>
> > > +#include <stdlib.h>
> > 
> > Not used? ^
> 
> We call exit() which is declared in stdlib.h.

Ignore me, I confused exit() with _exit().

> > > +#define MIN_VL 16
> 
> > <asm/sigcontext.h> has SVE_MIN_VL.  Maybe we can use that everywhere
> > these days?
> 
> I partly wanted the vector type neutral name, and I'm considering
> modifying the sysfs ABI file to define 0 as a valid vector length for
> consistency with accepting -1 as the maximum since SME doesn't have any
> architected guarantees as to which particular vector lengths are defined.

I see what you mean, but it is more than mere coincidence that this is
the same value as SVE_MIN_VL.

You could view SVE as defining the base architecture which SME then
extends.

Perhaps

#define ARCH_MIN_VL SVE_MIN_VL	/* architectural minimim VL */

would be neutral enough.  Anyway, I won't lose sleep over it.

> > > +/* Verify that we can write a minimum value and have it take effect */
> > > +void proc_write_min(struct vec_data *data)
> 
> > Could be proc_write_check_min() (though the "check" is a bit more
> > redundant here; from "write" it's clear that this function actually
> > does something nontrivial).
> 
> TBH I'm not sure people will be excssively confused by the idea that a
> test would validate the values it was trying to read or write were
> accurate.

It's not 100% clear that this is a test until one has read all the way
to the end of the file.  But now that I understand the pattern here I
wouldn't be too concerned, and the comment accurately describes what the
function does.

> 
> > > +/* Can we read back a VL from prctl? */
> > 
> > It's certainly possible.
> 
> The comment is describing what the test is verifying.

Ignore that, I somehow read the comment as something like

	[TODO] Can we read back a VL via ptrace?

which is not what the comment says.

> > Since this would test different kernel paths from getting the child
> > itself to do RVDL / PR_SVE_GET_VL, it would be a different test though.
> > I think this diff is still good, but beefing up the ptrace tests to do
> > the appropriate checks would be good too (if we don't have that already).
> 
> Yes, the ptrace stuff could have a bit more coverage.
> 
> > > +	proc_write_min,
> > > +	proc_write_max,
> 
> > Can we also check what happens when writing unsupported values here?
> 
> We could.
> 
> > If this patch is more about establishing the framework, these could be
> > TODOs for now.
> 
> It definitely feels like something we can do incrementally.

Is it worth committing a TODO list somewhere?  There's always the
possibility that someone else gets interested and contributes some tests
for us; otherwise, at least it makes it harder to forget them.

> > Can we be a good citizen and restore sve_default_vector_length to its
> > original value?
> 
> We do that in the tests that fiddle with the default vector length, it
> seems useful to keep it at a value different from min and max as much as
> possible to increase the chance that we notice a failure to set things.

Ah right, I hadn't understood that proc_read_default() reads the default
and then the subsequent tests write it back.

This might be a bit clearer if the setup code was clearly separate from
the tests, but so long as the ordering requirements are clearly
documented that seems reasonably OK.

In:

> +test_type tests[] = {
> +	/*
> +	 * The default/min/max tests must be first to provide data for
> +	 * other tests.
> +	 */
> +	proc_read_default,
> +	proc_write_min,
> +	proc_write_max,

can you also comment that proc_read_default needs to come first among
these?

> +
> +	prctl_get,
> +	prctl_set,
> +	prctl_set_no_child,
> +	prctl_set_for_child,
> +	prctl_set_onexec,
> +};

[...]

Cheers
---Dave