On 02/03/2023 2:53 pm, Oleksii wrote: > On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote: >> On 02/03/2023 1:23 pm, Oleksii Kurochko wrote: >>> + >>> + /* >>> + * 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. > I think that I'll use s2 and s3 to save bootcpu_id. > > But I am unsure I understand why the asm code shouldn't be changed.
Because nothing in the asm code (right now) touches any of the a registers. Therefore the parameters that OpenSBI prepared for start() (i.e. a0 and a1 here) are still correct for start_xen(). If, and only if, we need to modify a* for other reasons in start() do we need to preserve their values somehow. > If I understand you correctly I can write in a comment ahead of > ENTRY(start) that a0, and a1 are reserved for hart_id and dtb_base > which are passed from a bootloader but it will work only if start_xen > will be only C function called from head.S. Not quite. You want a comment explaining what the OpenSBI -> start() ABI is. So people know what a0/etc is at ENTRY(start). Here is an example from a different project: https://github.com/TrenchBoot/secure-kernel-loader/blob/master/head.S#L52-L68 There is no need to do unnecessary work (i.e. preserving them right now), until you find a reason to need to spill them. Right now, there's not need, and this isn't obviously going to change in the short term. ~Andrew
