Re: [PATCH v1 11/14] xen/riscv: introduce setup_trap_handler()

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 20/01/2023 2:59 pm, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  xen/arch/riscv/setup.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index d09ffe1454..174e134c93 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -1,16 +1,27 @@
>  #include <xen/compile.h>
>  #include <xen/init.h>
>  
> +#include <asm/csr.h>
>  #include <asm/early_printk.h>
> +#include <asm/traps.h>
>  
>  /* Xen stack for bringing up the first CPU. */
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +static void setup_trap_handler(void)

We'd normally call this trap_init(), but it wants to live in traps.c
rather than setup.c, with a prototype in traps.h.

> +{
> +    unsigned long addr = (unsigned long)&handle_exception;

Coding style.  Newline between variable declarations and code.

Having looked at the spec, the entrypoint should be named handle_trap
rather than handle_exception.  Per the spec, a trap is either an
interrupt or an exception based on the IRQ flag in CAUSE.

That adjustment to naming wants to percolate down through the calltree
and also in earlier patches.

To avoid the __handle_exception() function in C, you could call the C
version do_trap() which is a reasonably common idiom in other architectures.

> +    csr_write(CSR_STVEC, addr);
> +}
> +
>  void __init noreturn start_xen(void)
>  {
>      early_printk("Hello from C env\n");
>  
> +    setup_trap_handler();
> +    early_printk("exception handler has been setup\n");

Personally I don't think this printk() adds any value.  It's not one
looked at by the smoke test, and it's only a single CSRW away from the
"Hello from C" message.

~Andrew