Re: [PATCH 16/17] RISC-V: User-facing API

[James Hogan]

On Wed, Jul 12, 2017 at 09:24:24AM -0700, Palmer Dabbelt wrote:
> On Wed, 12 Jul 2017 04:07:51 PDT (-0700), james.hogan@xxxxxxxxxx wrote:
> > On Tue, Jul 11, 2017 at 06:31:29PM -0700, Palmer Dabbelt wrote:
> >> diff --git a/arch/riscv/kernel/signal.c b/arch/riscv/kernel/signal.c
> >> new file mode 100644
> >> index 000000000000..e0a1b89583ef
> >> --- /dev/null
> >> +++ b/arch/riscv/kernel/signal.c
> >> @@ -0,0 +1,289 @@
> >
> >> +static long setup_sigcontext(struct rt_sigframe __user *frame,
> >> +	struct pt_regs *regs)
> >> +{
> >> +	struct sigcontext __user *sc = &frame->uc.uc_mcontext;
> >> +	long err;
> >> +	size_t i;
> >> +	/* sc_regs is structured the same as the start of pt_regs */
> >> +	err = __copy_to_user(&sc->sc_regs, regs, sizeof(sc->sc_regs));
> >> +	/* Save the floating-point state. */
> >> +	err |= save_d_state(regs, &sc->sc_fpregs.d);
> >> +	/* We support no other extension state at this time. */
> >> +	for (i = 0; i < ARRAY_SIZE(sc->sc_fpregs.q.reserved); i++)
> >> +		err |= __put_user(0, &sc->sc_fpregs.q.reserved[i]);
> >
> > How should userland determine how to interpret sc_fpregs? It looks like
> > you couldn't add f or q state without using one of these reserved
> > fields, so why not just specify a field up front to say which fp format
> > (if any) to interpret?
> 
> We considered that, but didn't want to tie ourserves to an extension mechanism
> right now because we don't know what the vector extension is going to look
> like.
> 
> > That would allow userland wanting to interpret it to safely check that
> > field in a forward and backward compatible way without assuming a
> > specific format is in use.
> 
> We set ELF_HWCAP (which percolates to userspace via the auxvec.  This contains
> the entire set of extensions the kernel supports on the current machine, which
> allows userspace to figure out what the format of the floating-point state is.

But then (as far as I understand it) software written now could break
once support for that extension is made available and the format
suddenly changes (or to avoid that breakage you may need to split up
vector values, which is not what the current union describes). Wouldn't
it be better to define it now in such a way that you hopefully don't
need to worry about such ABI breakage in future?

E.g. does it make sense to have the fp state as an fcsr and an array of
32 unions, each of which can contain a 32bit, 64-bit, or 128-bit
quantity. That assumes the vector state aliases the FP state, such that
an FP program on a kernel with vector extensions continues to work, but
a program using vector extensions can use the same sigcontext sensibly.

Thats how the MIPS SIMD Architecture (MSA) would ideally have worked,
but there wasn't space in the fpregs fields, so the upper 64-bits of
each vector register needed to be added separately in the sigcontext as
an extension, but the lower 64-bits (aliasing FP state) remaining in the
fpregs array.

Alternatively if even larger vector extensions are expected it might
make sense to abstract further and specify the stride between fp
registers as another field so it can be made larger in future without
breaking software that properly uses the stride, but admitedly that adds
complexity.

Cheers
James

Attachment: signature.asc
Description: Digital signature