Date: Thu, 13 Jul 2017 14:50:11 -0700 (PDT)
Subject: Re: [PATCH 16/17] RISC-V: User-facing API
In-Reply-To: <20170712170955.GU6973@jhogan-linux.le.imgtec.org>
CC: Olof Johansson <olof@lixom.net>, Arnd Bergmann <arnd@arndb.de>,
        akpm@linux-foundation.org, albert@sifive.com,
        yamada.masahiro@socionext.com, mmarek@suse.com, will.deacon@arm.com,
        peterz@infradead.org, boqun.feng@gmail.com, mingo@redhat.com,
        daniel.lezcano@linaro.org, tglx@linutronix.de, jason@lakedaemon.net,
        marc.zyngier@arm.com, gregkh@linuxfoundation.org, jslaby@suse.com,
        davem@davemloft.net, mchehab@kernel.org, sfr@canb.auug.org.au,
        fweisbec@gmail.com, viro@zeniv.linux.org.uk, mcgrof@kernel.org,
        dledford@redhat.com, bart.vanassche@sandisk.com,
        sstabellini@kernel.org, daniel.vetter@ffwll.ch, mpe@ellerman.id.au,
        msalter@redhat.com, nicolas.dichtel@6wind.com,
        paul.gortmaker@windriver.com, linux@roeck-us.net,
        heiko.carstens@de.ibm.com, schwidefsky@de.ibm.com,
        linux-kernel@vger.kernel.org, patches@groups.riscv.org
From: Palmer Dabbelt <palmer@dabbelt.com>
To: james.hogan@imgtec.com
Message-ID: <mhng-2618fd68-d253-47b6-b19f-8c404537cab9@palmer-si-x1c4>
Mime-Version: 1.0 (MHng)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3731
Lines: 73

On Wed, 12 Jul 2017 10:09:55 PDT (-0700), james.hogan@imgtec.com wrote:
> 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@imgtec.com 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.

We considered the strided vesion, but it imposes a cost on the common case:
extra cache lines will be pulled in on D systems.

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

The V extension won't alias with the state of the F, D, and Q extensions (which
do alias each other).  We're planning on adding a whole extra block to the end
of sigcontext that contains the V extension state.