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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-03-02 at 14:02 +0000, Andrew Cooper wrote:
> 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.
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.

I mean that a0-7 are used as function arguments, a0-1 are used for
return value so they are expected to be changed. That is why we have to
save them somewhere.

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.

I probably misunderstand you...

~ Oleksii