Re: [PATCH v2 1/4] kselftest/arm64: Provide a helper binary and "library" for SVE RDVL

On Thu, Jul 29, 2021 at 12:22:17PM +0100, Mark Brown wrote:

On Thu, Jul 29, 2021 at 10:52:24AM +0100, Dave Martin wrote:

quoted

On Wed, Jul 28, 2021 at 05:33:15PM +0100, Mark Brown wrote:

quoted

quoted

+	return 0;

quoted

For consistency with the changes in vec-syscfg, we could use
EXIT_SUCCESS here.

0 and EXIT_SUCCESS are defined as equivalent (though they need not be
equal!) and 0 is much more idiomatic.

Well, it's a moot point, I guess.  It just seems a little inconsistent
now, but it's no big deal.

I would probably be equally happy if you called the child failure status
in vec-syscfg "1".

quoted

Although it's hard to see what could go wrong I/O-wise that doesn't
involve vec-syscfg itself having gone wrong, it's probably good
practice to do the final error check:

quoted

	if (ferror(stdout) || fclose(stdout))
		return EXIT_FAILURE;

quoted

	return EXIT_SUCCESS;

quoted

(In reality, people rarely seem to bother with this, so I'm not going
to lose sleep if we don't do it...)

Yeah, I think this is one of those raising more questions than it
answers kind of things.

Fair enough.  This is the kind of thing it may be better to do in the
library or framework, since even if people remember to do it, they are
unlikely to do consistent things...

quoted

quoted

+.globl rdvl_sve

quoted

Should we stick a

quoted

	.type rdvl_sve, @function

quoted

here?  This may avoid surprises with future toolchain behaviours.
Probably doesn't matter, but I have bad memories of Thumb-2...

quoted

Lacking this annotation is widespread though, as well as being de facto
standard before awkward architectures came along.

Yeah, it doesn't seem to be in the slightest bit idiomatic for the arm64
asm code the kernel has.  I don't know if you think it's worth adding
that to SYM_FUNC_START now we have it though?

Actually, I think the core definition of SYM_FUNC_END() in
<linux/linkage.h> does this.

It would be good to pick up the common linkage macros; if we have to
sprinkle .type manually all over the tests people will likely make
mistakes, to that's probably not worth it.

If picking up the macros isn't trivial to do, I guess it's not that
important at this stage.

quoted

If the selftests have access to the ENTRY() macro we could use that, but
I'm guessing that isn't exported for userspace.

We don't use that any more anyway, it's SYM_FUNC_START() and friends not

Yes, I was forgetting about the macros that replaced ENTRY().  It's a
while since I wrote much asm code in the kernel...

that those are outside the kernel either.  We will have to do something
like that if anyone starts building userspace with BTI though (or I
might just shove a BTI C in there unconditionally, I'm sure we'll cope
with the overhead on older systems).

I thought about that, but that .S file isn't annotated as supporting
BTI, so I guess there's no problem for now(?)

It would be harmless to add it, of course.

Cheers
---Dave

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel