Re: [PATCH v2 1/3] xen/riscv: read/save hart_id and dtb_base passed by bootloader

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 02/03/2023 1:23 pm, Oleksii Kurochko wrote:
> diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S
> index ffd95f9f89..851b4691a5 100644
> --- a/xen/arch/riscv/riscv64/head.S
> +++ b/xen/arch/riscv/riscv64/head.S
> @@ -6,8 +7,31 @@ ENTRY(start)
>          /* Mask all interrupts */
>          csrw    CSR_SIE, zero
>  
> +        /* Save HART ID and DTB base */
> +        lla     a6, _bootcpu_id
> +        REG_S   a0, (a6)
> +        lla     a6, _dtb_base
> +        REG_S   a1, (a6)
> +
>          la      sp, cpu0_boot_stack
>          li      t0, STACK_SIZE
>          add     sp, sp, t0
>  
> +        lla     a6, _bootcpu_id
> +        REG_L   a0, (a6)
> +        lla     a6, _dtb_base
> +        REG_L   a1, (a6)

This is overkill.

Put a comment at start identifying which parameters are in which
registers, and just make sure not to clobber them - RISCV has plenty of
registers.

Right now, we don't touch the a registers at all, which is why your v1
patch worked.  (a0 and a1 still have the same value when we get into C).

If we do need to start calling more complex things here (and I'm not
sure that we do), we could either store the parameters in s2-11, or
spill them onto the stack; both of which are preferable to spilling to
global variables like this.

> +
>          tail    start_xen
> +
> +        /*
> +         * Boot cpu id is passed by a bootloader
> +         */
> +_bootcpu_id:
> +        RISCV_PTR 0x0

Just a note (as you want to delete this anyway), this isn't a PTR, it's
a LONG.

> +
> +        /*
> +         * DTB base is passed by a bootloader
> +         */
> +_dtb_base:
> +        RISCV_PTR 0x0
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 1c87899e8e..d9723fe1c0 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -7,7 +7,8 @@
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> -void __init noreturn start_xen(void)
> +void __init noreturn start_xen(unsigned long bootcpu_id,
> +                               unsigned long dtb_base)

To be clear, this change should be this hunk exactly as it is, and a
comment immediately ahead of ENTRY(start) describing the entry ABI.

There is no need currently to change any of the asm code.

~Andrew