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 18:58 +0000, Andrew Cooper wrote:
> 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.
> 
Thanks for clarification.

I will document ABI for start and that's it for now
~ Oleksii