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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Mon, 2023-02-27 at 12:37 +0100, Jan Beulich wrote:
> On 27.02.2023 12:19, Oleksii wrote:
> > 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.
> 
> Still looks like a misunderstanding to me. Aiui a0 and a1 are the
> registers to hold first and second function arguments each. If
> firmware places the two values in these two registers, then
> start_xen(), with the adjusted declaration, will fit the purpose
> without any assembly code.
> 
It will work for the code we have now, but it will be more save to save
registers a0 and a1 to some variables and pass them to start_xen() as
arguments as they can be changed by something before the start_xen()
call in the future.

~ Oleksii