From: Jessica Clarke <jrtc27@jrtc27.com> To: Greentime Hu <greentime.hu@sifive.com> Cc: Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, linux-riscv <linux-riscv@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Albert Ou <aou@eecs.berkeley.edu> Subject: Re: [PATCH v9 08/17] riscv: Add vector struct and assembler definitions Date: Fri, 7 Jan 2022 13:53:20 +0000 [thread overview] Message-ID: <FAC07911-491C-46F5-AA96-7C97BC5BD7F4@jrtc27.com> (raw) In-Reply-To: <CAHCEehJhMQf9jtVkTvorriv6xY5QnEDmN_JjM-UxcCj9UL86Lg@mail.gmail.com> On 7 Jan 2022, at 13:28, Greentime Hu <greentime.hu@sifive.com> wrote: > > Palmer Dabbelt <palmer@dabbelt.com> 於 2021年12月15日 週三 上午12:29寫道: >> >> On Tue, 09 Nov 2021 01:48:20 PST (-0800), greentime.hu@sifive.com wrote: >>> Add vector state context struct in struct thread and asm-offsets.c >>> definitions. >>> >>> The vector registers will be saved in datap pointer of __riscv_v_state. It >>> will be dynamically allocated in kernel space. It will be put right after >>> the __riscv_v_state data structure in user space. >>> >>> Co-developed-by: Vincent Chen <vincent.chen@sifive.com> >>> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >>> Signed-off-by: Greentime Hu <greentime.hu@sifive.com> >>> --- >>> arch/riscv/include/asm/processor.h | 1 + >>> arch/riscv/include/uapi/asm/ptrace.h | 11 +++++++++++ >>> arch/riscv/kernel/asm-offsets.c | 6 ++++++ >>> 3 files changed, 18 insertions(+) >>> >>> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h >>> index 46b492c78cbb..a268f1382e52 100644 >>> --- a/arch/riscv/include/asm/processor.h >>> +++ b/arch/riscv/include/asm/processor.h >>> @@ -35,6 +35,7 @@ struct thread_struct { >>> unsigned long s[12]; /* s[0]: frame pointer */ >>> struct __riscv_d_ext_state fstate; >>> unsigned long bad_cause; >>> + struct __riscv_v_state vstate; >>> }; >>> >>> /* Whitelist the fstate from the task_struct for hardened usercopy */ >>> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h >>> index 882547f6bd5c..bd3b8a710246 100644 >>> --- a/arch/riscv/include/uapi/asm/ptrace.h >>> +++ b/arch/riscv/include/uapi/asm/ptrace.h >>> @@ -77,6 +77,17 @@ union __riscv_fp_state { >>> struct __riscv_q_ext_state q; >>> }; >>> >>> +struct __riscv_v_state { >>> + unsigned long vstart; >>> + unsigned long vl; >>> + unsigned long vtype; >>> + unsigned long vcsr; >> >> Don't we also need vlen to adequately determine the vector state? >> Otherwise we're going to end up dropping some state when vl isn't vlmax, >> which IIUC isn't legal. > > Do you mean vlenb? Since it is a constant value, we don't need to > save/restore it in the context. > >>> + void *datap; >>> +#if __riscv_xlen == 32 >>> + __u32 __padding; >>> +#endif >> >> Why is there padding? > > To keep vector registers saved in a 16-bytes aligned address for rv32. That struct has an alignment of 4 bytes. It doesn’t make sense to put the padding there; it should be wherever the 16 byte alignment is introduced, which looks like your __sc_riscv_v_state below (assuming you need to explicitly name the padding and can’t just rely on implicit compiler padding; presumably you need it so you can guarantee it’s zero when written to userspace memory?). Especially since the amount of padding you need in __riscv_v_state if doing it this way depends on the size of __riscv_ctx_hdr, because that happens to be in __sc_riscv_v_state. This is quite fragile and non-obvious as it stands. Jess > struct __riscv_ctx_hdr { > __u32 magic; > __u32 size; > }; > struct __sc_riscv_v_state { > struct __riscv_ctx_hdr head; > struct __riscv_v_state v_state; > } __attribute__((aligned(16))); > > rv64 => 48bytes -> 16byte aligned > rv32 => 32bytes -> 16byte aligned > > This struct and vector registers will be copied to > sigcontext.reserved[] for signal handler so we'd like to keep it is > 16-byte aligned. > > struct sigcontext { > struct user_regs_struct sc_regs; > union __riscv_fp_state sc_fpregs; > /* > * 4K + 128 reserved for vector state and future expansion. > * This space is enough to store the vector context whose VLENB > * is less or equal to 128. > * (The size of the vector context is 4144 byte as VLENB is 128) > */ > __u8 __reserved[4224] __attribute__((__aligned__(16))); > }; > > >>> +}; >>> + >>> #endif /* __ASSEMBLY__ */ >>> >>> #endif /* _UAPI_ASM_RISCV_PTRACE_H */ >>> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c >>> index 90f8ce64fa6f..34f43c84723a 100644 >>> --- a/arch/riscv/kernel/asm-offsets.c >>> +++ b/arch/riscv/kernel/asm-offsets.c >>> @@ -72,6 +72,12 @@ void asm_offsets(void) >>> OFFSET(TSK_STACK_CANARY, task_struct, stack_canary); >>> #endif >>> >>> + OFFSET(RISCV_V_STATE_VSTART, __riscv_v_state, vstart); >>> + OFFSET(RISCV_V_STATE_VL, __riscv_v_state, vl); >>> + OFFSET(RISCV_V_STATE_VTYPE, __riscv_v_state, vtype); >>> + OFFSET(RISCV_V_STATE_VCSR, __riscv_v_state, vcsr); >>> + OFFSET(RISCV_V_STATE_DATAP, __riscv_v_state, datap); >>> + >>> DEFINE(PT_SIZE, sizeof(struct pt_regs)); >>> OFFSET(PT_EPC, pt_regs, epc); >>> OFFSET(PT_RA, pt_regs, ra); > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv
WARNING: multiple messages have this Message-ID (diff)
From: Jessica Clarke <jrtc27@jrtc27.com> To: Greentime Hu <greentime.hu@sifive.com> Cc: Palmer Dabbelt <palmer@dabbelt.com>, Paul Walmsley <paul.walmsley@sifive.com>, linux-riscv <linux-riscv@lists.infradead.org>, Linux Kernel Mailing List <linux-kernel@vger.kernel.org>, Albert Ou <aou@eecs.berkeley.edu> Subject: Re: [PATCH v9 08/17] riscv: Add vector struct and assembler definitions Date: Fri, 7 Jan 2022 13:53:20 +0000 [thread overview] Message-ID: <FAC07911-491C-46F5-AA96-7C97BC5BD7F4@jrtc27.com> (raw) In-Reply-To: <CAHCEehJhMQf9jtVkTvorriv6xY5QnEDmN_JjM-UxcCj9UL86Lg@mail.gmail.com> On 7 Jan 2022, at 13:28, Greentime Hu <greentime.hu@sifive.com> wrote: > > Palmer Dabbelt <palmer@dabbelt.com> 於 2021年12月15日 週三 上午12:29寫道: >> >> On Tue, 09 Nov 2021 01:48:20 PST (-0800), greentime.hu@sifive.com wrote: >>> Add vector state context struct in struct thread and asm-offsets.c >>> definitions. >>> >>> The vector registers will be saved in datap pointer of __riscv_v_state. It >>> will be dynamically allocated in kernel space. It will be put right after >>> the __riscv_v_state data structure in user space. >>> >>> Co-developed-by: Vincent Chen <vincent.chen@sifive.com> >>> Signed-off-by: Vincent Chen <vincent.chen@sifive.com> >>> Signed-off-by: Greentime Hu <greentime.hu@sifive.com> >>> --- >>> arch/riscv/include/asm/processor.h | 1 + >>> arch/riscv/include/uapi/asm/ptrace.h | 11 +++++++++++ >>> arch/riscv/kernel/asm-offsets.c | 6 ++++++ >>> 3 files changed, 18 insertions(+) >>> >>> diff --git a/arch/riscv/include/asm/processor.h b/arch/riscv/include/asm/processor.h >>> index 46b492c78cbb..a268f1382e52 100644 >>> --- a/arch/riscv/include/asm/processor.h >>> +++ b/arch/riscv/include/asm/processor.h >>> @@ -35,6 +35,7 @@ struct thread_struct { >>> unsigned long s[12]; /* s[0]: frame pointer */ >>> struct __riscv_d_ext_state fstate; >>> unsigned long bad_cause; >>> + struct __riscv_v_state vstate; >>> }; >>> >>> /* Whitelist the fstate from the task_struct for hardened usercopy */ >>> diff --git a/arch/riscv/include/uapi/asm/ptrace.h b/arch/riscv/include/uapi/asm/ptrace.h >>> index 882547f6bd5c..bd3b8a710246 100644 >>> --- a/arch/riscv/include/uapi/asm/ptrace.h >>> +++ b/arch/riscv/include/uapi/asm/ptrace.h >>> @@ -77,6 +77,17 @@ union __riscv_fp_state { >>> struct __riscv_q_ext_state q; >>> }; >>> >>> +struct __riscv_v_state { >>> + unsigned long vstart; >>> + unsigned long vl; >>> + unsigned long vtype; >>> + unsigned long vcsr; >> >> Don't we also need vlen to adequately determine the vector state? >> Otherwise we're going to end up dropping some state when vl isn't vlmax, >> which IIUC isn't legal. > > Do you mean vlenb? Since it is a constant value, we don't need to > save/restore it in the context. > >>> + void *datap; >>> +#if __riscv_xlen == 32 >>> + __u32 __padding; >>> +#endif >> >> Why is there padding? > > To keep vector registers saved in a 16-bytes aligned address for rv32. That struct has an alignment of 4 bytes. It doesn’t make sense to put the padding there; it should be wherever the 16 byte alignment is introduced, which looks like your __sc_riscv_v_state below (assuming you need to explicitly name the padding and can’t just rely on implicit compiler padding; presumably you need it so you can guarantee it’s zero when written to userspace memory?). Especially since the amount of padding you need in __riscv_v_state if doing it this way depends on the size of __riscv_ctx_hdr, because that happens to be in __sc_riscv_v_state. This is quite fragile and non-obvious as it stands. Jess > struct __riscv_ctx_hdr { > __u32 magic; > __u32 size; > }; > struct __sc_riscv_v_state { > struct __riscv_ctx_hdr head; > struct __riscv_v_state v_state; > } __attribute__((aligned(16))); > > rv64 => 48bytes -> 16byte aligned > rv32 => 32bytes -> 16byte aligned > > This struct and vector registers will be copied to > sigcontext.reserved[] for signal handler so we'd like to keep it is > 16-byte aligned. > > struct sigcontext { > struct user_regs_struct sc_regs; > union __riscv_fp_state sc_fpregs; > /* > * 4K + 128 reserved for vector state and future expansion. > * This space is enough to store the vector context whose VLENB > * is less or equal to 128. > * (The size of the vector context is 4144 byte as VLENB is 128) > */ > __u8 __reserved[4224] __attribute__((__aligned__(16))); > }; > > >>> +}; >>> + >>> #endif /* __ASSEMBLY__ */ >>> >>> #endif /* _UAPI_ASM_RISCV_PTRACE_H */ >>> diff --git a/arch/riscv/kernel/asm-offsets.c b/arch/riscv/kernel/asm-offsets.c >>> index 90f8ce64fa6f..34f43c84723a 100644 >>> --- a/arch/riscv/kernel/asm-offsets.c >>> +++ b/arch/riscv/kernel/asm-offsets.c >>> @@ -72,6 +72,12 @@ void asm_offsets(void) >>> OFFSET(TSK_STACK_CANARY, task_struct, stack_canary); >>> #endif >>> >>> + OFFSET(RISCV_V_STATE_VSTART, __riscv_v_state, vstart); >>> + OFFSET(RISCV_V_STATE_VL, __riscv_v_state, vl); >>> + OFFSET(RISCV_V_STATE_VTYPE, __riscv_v_state, vtype); >>> + OFFSET(RISCV_V_STATE_VCSR, __riscv_v_state, vcsr); >>> + OFFSET(RISCV_V_STATE_DATAP, __riscv_v_state, datap); >>> + >>> DEFINE(PT_SIZE, sizeof(struct pt_regs)); >>> OFFSET(PT_EPC, pt_regs, epc); >>> OFFSET(PT_RA, pt_regs, ra); > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2022-01-07 13:53 UTC|newest] Thread overview: 73+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-11-09 9:48 [PATCH v9 00/17] riscv: Add vector ISA support Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 01/17] riscv: Separate patch for cflags and aflags Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 02/17] riscv: Rename __switch_to_aux -> fpu Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 03/17] riscv: Extending cpufeature.c to detect V-extension Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 04/17] riscv: Add new csr defines related to vector extension Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 05/17] riscv: Add vector feature to compile Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-12 0:54 ` kernel test robot 2021-11-12 0:54 ` kernel test robot 2021-11-12 0:54 ` kernel test robot 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 06/17] riscv: Add has_vector/riscv_vsize to save vector features Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 07/17] riscv: Reset vector register Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2022-01-04 6:02 ` Greentime Hu 2022-01-04 6:02 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 08/17] riscv: Add vector struct and assembler definitions Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-12-14 16:29 ` Palmer Dabbelt 2021-12-14 16:29 ` Palmer Dabbelt 2022-01-07 13:28 ` Greentime Hu 2022-01-07 13:28 ` Greentime Hu 2022-01-07 13:53 ` Jessica Clarke [this message] 2022-01-07 13:53 ` Jessica Clarke 2021-11-09 9:48 ` [PATCH v9 09/17] riscv: Add task switch support for vector Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-11 13:49 ` kernel test robot 2021-11-11 13:49 ` kernel test robot 2021-11-11 13:49 ` kernel test robot 2021-11-11 19:16 ` kernel test robot 2021-11-11 19:16 ` kernel test robot 2021-11-11 19:16 ` kernel test robot 2021-12-15 18:38 ` Palmer Dabbelt 2021-12-15 18:38 ` Palmer Dabbelt 2021-11-09 9:48 ` [PATCH v9 10/17] riscv: Add ptrace vector support Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 11/17] riscv: Add sigcontext save/restore for vector Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 12/17] riscv: signal: Report signal frame size to userspace via auxv Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 13/17] riscv: Add support for kernel mode vector Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 14/17] riscv: Use CSR_STATUS to replace sstatus in vector.S Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 15/17] riscv: Add vector extension XOR implementation Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 16/17] riscv: Fix an illegal instruction exception when accessing vlenb without enable vector first Greentime Hu 2021-11-09 9:48 ` Greentime Hu 2021-11-10 10:38 ` Ben Dooks 2021-11-10 10:38 ` Ben Dooks 2021-11-16 13:14 ` Greentime Hu 2021-11-16 13:14 ` Greentime Hu 2021-11-09 9:48 ` [PATCH v9 17/17] riscv: Fix a kernel panic issue if $s2 is set to a specific value before entering Linux Greentime Hu 2021-11-09 9:48 ` Greentime Hu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=FAC07911-491C-46F5-AA96-7C97BC5BD7F4@jrtc27.com \ --to=jrtc27@jrtc27.com \ --cc=aou@eecs.berkeley.edu \ --cc=greentime.hu@sifive.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=palmer@dabbelt.com \ --cc=paul.walmsley@sifive.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.