| Message ID | 00ecc26833738377003ad21603c198ae4278cfd3.1674226563.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV basic exception handling implementation | expand |
On 20/01/2023 2:59 pm, Oleksii Kurochko wrote: > diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h > new file mode 100644 > index 0000000000..5898a09ce6 > --- /dev/null > +++ b/xen/arch/riscv/include/asm/processor.h > @@ -0,0 +1,114 @@ > +/* SPDX-License-Identifier: MIT */ > +/****************************************************************************** > + * > + * Copyright 2019 (C) Alistair Francis <alistair.francis@wdc.com> > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> > + * Copyright 2023 (C) Vates > + * > + */ > + > +#ifndef _ASM_RISCV_PROCESSOR_H > +#define _ASM_RISCV_PROCESSOR_H > + > +#include <asm/types.h> > + > +#define RISCV_CPU_USER_REGS_zero 0 > +#define RISCV_CPU_USER_REGS_ra 1 > +#define RISCV_CPU_USER_REGS_sp 2 > +#define RISCV_CPU_USER_REGS_gp 3 > +#define RISCV_CPU_USER_REGS_tp 4 > +#define RISCV_CPU_USER_REGS_t0 5 > +#define RISCV_CPU_USER_REGS_t1 6 > +#define RISCV_CPU_USER_REGS_t2 7 > +#define RISCV_CPU_USER_REGS_s0 8 > +#define RISCV_CPU_USER_REGS_s1 9 > +#define RISCV_CPU_USER_REGS_a0 10 > +#define RISCV_CPU_USER_REGS_a1 11 > +#define RISCV_CPU_USER_REGS_a2 12 > +#define RISCV_CPU_USER_REGS_a3 13 > +#define RISCV_CPU_USER_REGS_a4 14 > +#define RISCV_CPU_USER_REGS_a5 15 > +#define RISCV_CPU_USER_REGS_a6 16 > +#define RISCV_CPU_USER_REGS_a7 17 > +#define RISCV_CPU_USER_REGS_s2 18 > +#define RISCV_CPU_USER_REGS_s3 19 > +#define RISCV_CPU_USER_REGS_s4 20 > +#define RISCV_CPU_USER_REGS_s5 21 > +#define RISCV_CPU_USER_REGS_s6 22 > +#define RISCV_CPU_USER_REGS_s7 23 > +#define RISCV_CPU_USER_REGS_s8 24 > +#define RISCV_CPU_USER_REGS_s9 25 > +#define RISCV_CPU_USER_REGS_s10 26 > +#define RISCV_CPU_USER_REGS_s11 27 > +#define RISCV_CPU_USER_REGS_t3 28 > +#define RISCV_CPU_USER_REGS_t4 29 > +#define RISCV_CPU_USER_REGS_t5 30 > +#define RISCV_CPU_USER_REGS_t6 31 > +#define RISCV_CPU_USER_REGS_sepc 32 > +#define RISCV_CPU_USER_REGS_sstatus 33 > +#define RISCV_CPU_USER_REGS_pregs 34 > +#define RISCV_CPU_USER_REGS_last 35 This block wants moving into the asm-offsets infrastructure, but I suspect they won't want to survive in this form. edit: yeah, definitely not this form. RISCV_CPU_USER_REGS_OFFSET is a recipe for bugs. > + > +#define RISCV_CPU_USER_REGS_OFFSET(x) ((RISCV_CPU_USER_REGS_##x) * __SIZEOF_POINTER__) > +#define RISCV_CPU_USER_REGS_SIZE RISCV_CPU_USER_REGS_OFFSET(last) > + > +#ifndef __ASSEMBLY__ > + > +/* On stack VCPU state */ > +struct cpu_user_regs > +{ > + register_t zero; unsigned long. > + register_t ra; > + register_t sp; > + register_t gp; > + register_t tp; > + register_t t0; > + register_t t1; > + register_t t2; > + register_t s0; > + register_t s1; > + register_t a0; > + register_t a1; > + register_t a2; > + register_t a3; > + register_t a4; > + register_t a5; > + register_t a6; > + register_t a7; > + register_t s2; > + register_t s3; > + register_t s4; > + register_t s5; > + register_t s6; > + register_t s7; > + register_t s8; > + register_t s9; > + register_t s10; > + register_t s11; > + register_t t3; > + register_t t4; > + register_t t5; > + register_t t6; > + register_t sepc; > + register_t sstatus; > + /* pointer to previous stack_cpu_regs */ > + register_t pregs; Stale comment? Also, surely this wants to be cpu_user_regs *pregs; ? > +}; > + > +static inline void wait_for_interrupt(void) There's no point writing out the name in longhand for a wrapper around a single instruction. ~Andrew
On 20.01.2023 15:59, Oleksii Kurochko wrote: > +/* On stack VCPU state */ > +struct cpu_user_regs > +{ > + register_t zero; > + register_t ra; > + register_t sp; > + register_t gp; > + register_t tp; > + register_t t0; > + register_t t1; > + register_t t2; > + register_t s0; > + register_t s1; > + register_t a0; > + register_t a1; > + register_t a2; > + register_t a3; > + register_t a4; > + register_t a5; > + register_t a6; > + register_t a7; > + register_t s2; > + register_t s3; > + register_t s4; > + register_t s5; > + register_t s6; > + register_t s7; > + register_t s8; > + register_t s9; > + register_t s10; > + register_t s11; > + register_t t3; > + register_t t4; > + register_t t5; > + register_t t6; > + register_t sepc; > + register_t sstatus; > + /* pointer to previous stack_cpu_regs */ > + register_t pregs; > +}; What is the planned correlation of this to what x86 a Arm have in their public headers (under the same name)? I think the public header want spelling out first, and if a different internal structure is intended to be used, the interaction between the two would then want outlining in the description here. Jan
On Fri, 2023-01-20 at 15:54 +0000, Andrew Cooper wrote: > On 20/01/2023 2:59 pm, Oleksii Kurochko wrote: > > diff --git a/xen/arch/riscv/include/asm/processor.h > > b/xen/arch/riscv/include/asm/processor.h > > new file mode 100644 > > index 0000000000..5898a09ce6 > > --- /dev/null > > +++ b/xen/arch/riscv/include/asm/processor.h > > @@ -0,0 +1,114 @@ > > +/* SPDX-License-Identifier: MIT */ > > +/***************************************************************** > > ************* > > + * > > + * Copyright 2019 (C) Alistair Francis <alistair.francis@wdc.com> > > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> > > + * Copyright 2023 (C) Vates > > + * > > + */ > > + > > +#ifndef _ASM_RISCV_PROCESSOR_H > > +#define _ASM_RISCV_PROCESSOR_H > > + > > +#include <asm/types.h> > > + > > +#define RISCV_CPU_USER_REGS_zero 0 > > +#define RISCV_CPU_USER_REGS_ra 1 > > +#define RISCV_CPU_USER_REGS_sp 2 > > +#define RISCV_CPU_USER_REGS_gp 3 > > +#define RISCV_CPU_USER_REGS_tp 4 > > +#define RISCV_CPU_USER_REGS_t0 5 > > +#define RISCV_CPU_USER_REGS_t1 6 > > +#define RISCV_CPU_USER_REGS_t2 7 > > +#define RISCV_CPU_USER_REGS_s0 8 > > +#define RISCV_CPU_USER_REGS_s1 9 > > +#define RISCV_CPU_USER_REGS_a0 10 > > +#define RISCV_CPU_USER_REGS_a1 11 > > +#define RISCV_CPU_USER_REGS_a2 12 > > +#define RISCV_CPU_USER_REGS_a3 13 > > +#define RISCV_CPU_USER_REGS_a4 14 > > +#define RISCV_CPU_USER_REGS_a5 15 > > +#define RISCV_CPU_USER_REGS_a6 16 > > +#define RISCV_CPU_USER_REGS_a7 17 > > +#define RISCV_CPU_USER_REGS_s2 18 > > +#define RISCV_CPU_USER_REGS_s3 19 > > +#define RISCV_CPU_USER_REGS_s4 20 > > +#define RISCV_CPU_USER_REGS_s5 21 > > +#define RISCV_CPU_USER_REGS_s6 22 > > +#define RISCV_CPU_USER_REGS_s7 23 > > +#define RISCV_CPU_USER_REGS_s8 24 > > +#define RISCV_CPU_USER_REGS_s9 25 > > +#define RISCV_CPU_USER_REGS_s10 26 > > +#define RISCV_CPU_USER_REGS_s11 27 > > +#define RISCV_CPU_USER_REGS_t3 28 > > +#define RISCV_CPU_USER_REGS_t4 29 > > +#define RISCV_CPU_USER_REGS_t5 30 > > +#define RISCV_CPU_USER_REGS_t6 31 > > +#define RISCV_CPU_USER_REGS_sepc 32 > > +#define RISCV_CPU_USER_REGS_sstatus 33 > > +#define RISCV_CPU_USER_REGS_pregs 34 > > +#define RISCV_CPU_USER_REGS_last 35 > > This block wants moving into the asm-offsets infrastructure, but I > suspect they won't want to survive in this form. > > edit: yeah, definitely not this form. RISCV_CPU_USER_REGS_OFFSET is > a > recipe for bugs. > Thanks for the recommendation I'll take it into account during a work on new version of the patch series. > > + > > +#define RISCV_CPU_USER_REGS_OFFSET(x) ((RISCV_CPU_USER_REGS_##x) > > * __SIZEOF_POINTER__) > > +#define RISCV_CPU_USER_REGS_SIZE > > RISCV_CPU_USER_REGS_OFFSET(last) > > + > > +#ifndef __ASSEMBLY__ > > + > > +/* On stack VCPU state */ > > +struct cpu_user_regs > > +{ > > + register_t zero; > > unsigned long. Why is it better to define them as \unsigned long' instead of register_t? > > > + register_t ra; > > + register_t sp; > > + register_t gp; > > + register_t tp; > > + register_t t0; > > + register_t t1; > > + register_t t2; > > + register_t s0; > > + register_t s1; > > + register_t a0; > > + register_t a1; > > + register_t a2; > > + register_t a3; > > + register_t a4; > > + register_t a5; > > + register_t a6; > > + register_t a7; > > + register_t s2; > > + register_t s3; > > + register_t s4; > > + register_t s5; > > + register_t s6; > > + register_t s7; > > + register_t s8; > > + register_t s9; > > + register_t s10; > > + register_t s11; > > + register_t t3; > > + register_t t4; > > + register_t t5; > > + register_t t6; > > + register_t sepc; > > + register_t sstatus; > > + /* pointer to previous stack_cpu_regs */ > > + register_t pregs; > > Stale comment? Also, surely this wants to be cpu_user_regs *pregs; ? > Not really. Later it would be introduced another one structure: struct pcpu_info { ... struct cpu_user_regs *stack_cpu_regs; ... }; And stack_cpu_regs will be updated during context saving before jump to __handle_exception: /* new_stack_cpu_regs.pregs = old_stack_cpu_res */ REG_L t0, RISCV_PCPUINFO_OFFSET(stack_cpu_regs)(tp) REG_S t0, RISCV_CPU_USER_REGS_OFFSET(pregs)(sp) /* Update stack_cpu_regs */ REG_S sp, RISCV_PCPUINFO_OFFSET(stack_cpu_regs)(tp) And I skipped this part as pcpu_info isn't used anywhere now but reserve some place for pregs in advance. > > +}; > > + > > +static inline void wait_for_interrupt(void) > > There's no point writing out the name in longhand for a wrapper > around a > single instruction. > Will change it to "... wfi(void)" > ~Andrew
On 23/01/2023 12:03 pm, Oleksii wrote: > On Fri, 2023-01-20 at 15:54 +0000, Andrew Cooper wrote: >> On 20/01/2023 2:59 pm, Oleksii Kurochko wrote: >>> + >>> +#define RISCV_CPU_USER_REGS_OFFSET(x) ((RISCV_CPU_USER_REGS_##x) >>> * __SIZEOF_POINTER__) >>> +#define RISCV_CPU_USER_REGS_SIZE >>> RISCV_CPU_USER_REGS_OFFSET(last) >>> + >>> +#ifndef __ASSEMBLY__ >>> + >>> +/* On stack VCPU state */ >>> +struct cpu_user_regs >>> +{ >>> + register_t zero; >> unsigned long. > Why is it better to define them as \unsigned long' instead of > register_t? Because there is a material cost to deliberately hiding the type, in terms of code clarity and legibility. Things like register_t and vaddr_t are nonsense in a POSIX-y build environment where these things are spelled "unsigned long", not to mention that the associated infrastructure is longer than the non-obfuscated form. ~Andrew
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h new file mode 100644 index 0000000000..5898a09ce6 --- /dev/null +++ b/xen/arch/riscv/include/asm/processor.h @@ -0,0 +1,114 @@ +/* SPDX-License-Identifier: MIT */ +/****************************************************************************** + * + * Copyright 2019 (C) Alistair Francis <alistair.francis@wdc.com> + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@gmail.com> + * Copyright 2023 (C) Vates + * + */ + +#ifndef _ASM_RISCV_PROCESSOR_H +#define _ASM_RISCV_PROCESSOR_H + +#include <asm/types.h> + +#define RISCV_CPU_USER_REGS_zero 0 +#define RISCV_CPU_USER_REGS_ra 1 +#define RISCV_CPU_USER_REGS_sp 2 +#define RISCV_CPU_USER_REGS_gp 3 +#define RISCV_CPU_USER_REGS_tp 4 +#define RISCV_CPU_USER_REGS_t0 5 +#define RISCV_CPU_USER_REGS_t1 6 +#define RISCV_CPU_USER_REGS_t2 7 +#define RISCV_CPU_USER_REGS_s0 8 +#define RISCV_CPU_USER_REGS_s1 9 +#define RISCV_CPU_USER_REGS_a0 10 +#define RISCV_CPU_USER_REGS_a1 11 +#define RISCV_CPU_USER_REGS_a2 12 +#define RISCV_CPU_USER_REGS_a3 13 +#define RISCV_CPU_USER_REGS_a4 14 +#define RISCV_CPU_USER_REGS_a5 15 +#define RISCV_CPU_USER_REGS_a6 16 +#define RISCV_CPU_USER_REGS_a7 17 +#define RISCV_CPU_USER_REGS_s2 18 +#define RISCV_CPU_USER_REGS_s3 19 +#define RISCV_CPU_USER_REGS_s4 20 +#define RISCV_CPU_USER_REGS_s5 21 +#define RISCV_CPU_USER_REGS_s6 22 +#define RISCV_CPU_USER_REGS_s7 23 +#define RISCV_CPU_USER_REGS_s8 24 +#define RISCV_CPU_USER_REGS_s9 25 +#define RISCV_CPU_USER_REGS_s10 26 +#define RISCV_CPU_USER_REGS_s11 27 +#define RISCV_CPU_USER_REGS_t3 28 +#define RISCV_CPU_USER_REGS_t4 29 +#define RISCV_CPU_USER_REGS_t5 30 +#define RISCV_CPU_USER_REGS_t6 31 +#define RISCV_CPU_USER_REGS_sepc 32 +#define RISCV_CPU_USER_REGS_sstatus 33 +#define RISCV_CPU_USER_REGS_pregs 34 +#define RISCV_CPU_USER_REGS_last 35 + +#define RISCV_CPU_USER_REGS_OFFSET(x) ((RISCV_CPU_USER_REGS_##x) * __SIZEOF_POINTER__) +#define RISCV_CPU_USER_REGS_SIZE RISCV_CPU_USER_REGS_OFFSET(last) + +#ifndef __ASSEMBLY__ + +/* On stack VCPU state */ +struct cpu_user_regs +{ + register_t zero; + register_t ra; + register_t sp; + register_t gp; + register_t tp; + register_t t0; + register_t t1; + register_t t2; + register_t s0; + register_t s1; + register_t a0; + register_t a1; + register_t a2; + register_t a3; + register_t a4; + register_t a5; + register_t a6; + register_t a7; + register_t s2; + register_t s3; + register_t s4; + register_t s5; + register_t s6; + register_t s7; + register_t s8; + register_t s9; + register_t s10; + register_t s11; + register_t t3; + register_t t4; + register_t t5; + register_t t6; + register_t sepc; + register_t sstatus; + /* pointer to previous stack_cpu_regs */ + register_t pregs; +}; + +static inline void wait_for_interrupt(void) +{ + __asm__ __volatile__ ("wfi"); +} + +#endif /* __ASSEMBLY__ */ + +#endif /* _ASM_RISCV_PROCESSOR_H */ + +/* + * Local variables: + * mode: C + * c-file-style: "BSD" + * c-basic-offset: 4 + * indent-tabs-mode: nil + * End: + */