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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-02-27 at 10:17 +0100, Jan Beulich wrote:
> On 24.02.2023 17:36, Oleksii wrote:
> > 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;
> 
> As per below - no, I don't think so. I continue to think these want
> to be function parameters.
> 
> > 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.
> 
> Avoiding assembly code where possible if of course appreciated,
> because
> generally C code is easier to maintain. But of course this can only
> be
> done if things can be expressed correctly. And you need to keep in
> mind
> that asm() statements also are assembly code, just lower volume.
> 
Let's get hard_id and dtb_base in head.S and pass them as arguments of
start_xen() function as you mentioned before.
I'll update the patch and send new version.
> Jan
~ Oleksii