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

[Palmer Dabbelt]

On Wed, 12 Jul 2017 10:09:55 PDT (-0700), james.ho...@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.ho...@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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
>> >> + /* Save the floating-point state. */
>> >> + err |= save_d_state(regs, >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_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.

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

[Palmer Dabbelt]

On Wed, 12 Jul 2017 10:09:55 PDT (-0700), james.ho...@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.ho...@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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
>> >> + /* Save the floating-point state. */
>> >> + err |= save_d_state(regs, >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_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.

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.ho...@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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
> >> +  /* Save the floating-point state. */
> >> +  err |= save_d_state(regs, >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_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


signature.asc
Description: Digital signature

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.ho...@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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
> >> +  /* Save the floating-point state. */
> >> +  err |= save_d_state(regs, >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_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


signature.asc
Description: Digital signature

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

[Palmer Dabbelt]

On Wed, 12 Jul 2017 04:07:51 PDT (-0700), james.ho...@imgtec.com 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 ..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 
>
> 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 ..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(, , , , 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(, , , , , 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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
>> +/* Save the floating-point state. */
>> +err |= save_d_state(regs, >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_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.

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

[Palmer Dabbelt]

On Wed, 12 Jul 2017 04:07:51 PDT (-0700), james.ho...@imgtec.com 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 ..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 
>
> 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 ..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(, , , , 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(, , , , , 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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
>> +/* Save the floating-point state. */
>> +err |= save_d_state(regs, >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_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.

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

[James Hogan]

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

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.

> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> new file mode 100644
> index ..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(, , , , 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(, , , , , 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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
> + /* Save the floating-point state. */
> + err |= save_d_state(regs, >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_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?

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.

Cheers
James


signature.asc
Description: Digital signature

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

[James Hogan]

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

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.

> diff --git a/arch/riscv/kernel/ptrace.c b/arch/riscv/kernel/ptrace.c
> new file mode 100644
> index ..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(, , , , 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(, , , , , 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 ..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 = >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_regs, regs, sizeof(sc->sc_regs));
> + /* Save the floating-point state. */
> + err |= save_d_state(regs, >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_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?

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.

Cheers
James


signature.asc
Description: Digital signature

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

[Palmer Dabbelt]

This patch contains code that is in some way visible to the user:
including via system calls, the VDSO, module loading and signal
handling.  It also contains some generic code that is ABI visible.

Signed-off-by: Palmer Dabbelt 
---
 arch/riscv/include/asm/mmu.h  |  26 +++
 arch/riscv/include/asm/ptrace.h   | 116 
 arch/riscv/include/asm/syscall.h  |  90 ++
 arch/riscv/include/asm/unistd.h   |  16 ++
 arch/riscv/include/asm/vdso.h |  32 
 arch/riscv/include/uapi/asm/Kbuild|   5 +
 arch/riscv/include/uapi/asm/auxvec.h  |  24 +++
 arch/riscv/include/uapi/asm/bitsperlong.h |  25 +++
 arch/riscv/include/uapi/asm/byteorder.h   |  23 +++
 arch/riscv/include/uapi/asm/elf.h |  83 +
 arch/riscv/include/uapi/asm/hwcap.h   |  36 
 arch/riscv/include/uapi/asm/ptrace.h  |  90 ++
 arch/riscv/include/uapi/asm/sigcontext.h  |  30 
 arch/riscv/include/uapi/asm/siginfo.h |  24 +++
 arch/riscv/include/uapi/asm/ucontext.h|  35 
 arch/riscv/kernel/cpufeature.c|  61 +++
 arch/riscv/kernel/module.c| 217 ++
 arch/riscv/kernel/ptrace.c| 125 +
 arch/riscv/kernel/riscv_ksyms.c   |  15 ++
 arch/riscv/kernel/signal.c| 289 ++
 arch/riscv/kernel/sys_riscv.c |  49 +
 arch/riscv/kernel/syscall_table.c |  25 +++
 arch/riscv/kernel/vdso/.gitignore |   2 +
 arch/riscv/kernel/vdso/Makefile   |  64 +++
 arch/riscv/kernel/vdso/rt_sigreturn.S |  24 +++
 arch/riscv/kernel/vdso/vdso.S |  27 +++
 arch/riscv/kernel/vdso/vdso.lds.S |  77 
 27 files changed, 1630 insertions(+)
 create mode 100644 arch/riscv/include/asm/mmu.h
 create mode 100644 arch/riscv/include/asm/ptrace.h
 create mode 100644 arch/riscv/include/asm/syscall.h
 create mode 100644 arch/riscv/include/asm/unistd.h
 create mode 100644 arch/riscv/include/asm/vdso.h
 create mode 100644 arch/riscv/include/uapi/asm/Kbuild
 create mode 100644 arch/riscv/include/uapi/asm/auxvec.h
 create mode 100644 arch/riscv/include/uapi/asm/bitsperlong.h
 create mode 100644 arch/riscv/include/uapi/asm/byteorder.h
 create mode 100644 arch/riscv/include/uapi/asm/elf.h
 create mode 100644 arch/riscv/include/uapi/asm/hwcap.h
 create mode 100644 arch/riscv/include/uapi/asm/ptrace.h
 create mode 100644 arch/riscv/include/uapi/asm/sigcontext.h
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h
 create mode 100644 arch/riscv/include/uapi/asm/ucontext.h
 create mode 100644 arch/riscv/kernel/cpufeature.c
 create mode 100644 arch/riscv/kernel/module.c
 create mode 100644 arch/riscv/kernel/ptrace.c
 create mode 100644 arch/riscv/kernel/riscv_ksyms.c
 create mode 100644 arch/riscv/kernel/signal.c
 create mode 100644 arch/riscv/kernel/sys_riscv.c
 create mode 100644 arch/riscv/kernel/syscall_table.c
 create mode 100644 arch/riscv/kernel/vdso/.gitignore
 create mode 100644 arch/riscv/kernel/vdso/Makefile
 create mode 100644 arch/riscv/kernel/vdso/rt_sigreturn.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S

diff --git a/arch/riscv/include/asm/mmu.h b/arch/riscv/include/asm/mmu.h
new file mode 100644
index ..66805cba9a27
--- /dev/null
+++ b/arch/riscv/include/asm/mmu.h
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+
+#ifndef _ASM_RISCV_MMU_H
+#define _ASM_RISCV_MMU_H
+
+#ifndef __ASSEMBLY__
+
+typedef struct {
+   void *vdso;
+} mm_context_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_MMU_H */
diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
new file mode 100644
index ..340201868842
--- /dev/null
+++ b/arch/riscv/include/asm/ptrace.h
@@ -0,0 +1,116 @@
+/*
+ * 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.
+ */
+
+#ifndef _ASM_RISCV_PTRACE_H
+#define _ASM_RISCV_PTRACE_H
+

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

[Palmer Dabbelt]

This patch contains code that is in some way visible to the user:
including via system calls, the VDSO, module loading and signal
handling.  It also contains some generic code that is ABI visible.

Signed-off-by: Palmer Dabbelt 
---
 arch/riscv/include/asm/mmu.h  |  26 +++
 arch/riscv/include/asm/ptrace.h   | 116 
 arch/riscv/include/asm/syscall.h  |  90 ++
 arch/riscv/include/asm/unistd.h   |  16 ++
 arch/riscv/include/asm/vdso.h |  32 
 arch/riscv/include/uapi/asm/Kbuild|   5 +
 arch/riscv/include/uapi/asm/auxvec.h  |  24 +++
 arch/riscv/include/uapi/asm/bitsperlong.h |  25 +++
 arch/riscv/include/uapi/asm/byteorder.h   |  23 +++
 arch/riscv/include/uapi/asm/elf.h |  83 +
 arch/riscv/include/uapi/asm/hwcap.h   |  36 
 arch/riscv/include/uapi/asm/ptrace.h  |  90 ++
 arch/riscv/include/uapi/asm/sigcontext.h  |  30 
 arch/riscv/include/uapi/asm/siginfo.h |  24 +++
 arch/riscv/include/uapi/asm/ucontext.h|  35 
 arch/riscv/kernel/cpufeature.c|  61 +++
 arch/riscv/kernel/module.c| 217 ++
 arch/riscv/kernel/ptrace.c| 125 +
 arch/riscv/kernel/riscv_ksyms.c   |  15 ++
 arch/riscv/kernel/signal.c| 289 ++
 arch/riscv/kernel/sys_riscv.c |  49 +
 arch/riscv/kernel/syscall_table.c |  25 +++
 arch/riscv/kernel/vdso/.gitignore |   2 +
 arch/riscv/kernel/vdso/Makefile   |  64 +++
 arch/riscv/kernel/vdso/rt_sigreturn.S |  24 +++
 arch/riscv/kernel/vdso/vdso.S |  27 +++
 arch/riscv/kernel/vdso/vdso.lds.S |  77 
 27 files changed, 1630 insertions(+)
 create mode 100644 arch/riscv/include/asm/mmu.h
 create mode 100644 arch/riscv/include/asm/ptrace.h
 create mode 100644 arch/riscv/include/asm/syscall.h
 create mode 100644 arch/riscv/include/asm/unistd.h
 create mode 100644 arch/riscv/include/asm/vdso.h
 create mode 100644 arch/riscv/include/uapi/asm/Kbuild
 create mode 100644 arch/riscv/include/uapi/asm/auxvec.h
 create mode 100644 arch/riscv/include/uapi/asm/bitsperlong.h
 create mode 100644 arch/riscv/include/uapi/asm/byteorder.h
 create mode 100644 arch/riscv/include/uapi/asm/elf.h
 create mode 100644 arch/riscv/include/uapi/asm/hwcap.h
 create mode 100644 arch/riscv/include/uapi/asm/ptrace.h
 create mode 100644 arch/riscv/include/uapi/asm/sigcontext.h
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h
 create mode 100644 arch/riscv/include/uapi/asm/ucontext.h
 create mode 100644 arch/riscv/kernel/cpufeature.c
 create mode 100644 arch/riscv/kernel/module.c
 create mode 100644 arch/riscv/kernel/ptrace.c
 create mode 100644 arch/riscv/kernel/riscv_ksyms.c
 create mode 100644 arch/riscv/kernel/signal.c
 create mode 100644 arch/riscv/kernel/sys_riscv.c
 create mode 100644 arch/riscv/kernel/syscall_table.c
 create mode 100644 arch/riscv/kernel/vdso/.gitignore
 create mode 100644 arch/riscv/kernel/vdso/Makefile
 create mode 100644 arch/riscv/kernel/vdso/rt_sigreturn.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S

diff --git a/arch/riscv/include/asm/mmu.h b/arch/riscv/include/asm/mmu.h
new file mode 100644
index ..66805cba9a27
--- /dev/null
+++ b/arch/riscv/include/asm/mmu.h
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+
+#ifndef _ASM_RISCV_MMU_H
+#define _ASM_RISCV_MMU_H
+
+#ifndef __ASSEMBLY__
+
+typedef struct {
+   void *vdso;
+} mm_context_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_MMU_H */
diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
new file mode 100644
index ..340201868842
--- /dev/null
+++ b/arch/riscv/include/asm/ptrace.h
@@ -0,0 +1,116 @@
+/*
+ * 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.
+ */
+
+#ifndef _ASM_RISCV_PTRACE_H
+#define _ASM_RISCV_PTRACE_H
+
+#include 

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

[Palmer Dabbelt]

On Tue, 11 Jul 2017 07:01:32 PDT (-0700), james.ho...@imgtec.com wrote:
> Hi Christoph,
>
> On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
>> > +#ifdef CONFIG_64BIT
>> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
>> > +  unsigned long, prot, unsigned long, flags,
>> > +  unsigned long, fd, off_t, offset)
>> > +{
>> > +  if (unlikely(offset & (~PAGE_MASK)))
>> > +  return -EINVAL;
>> > +  return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
>> > +}
>> > +#else
>> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>> > +  unsigned long, prot, unsigned long, flags,
>> > +  unsigned long, fd, off_t, offset)
>> > +{
>> > +  /*
>> > +   * Note that the shift for mmap2 is constant (12),
>> > +   * regardless of PAGE_SIZE
>> > +   */
>> > +  if (unlikely(offset & (~PAGE_MASK >> 12)))
>> > +  return -EINVAL;
>> > +  return sys_mmap_pgoff(addr, len, prot, flags, fd,
>> > +  offset >> (PAGE_SHIFT - 12));
>> > +}
>> > +#endif /* !CONFIG_64BIT */
>>
>> Most modern ports seem to expose sys_mmap_pgoff as the
>> syscall directly.  Any reason you're doing this differently?
>
> I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
> is only really correct on 32-bit arches where the only page size is 4k.
> If other page sizes are supported then this is the correct way to handle
> it as the page offset from 32-bit userland is supposed to be in 4k
> units.
>
> 64-bit doesn't need to worry about squeezing big file offsets into the
> off_t offset so don't need to do the shift at all.
>
> See the mmap2 man page. It says "the final argument specifies the offset
> into the file in 4096-byte units", and it points out ia64 as an
> exception where it depends on the page size of the system.
>
>>
>> But even the code for the older ones should probably be consolidated..
>
> Quite probably, yes.

This looks like what arm64 does, though I'm OK either way.  Here's my attempt
at consolidating the code, even though there isn't a lot to help with:

  diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
  index e18fc0ebdd91..4351be7d0533 100644
  --- a/arch/riscv/kernel/sys_riscv.c
  +++ b/arch/riscv/kernel/sys_riscv.c
  @@ -17,14 +17,23 @@
   #include 
   #include 

  +static long riscv_sys_mmap(unsigned long addr, unsigned long len,
  +  unsigned long prot, unsigned long flags,
  +  unsigned long fd, off_t offset,
  +  unsigned long page_shift_offset)
  +{
  +   if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
  +   return -EINVAL;
  +   return sys_mmap_pgoff(addr, len, prot, flags, fd,
  + offset >> (PAGE_SHIFT - page_shift_offset));
  +}
  +
   #ifdef CONFIG_64BIT
   SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
  unsigned long, prot, unsigned long, flags,
  unsigned long, fd, off_t, offset)
   {
  -   if (unlikely(offset & (~PAGE_MASK)))
  -   return -EINVAL;
  -   return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> 
PAGE_SHIFT);
  +   return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 0);
   }
   #else
   SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
  @@ -35,9 +44,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, 
len,
   * Note that the shift for mmap2 is constant (12),
   * regardless of PAGE_SIZE
   */
  -   if (unlikely(offset & (~PAGE_MASK >> 12)))
  -   return -EINVAL;
  -   return sys_mmap_pgoff(addr, len, prot, flags, fd,
  -   offset >> (PAGE_SHIFT - 12));
  +   return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 12);
   }
   #endif /* !CONFIG_64BIT */

I'll submit this as part of our v6, which will hopefully be coming out soon.

Thanks!

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

[Palmer Dabbelt]

On Tue, 11 Jul 2017 07:01:32 PDT (-0700), james.ho...@imgtec.com wrote:
> Hi Christoph,
>
> On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
>> > +#ifdef CONFIG_64BIT
>> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
>> > +  unsigned long, prot, unsigned long, flags,
>> > +  unsigned long, fd, off_t, offset)
>> > +{
>> > +  if (unlikely(offset & (~PAGE_MASK)))
>> > +  return -EINVAL;
>> > +  return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
>> > +}
>> > +#else
>> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
>> > +  unsigned long, prot, unsigned long, flags,
>> > +  unsigned long, fd, off_t, offset)
>> > +{
>> > +  /*
>> > +   * Note that the shift for mmap2 is constant (12),
>> > +   * regardless of PAGE_SIZE
>> > +   */
>> > +  if (unlikely(offset & (~PAGE_MASK >> 12)))
>> > +  return -EINVAL;
>> > +  return sys_mmap_pgoff(addr, len, prot, flags, fd,
>> > +  offset >> (PAGE_SHIFT - 12));
>> > +}
>> > +#endif /* !CONFIG_64BIT */
>>
>> Most modern ports seem to expose sys_mmap_pgoff as the
>> syscall directly.  Any reason you're doing this differently?
>
> I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
> is only really correct on 32-bit arches where the only page size is 4k.
> If other page sizes are supported then this is the correct way to handle
> it as the page offset from 32-bit userland is supposed to be in 4k
> units.
>
> 64-bit doesn't need to worry about squeezing big file offsets into the
> off_t offset so don't need to do the shift at all.
>
> See the mmap2 man page. It says "the final argument specifies the offset
> into the file in 4096-byte units", and it points out ia64 as an
> exception where it depends on the page size of the system.
>
>>
>> But even the code for the older ones should probably be consolidated..
>
> Quite probably, yes.

This looks like what arm64 does, though I'm OK either way.  Here's my attempt
at consolidating the code, even though there isn't a lot to help with:

  diff --git a/arch/riscv/kernel/sys_riscv.c b/arch/riscv/kernel/sys_riscv.c
  index e18fc0ebdd91..4351be7d0533 100644
  --- a/arch/riscv/kernel/sys_riscv.c
  +++ b/arch/riscv/kernel/sys_riscv.c
  @@ -17,14 +17,23 @@
   #include 
   #include 

  +static long riscv_sys_mmap(unsigned long addr, unsigned long len,
  +  unsigned long prot, unsigned long flags,
  +  unsigned long fd, off_t offset,
  +  unsigned long page_shift_offset)
  +{
  +   if (unlikely(offset & (~PAGE_MASK >> page_shift_offset)))
  +   return -EINVAL;
  +   return sys_mmap_pgoff(addr, len, prot, flags, fd,
  + offset >> (PAGE_SHIFT - page_shift_offset));
  +}
  +
   #ifdef CONFIG_64BIT
   SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
  unsigned long, prot, unsigned long, flags,
  unsigned long, fd, off_t, offset)
   {
  -   if (unlikely(offset & (~PAGE_MASK)))
  -   return -EINVAL;
  -   return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> 
PAGE_SHIFT);
  +   return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 0);
   }
   #else
   SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
  @@ -35,9 +44,6 @@ SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, 
len,
   * Note that the shift for mmap2 is constant (12),
   * regardless of PAGE_SIZE
   */
  -   if (unlikely(offset & (~PAGE_MASK >> 12)))
  -   return -EINVAL;
  -   return sys_mmap_pgoff(addr, len, prot, flags, fd,
  -   offset >> (PAGE_SHIFT - 12));
  +   return riscv_sys_mmap(addr, len, prot, flags, fd, offset, 12);
   }
   #endif /* !CONFIG_64BIT */

I'll submit this as part of our v6, which will hopefully be coming out soon.

Thanks!

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

[James Hogan]

Hi Christoph,

On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
> > +#ifdef CONFIG_64BIT
> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> > +   unsigned long, prot, unsigned long, flags,
> > +   unsigned long, fd, off_t, offset)
> > +{
> > +   if (unlikely(offset & (~PAGE_MASK)))
> > +   return -EINVAL;
> > +   return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> > +}
> > +#else
> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> > +   unsigned long, prot, unsigned long, flags,
> > +   unsigned long, fd, off_t, offset)
> > +{
> > +   /*
> > +* Note that the shift for mmap2 is constant (12),
> > +* regardless of PAGE_SIZE
> > +*/
> > +   if (unlikely(offset & (~PAGE_MASK >> 12)))
> > +   return -EINVAL;
> > +   return sys_mmap_pgoff(addr, len, prot, flags, fd,
> > +   offset >> (PAGE_SHIFT - 12));
> > +}
> > +#endif /* !CONFIG_64BIT */
> 
> Most modern ports seem to expose sys_mmap_pgoff as the
> syscall directly.  Any reason you're doing this differently?

I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
is only really correct on 32-bit arches where the only page size is 4k.
If other page sizes are supported then this is the correct way to handle
it as the page offset from 32-bit userland is supposed to be in 4k
units.

64-bit doesn't need to worry about squeezing big file offsets into the
off_t offset so don't need to do the shift at all.

See the mmap2 man page. It says "the final argument specifies the offset
into the file in 4096-byte units", and it points out ia64 as an
exception where it depends on the page size of the system.

> 
> But even the code for the older ones should probably be consolidated..

Quite probably, yes.

Cheers
James


signature.asc
Description: Digital signature

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

[James Hogan]

Hi Christoph,

On Tue, Jul 11, 2017 at 06:39:48AM -0700, Christoph Hellwig wrote:
> > +#ifdef CONFIG_64BIT
> > +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> > +   unsigned long, prot, unsigned long, flags,
> > +   unsigned long, fd, off_t, offset)
> > +{
> > +   if (unlikely(offset & (~PAGE_MASK)))
> > +   return -EINVAL;
> > +   return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> > +}
> > +#else
> > +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> > +   unsigned long, prot, unsigned long, flags,
> > +   unsigned long, fd, off_t, offset)
> > +{
> > +   /*
> > +* Note that the shift for mmap2 is constant (12),
> > +* regardless of PAGE_SIZE
> > +*/
> > +   if (unlikely(offset & (~PAGE_MASK >> 12)))
> > +   return -EINVAL;
> > +   return sys_mmap_pgoff(addr, len, prot, flags, fd,
> > +   offset >> (PAGE_SHIFT - 12));
> > +}
> > +#endif /* !CONFIG_64BIT */
> 
> Most modern ports seem to expose sys_mmap_pgoff as the
> syscall directly.  Any reason you're doing this differently?

I think Palmer's patch is probably correct here. Exposing sys_mmap_pgoff
is only really correct on 32-bit arches where the only page size is 4k.
If other page sizes are supported then this is the correct way to handle
it as the page offset from 32-bit userland is supposed to be in 4k
units.

64-bit doesn't need to worry about squeezing big file offsets into the
off_t offset so don't need to do the shift at all.

See the mmap2 man page. It says "the final argument specifies the offset
into the file in 4096-byte units", and it points out ia64 as an
exception where it depends on the page size of the system.

> 
> But even the code for the older ones should probably be consolidated..

Quite probably, yes.

Cheers
James


signature.asc
Description: Digital signature

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

[Christoph Hellwig]

> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2014 Darius Rad 
> + * Copyright (C) 2017 SiFive
> + *
> + *   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.
> + */
> +
> +#include 
> +#include 

Should not be needed.

> +#ifdef CONFIG_64BIT
> +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, off_t, offset)
> +{
> + if (unlikely(offset & (~PAGE_MASK)))
> + return -EINVAL;
> + return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> +}
> +#else
> +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, off_t, offset)
> +{
> + /*
> +  * Note that the shift for mmap2 is constant (12),
> +  * regardless of PAGE_SIZE
> +  */
> + if (unlikely(offset & (~PAGE_MASK >> 12)))
> + return -EINVAL;
> + return sys_mmap_pgoff(addr, len, prot, flags, fd,
> + offset >> (PAGE_SHIFT - 12));
> +}
> +#endif /* !CONFIG_64BIT */

Most modern ports seem to expose sys_mmap_pgoff as the
syscall directly.  Any reason you're doing this differently?

But even the code for the older ones should probably be consolidated..

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

[Christoph Hellwig]

> +++ b/arch/riscv/kernel/sys_riscv.c
> @@ -0,0 +1,43 @@
> +/*
> + * Copyright (C) 2012 Regents of the University of California
> + * Copyright (C) 2014 Darius Rad 
> + * Copyright (C) 2017 SiFive
> + *
> + *   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.
> + */
> +
> +#include 
> +#include 

Should not be needed.

> +#ifdef CONFIG_64BIT
> +SYSCALL_DEFINE6(mmap, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, off_t, offset)
> +{
> + if (unlikely(offset & (~PAGE_MASK)))
> + return -EINVAL;
> + return sys_mmap_pgoff(addr, len, prot, flags, fd, offset >> PAGE_SHIFT);
> +}
> +#else
> +SYSCALL_DEFINE6(mmap2, unsigned long, addr, unsigned long, len,
> + unsigned long, prot, unsigned long, flags,
> + unsigned long, fd, off_t, offset)
> +{
> + /*
> +  * Note that the shift for mmap2 is constant (12),
> +  * regardless of PAGE_SIZE
> +  */
> + if (unlikely(offset & (~PAGE_MASK >> 12)))
> + return -EINVAL;
> + return sys_mmap_pgoff(addr, len, prot, flags, fd,
> + offset >> (PAGE_SHIFT - 12));
> +}
> +#endif /* !CONFIG_64BIT */

Most modern ports seem to expose sys_mmap_pgoff as the
syscall directly.  Any reason you're doing this differently?

But even the code for the older ones should probably be consolidated..

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

[Palmer Dabbelt]

This patch contains code that is in some way visible to the user:
including via system calls, the VDSO, module loading and signal
handling.  It also contains some generic code that is ABI visible.

Signed-off-by: Palmer Dabbelt 
---
 arch/riscv/include/asm/mmu.h  |  26 +++
 arch/riscv/include/asm/ptrace.h   | 116 
 arch/riscv/include/asm/syscall.h  |  90 ++
 arch/riscv/include/asm/unistd.h   |  16 ++
 arch/riscv/include/asm/vdso.h |  32 
 arch/riscv/include/uapi/asm/Kbuild|   5 +
 arch/riscv/include/uapi/asm/auxvec.h  |  24 +++
 arch/riscv/include/uapi/asm/bitsperlong.h |  25 +++
 arch/riscv/include/uapi/asm/byteorder.h   |  23 +++
 arch/riscv/include/uapi/asm/elf.h |  83 +
 arch/riscv/include/uapi/asm/hwcap.h   |  36 
 arch/riscv/include/uapi/asm/ptrace.h  |  90 ++
 arch/riscv/include/uapi/asm/sigcontext.h  |  30 
 arch/riscv/include/uapi/asm/siginfo.h |  24 +++
 arch/riscv/include/uapi/asm/ucontext.h|  35 
 arch/riscv/kernel/cpufeature.c|  61 +++
 arch/riscv/kernel/module.c| 217 ++
 arch/riscv/kernel/ptrace.c| 125 +
 arch/riscv/kernel/riscv_ksyms.c   |  15 ++
 arch/riscv/kernel/signal.c| 289 ++
 arch/riscv/kernel/sys_riscv.c |  43 +
 arch/riscv/kernel/syscall_table.c |  25 +++
 arch/riscv/kernel/vdso/.gitignore |   2 +
 arch/riscv/kernel/vdso/Makefile   |  64 +++
 arch/riscv/kernel/vdso/rt_sigreturn.S |  24 +++
 arch/riscv/kernel/vdso/vdso.S |  27 +++
 arch/riscv/kernel/vdso/vdso.lds.S |  77 
 27 files changed, 1624 insertions(+)
 create mode 100644 arch/riscv/include/asm/mmu.h
 create mode 100644 arch/riscv/include/asm/ptrace.h
 create mode 100644 arch/riscv/include/asm/syscall.h
 create mode 100644 arch/riscv/include/asm/unistd.h
 create mode 100644 arch/riscv/include/asm/vdso.h
 create mode 100644 arch/riscv/include/uapi/asm/Kbuild
 create mode 100644 arch/riscv/include/uapi/asm/auxvec.h
 create mode 100644 arch/riscv/include/uapi/asm/bitsperlong.h
 create mode 100644 arch/riscv/include/uapi/asm/byteorder.h
 create mode 100644 arch/riscv/include/uapi/asm/elf.h
 create mode 100644 arch/riscv/include/uapi/asm/hwcap.h
 create mode 100644 arch/riscv/include/uapi/asm/ptrace.h
 create mode 100644 arch/riscv/include/uapi/asm/sigcontext.h
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h
 create mode 100644 arch/riscv/include/uapi/asm/ucontext.h
 create mode 100644 arch/riscv/kernel/cpufeature.c
 create mode 100644 arch/riscv/kernel/module.c
 create mode 100644 arch/riscv/kernel/ptrace.c
 create mode 100644 arch/riscv/kernel/riscv_ksyms.c
 create mode 100644 arch/riscv/kernel/signal.c
 create mode 100644 arch/riscv/kernel/sys_riscv.c
 create mode 100644 arch/riscv/kernel/syscall_table.c
 create mode 100644 arch/riscv/kernel/vdso/.gitignore
 create mode 100644 arch/riscv/kernel/vdso/Makefile
 create mode 100644 arch/riscv/kernel/vdso/rt_sigreturn.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S

diff --git a/arch/riscv/include/asm/mmu.h b/arch/riscv/include/asm/mmu.h
new file mode 100644
index ..66805cba9a27
--- /dev/null
+++ b/arch/riscv/include/asm/mmu.h
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+
+#ifndef _ASM_RISCV_MMU_H
+#define _ASM_RISCV_MMU_H
+
+#ifndef __ASSEMBLY__
+
+typedef struct {
+   void *vdso;
+} mm_context_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_MMU_H */
diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
new file mode 100644
index ..340201868842
--- /dev/null
+++ b/arch/riscv/include/asm/ptrace.h
@@ -0,0 +1,116 @@
+/*
+ * 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.
+ */
+
+#ifndef _ASM_RISCV_PTRACE_H
+#define _ASM_RISCV_PTRACE_H
+

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

[Palmer Dabbelt]

This patch contains code that is in some way visible to the user:
including via system calls, the VDSO, module loading and signal
handling.  It also contains some generic code that is ABI visible.

Signed-off-by: Palmer Dabbelt 
---
 arch/riscv/include/asm/mmu.h  |  26 +++
 arch/riscv/include/asm/ptrace.h   | 116 
 arch/riscv/include/asm/syscall.h  |  90 ++
 arch/riscv/include/asm/unistd.h   |  16 ++
 arch/riscv/include/asm/vdso.h |  32 
 arch/riscv/include/uapi/asm/Kbuild|   5 +
 arch/riscv/include/uapi/asm/auxvec.h  |  24 +++
 arch/riscv/include/uapi/asm/bitsperlong.h |  25 +++
 arch/riscv/include/uapi/asm/byteorder.h   |  23 +++
 arch/riscv/include/uapi/asm/elf.h |  83 +
 arch/riscv/include/uapi/asm/hwcap.h   |  36 
 arch/riscv/include/uapi/asm/ptrace.h  |  90 ++
 arch/riscv/include/uapi/asm/sigcontext.h  |  30 
 arch/riscv/include/uapi/asm/siginfo.h |  24 +++
 arch/riscv/include/uapi/asm/ucontext.h|  35 
 arch/riscv/kernel/cpufeature.c|  61 +++
 arch/riscv/kernel/module.c| 217 ++
 arch/riscv/kernel/ptrace.c| 125 +
 arch/riscv/kernel/riscv_ksyms.c   |  15 ++
 arch/riscv/kernel/signal.c| 289 ++
 arch/riscv/kernel/sys_riscv.c |  43 +
 arch/riscv/kernel/syscall_table.c |  25 +++
 arch/riscv/kernel/vdso/.gitignore |   2 +
 arch/riscv/kernel/vdso/Makefile   |  64 +++
 arch/riscv/kernel/vdso/rt_sigreturn.S |  24 +++
 arch/riscv/kernel/vdso/vdso.S |  27 +++
 arch/riscv/kernel/vdso/vdso.lds.S |  77 
 27 files changed, 1624 insertions(+)
 create mode 100644 arch/riscv/include/asm/mmu.h
 create mode 100644 arch/riscv/include/asm/ptrace.h
 create mode 100644 arch/riscv/include/asm/syscall.h
 create mode 100644 arch/riscv/include/asm/unistd.h
 create mode 100644 arch/riscv/include/asm/vdso.h
 create mode 100644 arch/riscv/include/uapi/asm/Kbuild
 create mode 100644 arch/riscv/include/uapi/asm/auxvec.h
 create mode 100644 arch/riscv/include/uapi/asm/bitsperlong.h
 create mode 100644 arch/riscv/include/uapi/asm/byteorder.h
 create mode 100644 arch/riscv/include/uapi/asm/elf.h
 create mode 100644 arch/riscv/include/uapi/asm/hwcap.h
 create mode 100644 arch/riscv/include/uapi/asm/ptrace.h
 create mode 100644 arch/riscv/include/uapi/asm/sigcontext.h
 create mode 100644 arch/riscv/include/uapi/asm/siginfo.h
 create mode 100644 arch/riscv/include/uapi/asm/ucontext.h
 create mode 100644 arch/riscv/kernel/cpufeature.c
 create mode 100644 arch/riscv/kernel/module.c
 create mode 100644 arch/riscv/kernel/ptrace.c
 create mode 100644 arch/riscv/kernel/riscv_ksyms.c
 create mode 100644 arch/riscv/kernel/signal.c
 create mode 100644 arch/riscv/kernel/sys_riscv.c
 create mode 100644 arch/riscv/kernel/syscall_table.c
 create mode 100644 arch/riscv/kernel/vdso/.gitignore
 create mode 100644 arch/riscv/kernel/vdso/Makefile
 create mode 100644 arch/riscv/kernel/vdso/rt_sigreturn.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.S
 create mode 100644 arch/riscv/kernel/vdso/vdso.lds.S

diff --git a/arch/riscv/include/asm/mmu.h b/arch/riscv/include/asm/mmu.h
new file mode 100644
index ..66805cba9a27
--- /dev/null
+++ b/arch/riscv/include/asm/mmu.h
@@ -0,0 +1,26 @@
+/*
+ * 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.
+ */
+
+
+#ifndef _ASM_RISCV_MMU_H
+#define _ASM_RISCV_MMU_H
+
+#ifndef __ASSEMBLY__
+
+typedef struct {
+   void *vdso;
+} mm_context_t;
+
+#endif /* __ASSEMBLY__ */
+
+#endif /* _ASM_RISCV_MMU_H */
diff --git a/arch/riscv/include/asm/ptrace.h b/arch/riscv/include/asm/ptrace.h
new file mode 100644
index ..340201868842
--- /dev/null
+++ b/arch/riscv/include/asm/ptrace.h
@@ -0,0 +1,116 @@
+/*
+ * 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.
+ */
+
+#ifndef _ASM_RISCV_PTRACE_H
+#define _ASM_RISCV_PTRACE_H
+
+#include