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.