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

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:

quoted

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.

quoted

+#include <stddef.h>
+#include <stdio.h>
+#include <stdlib.h>

Not used? ^

We call exit() which is declared in stdlib.h.

quoted

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

quoted

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

quoted

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

quoted

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