Re: [PATCH] xen/riscv: read hart_id and dtb_base passed by bootloader

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Fri, 2023-02-24 at 16:04 +0100, Jan Beulich wrote:
> On 24.02.2023 15:48, Oleksii Kurochko wrote:
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> >  xen/arch/riscv/setup.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> > 
> > diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> > index b3f8b10f71..154bf3a0bc 100644
> > --- a/xen/arch/riscv/setup.c
> > +++ b/xen/arch/riscv/setup.c
> > @@ -26,6 +26,18 @@ static void test_macros_from_bug_h(void)
> >  
> >  void __init noreturn start_xen(void)
> >  {
> > +    /*
> > +     * The following things are passed by bootloader:
> > +     *   a0 -> hart_id
> > +     *   a1 -> dtb_base
> > +    */
> > +    register unsigned long hart_id  asm("a0");
> > +    register unsigned long dtb_base asm("a1");
> > +
> > +    asm volatile( "mv %0, a0" : "=r" (hart_id) );
> > +
> > +    asm volatile( "mv %0, a1" : "=r" (dtb_base) );
> 
> Are you sure this (a) works and (b) is what you want? You're
> inserting
Oh, yeah... it should be:
  unsigned long hart_id;
  unsigned long dtb_base;

I did experiments with 'register' and 'asm()' and after rebase of my
internal branches git backed these changes...

Sorry for that I have to double check patches next time.

It looks like it works only because the variables aren't used anywhere.
> "mov a0, a0" and "mov a1, a1". I suppose as long as the two local
> variables aren't used further down in the function, the compiler will
> be able to recognize both registers as dead, and hence use them for
> argument passing to later functions that you call. But I would expect
> that to break once you actually consume either of the variables.
> 
> Fundamentally I think this is the kind of thing you want do to in
> assembly. However, in the specific case here, can't you simply have
> 
> void __init noreturn start_xen(unsigned long hard_id,
>                                unsigned long dtb_base)
> {
>     ...
> 
> and all is going to be fine, without any asm()?
One of the things that I would like to do is to not use an assembler as
much as possible. And as we have C environment ready after a few
assembly instructions in head.S I thought it would be OK to do it in C
code somewhere at the start so someone/sonething doesn't alter register
a0 and a1.
> 
> Otherwise again a style nit: In the asm statements (not the register
> declarations) there is a missing blank each before the opening
> parenthesis.
> 


> Jan
~Oleksii