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

[Palmer Dabbelt]

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/include/asm/unistd.h b/arch/riscv/include/asm/unistd.h
>> new file mode 100644
>> index 000000000000..9f250ed007cd
>> --- /dev/null
>> +++ b/arch/riscv/include/asm/unistd.h
>> @@ -0,0 +1,16 @@
>> +/*
>> + * Copyright (C) 2012 Regents of the University of California
>> + *
>> + *   This program is free software; you can redistribute it and/or
>> + *   modify it under the terms of the GNU General Public License
>> + *   as published by the Free Software Foundation, version 2.
>> + *
>> + *   This program is distributed in the hope that it will be useful,
>> + *   but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + *   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + *   GNU General Public License for more details.
>> + */
>> +
>> +#define __ARCH_HAVE_MMU
>> +#define __ARCH_WANT_SYS_CLONE
>> +#include <uapi/asm/unistd.h>
>
> It might be worth keeping arch/risc/include/uapi/asm/unistd.h around,
> even if it only includes asm-generic/unistd.h, as it'll only get added
> again the next time a syscall is deprecated from the default list in
> order to add the appropriate __ARCH_WANT_RENAMEAT-like define, but yeh
> no big deal.

That makes sense, but since it's gone I'll juts add it later -- I figure it's
always better to have less code when possible :).

>
>> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
>> new file mode 100644
>> index 000000000000..ba3e80712797
>> --- /dev/null
>> +++ b/arch/riscv/kernel/ptrace.c
>> @@ -0,0 +1,125 @@
>
>> +static int riscv_gpr_get(struct task_struct *target,
>> +			 const struct user_regset *regset,
>> +			 unsigned int pos, unsigned int count,
>> +			 void *kbuf, void __user *ubuf)
>> +{
>> +	struct pt_regs *regs;
>> +
>> +	regs = task_pt_regs(target);
>> +	return user_regset_copyout(&pos, &count, &kbuf, &ubuf, regs, 0, -1);
>> +}
>> +
>> +static int riscv_gpr_set(struct task_struct *target,
>> +			 const struct user_regset *regset,
>> +			 unsigned int pos, unsigned int count,
>> +			 const void *kbuf, const void __user *ubuf)
>> +{
>> +	int ret;
>> +	struct pt_regs *regs;
>> +
>> +	regs = task_pt_regs(target);
>> +	ret = user_regset_copyin(&pos, &count, &kbuf, &ubuf, &regs, 0, -1);
>> +	return ret;
>> +}
>
> This is looking much safer now (the caller at least seems to always
> check pos + count is in range).
>
>> 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.