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.

> > +#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.

> > +/* 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.

> > +/* Can we read back a VL from prctl? */
> 
> It's certainly possible.

The comment is describing what the test is verifying.

> 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.

> 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.

> Also, we should probably disable the default vector length writing tests
> if not running with EUID==0.  Verifying that writing the default vector

kselftest in general isn't going to have a great time run as non-root
but yes, it wouldn't hurt to check.

> length fails when non-root would be worth testing.  If running as root,
> a temporary seteuid(nobody_uid) could be used for that.

This feels like another thing that could be done incrementally.