xen/riscv: read hart_id and dtb_base passed by bootloader

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

Posted by Oleksii Kurochko 1 year, 2 months ago

Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
 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) );
+
     early_printk("Hello from C env\n");
 
     trap_init();
-- 
2.39.0

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

Posted by Jan Beulich 1 year, 2 months ago

On 24.02.2023 15:48, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
>  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
"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()?

Otherwise again a style nit: In the asm statements (not the register
declarations) there is a missing blank each before the opening
parenthesis.

Jan

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

Posted by Oleksii 1 year, 2 months ago

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@gmail.com>
> > ---
> >  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

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

Posted by Jan Beulich 1 year, 2 months ago

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@gmail.com>
>>> ---
>>>  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.

Jan

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

Posted by Oleksii 1 year, 2 months ago

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@gmail.com>
> > > > ---
> > > >  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

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

Posted by Jan Beulich 1 year, 2 months ago

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@gmail.com>
>>>>> ---
>>>>>  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.

Jan

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

Posted by Oleksii 1 year, 2 months ago

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@gmail.com>
> > > > > > ---
> > > > > >  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