| Message ID | 15d09938180ee45bc5481c4a2d41ad656ca23c82.1636362169.git.greentime.hu@sifive.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: Add vector ISA support | expand |
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. > + void *datap; > +#if __riscv_xlen == 32 > + __u32 __padding; > +#endif Why is there padding? > +}; > + > #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);
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. 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);
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
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; + void *datap; +#if __riscv_xlen == 32 + __u32 __padding; +#endif +}; + #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);