Messages in this thread | | | Subject | Re: [PATCH v9 08/17] riscv: Add vector struct and assembler definitions | From | Jessica Clarke <> | Date | Fri, 7 Jan 2022 13:53:20 +0000 |
| |
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
| |