Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756508AbdGLLHz (ORCPT ); Wed, 12 Jul 2017 07:07:55 -0400 Received: from mailapp01.imgtec.com ([195.59.15.196]:61413 "EHLO imgpgp01.kl.imgtec.org" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756255AbdGLLHy (ORCPT ); Wed, 12 Jul 2017 07:07:54 -0400 X-PGP-Universal: processed; by imgpgp01.kl.imgtec.org on Wed, 12 Jul 2017 13:18:33 +0100 Date: Wed, 12 Jul 2017 12:07:51 +0100 From: James Hogan To: Palmer Dabbelt CC: Olof Johansson , Arnd Bergmann , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , , Subject: Re: [PATCH 16/17] RISC-V: User-facing API Message-ID: <20170712110751.GS6973@jhogan-linux.le.imgtec.org> References: <20170712013130.14792-1-palmer@dabbelt.com> <20170712013130.14792-17-palmer@dabbelt.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="XnddjV6rC62KGhww" Content-Disposition: inline In-Reply-To: <20170712013130.14792-17-palmer@dabbelt.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Originating-IP: [192.168.154.110] X-ESG-ENCRYPT-TAG: 1b7d744b Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4493 Lines: 123 --XnddjV6rC62KGhww Content-Type: text/plain; charset=utf-8 Content-Disposition: inline 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 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 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, ®s, 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? 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 --XnddjV6rC62KGhww Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEEd80NauSabkiESfLYbAtpk944dnoFAllmAv8ACgkQbAtpk944 dnrx3BAAt5eSiRBL+RMFQ+16xK8NPAs0pYIr8Z1Gv11h2mUXBDTd8lM4e3f5Vb7b WlDUOm8+hPqnfNccPDhc8rlOgI3VIfD7g3kZp2JttPGLNsFfeT6Eq14ypC51cVq+ STgzx/Hsfs9p7U7DAIxcVG0X2s7Le+xHbIWdB/Ab18aKAvRslAllr9PPJ+ouNtrO fp+0Tsq2BW+0QoseU75i74hPmdzeBkhSQ3c4wLKbQPGlqt4iDuk2Yq2F7dKMv/Nl FB0a9OblemxXJLXDRDkidh1AGn2XyFyT3QboaU4AbqRFqiFoj9SVZMq5QqZRFIYs e+0/QiBRBXMP/mgSubFjyxqRTFzrRs1jVCE4D4caWJdo9vAlwqudYzv5Gevq9YVa gBkkFa1LD+VM6BJwXdPXxSXj6XzG3ENvC4Xz2tWRMHQzeu0Ghv0udlLOLd0jjYWM gDylyFPH0bbevliD0DWBeR5udQPWMj75eOmbDiDvcDkX+WUl5e/pPjR50YUKUvWC mm3bf0XssfGu4Ld1m9tLPr6Sa9YzLCrRjG9yQ3Kn3NfFGmtO0azQI5P8k/cnhJzm 2EUik0QsVJFxUvXHxVMp8pCAZzDESIibWokywTbgt5N74RGDl/8wgRE4iO77L0uk BxnYlCIISpvuCKT58c9hAFz18hGvAdg8aYjuOZfrY5iNUw1TWbA= =BhXt -----END PGP SIGNATURE----- --XnddjV6rC62KGhww--