On Wed, Oct 02, 2024 at 08:21:36AM +0200, Greg KH wrote: > On Wed, Oct 02, 2024 at 06:13:45AM +0200, Jason A. Donenfeld wrote: > > On Tue, Oct 01, 2024 at 09:29:45AM -0600, Shuah Khan wrote: > > > On 10/1/24 09:03, Jason A. Donenfeld wrote: > > > > On Tue, Oct 01, 2024 at 08:56:43AM -0600, Shuah Khan wrote: > > > >> On 10/1/24 08:45, Jason A. Donenfeld wrote: > > > >>> On Tue, Oct 01, 2024 at 08:43:05AM -0600, Shuah Khan wrote: > > > >>>> On 9/30/24 21:56, Jason A. Donenfeld wrote: > > > >>>>> This is not stable material and I didn't mark it as such. Do not backport. > > > >>>> > > > >>>> The way selftest work is they just skip if a feature isn't supported. > > > >>>> As such this test should run gracefully on stable releases. > > > >>>> > > > >>>> I would say backport unless and skip if the feature isn't supported. > > > >>> > > > >>> Nonsense. 6.11 never returns ENOSYS from vDSO. This doesn't make sense. > > > >> > > > >> Not sure what you mean by Nonsense. ENOSYS can be used to skip?? > > > > > > > > The branch that this patch adds will never be reached in 6.11 because > > > > the kernel does not have the corresponding code. > > > > > > What should/would happen if this test is run on a kernel that doesn't > > > support the feature? > > > > The build system doesn't compile it for kernels without the feature. > > > > That's not how the kselftests should be working. If you'd like to get involved in the development of these, by all means send patches. As you can see, for 6.12, these were intensely improved in all manner of ways: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/log/tools/testing/selftests/vDSO Just look at that flurry of activity. Things are getting better! And things were in pretty bad shape before. If you think there's an interesting subset of that for backporting, by all means go for it, but do it thoughtfully and don't pick patches willy-nilly. > They can run on any > kernel image (build is separate from running on many test systems), and > so they should just fail with whatever the "feature not present" error > is if the feature isn't present in the system-that-is-being-tested. So, it's actually not that clear what the best thing is. Firstly, for vdso_test_chacha.c, it can't even compile without the symlink and a resolving tools/arch/$(SRCARCH)/vdso/vgetrandom-chacha.S symlink, which is on a per-arch basis. You might say that in this case, it's best to condition the Makefile on `ifneq ($(wildcard tools/arch/$(SRCARCH)/vdso/ vgetrandom-chacha.S),)` instead of on $(ARCH), but there's this ugly wrinkle where some of the code that's being compiled is 64-bit only, and x86_64 and x86 share a $(SRCARCH) path. (That Makefile makes use of $(CONFIG_X86_32), which is pretty gross and might not work; I'm not yet sure how to fix that.) Christophe experimented with having the compiler decide, and then there being a runtime result, but it added a lot of complexity that didn't seem necessary. There's more experimentation to be done here. Meanwhile, part of vdso_test_getrandom.c's purpose is to test whether __kernel_getrandom() or __vdso_getrandom() is actually being properly exported from the vDSO. This is also interesting on powerpc, where it's implemented on both 32-bit and 64-bit, so there's the compat case to worry about. That in turn means that this test has in it: vgrnd.fn = (__typeof__(vgrnd.fn))vdso_sym(version, name); if (!vgrnd.fn) { printf("%s is missing!\n", name); exit(KSFT_FAIL); } And not exit(KSFT_SKIP), since that would hide the failure to export the symbol. Now, you could say that since development on the fundamental part is mostly concluded, we could move to a KSFT_SKIP, in order to simplify the build choice and such. I'm not sure where I stand on that. At the very least, there's still RISC-V coming down the pipeline for this feature, so it probably would change after that comes out. Anyway, that is all to say that this stuff has been thoroughly considered, not haphazardly glued together or something. Maybe that consideration has reached wrong conclusions -- that's an entirely possible of an outcome -- but it wouldn't be for lack of caring. If you'd like to contribute to it, I'd certainly welcome a hand. But please don't do the arm-chair coding thing. Meanwhile, this ENOSYS thing has nothing to do with what either of you assumed it does. This is to handle obscure/exotic arm64 hardware, which might not exist in the Linux world, that doesn't have NEON support. But since arm64 support for this function didn't even come to Linux 6.11, there's no point in discussing it as a backport. Jason
