Re: [PATCH v1 07/14] xen/riscv: introduce exception handlers implementation

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-01-23 at 11:50 +0000, Andrew Cooper wrote:
> On 20/01/2023 2:59 pm, Oleksii Kurochko wrote:
> > diff --git a/xen/arch/riscv/entry.S b/xen/arch/riscv/entry.S
> > new file mode 100644
> > index 0000000000..f7d46f42bb
> > --- /dev/null
> > +++ b/xen/arch/riscv/entry.S
> > @@ -0,0 +1,97 @@
> > +#include <asm/asm.h>
> > +#include <asm/processor.h>
> > +#include <asm/riscv_encoding.h>
> > +#include <asm/traps.h>
> > +
> > +        .global handle_exception
> > +        .align 4
> > +
> > +handle_exception:
> 
> ENTRY() which takes care of the global and the align.
> 
> Also, you want a size and type at the end, just like in head.S 
> (Sorry,
> we *still* don't have any sane infrastructure for doing that nicely. 
> Opencode it for now.)
> 
> > +
> > +    /* Exceptions from xen */
> > +save_to_stack:
> 
> This label isn't used at all, is it?
> 
> > +        /* Save context to stack */
> > +        REG_S   sp, (RISCV_CPU_USER_REGS_OFFSET(sp) -
> > RISCV_CPU_USER_REGS_SIZE) (sp)
> > +        addi    sp, sp, -RISCV_CPU_USER_REGS_SIZE
> > +        REG_S   t0, RISCV_CPU_USER_REGS_OFFSET(t0)(sp)
> 
> Exceptions on RISC-V don't adjust the stack pointer.  This logic
> depends
> on interrupting Xen code, and Xen not having suffered a stack
> overflow
> (and actually, that the space on the stack for all registers also
> doesn't overflow).
> 
> Which might be fine for now, but I think it warrants a comment
> somewhere
> (probably at handle_exception itself) stating the expectations while
> it's still a work in progress.  So in this case something like:
> 
> /* Work-in-progress:  Depends on interrupting Xen, and the stack
> being
> good. */
> 
> 
> But, do we want to allocate stemp right away (even with an empty
> struct), and get tp set up properly?
> 
I am not sure that I get you here about stemp. Could you please clarify
a little bit.

> That said, aren't we going to have to rewrite this when enabling H
> mode
> anyway?
I based these code on a code from Bobby's repo ( on top of which with
some additional patches I've successfully ran Dom0 ) so I am not sure
that it will be re-written.
Probably I don't understand about which one part you are talking about.

Regarding H mode if to be honest I didn't see where it is switched to
it.
Maybe Bobby or Alistair can explain me?
> 
> > +        j       save_context
> > +
> > +save_context:
> 
> I'd drop this.  It's a nop right now.
> 
> > <snip>
> > +        csrr    t0, CSR_SEPC
> > +        REG_S   t0, RISCV_CPU_USER_REGS_OFFSET(sepc)(sp)
> > +        csrr    t0, CSR_SSTATUS
> > +        REG_S   t0, RISCV_CPU_USER_REGS_OFFSET(sstatus)(sp)
> 
> So something I've noticed about CSRs through this series.
> 
> The C CSR macros are set up to use real CSR names, but the CSR_*
> constants used in C and ASM are raw numbers.
> 
> If we're using raw numbers, then the C CSR accessors should be static
> inlines instead, but the advantage of using names is the toolchain
> can
> issue an error when we reference a CSR not supported by the current
> extensions.
> 
> We ought to use a single form, consistently through Xen.  How
> feasible
> will it be to use names throughout?
> 
> ~Andrew