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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

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