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.fran...@wdc.com>
> > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshle...@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
