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? That said, aren't we going to have to rewrite this when enabling H mode anyway? > + 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
