Re: [PATCH v1 06/14] xen/riscv: introduce exception context

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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@xxxxxxx>
> > + * Copyright 2021 (C) Bobby Eshleman <bobby.eshleman@xxxxxxxxx>
> > + * 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