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.
Attachment:
signature.asc
Description: PGP signature
