diff mbox series

OpenSBI: Boot HART ISA display

Message ID 3e977253-0836-e76f-964b-49b7f2541fde@gmx.de
State New
Headers show
Series OpenSBI: Boot HART ISA display | expand

Commit Message

Heinrich Schuchardt Sept. 28, 2020, 10:17 a.m. UTC
Hello Atish,

on the Kendryte 210 MaixDuino the OpenSBI output line

Boot HART ISA       : rv64cicacsidcacsi

looks a bit strange to me (see full output at the end of the mail).
Assuming that the characters after rv64 are related to extensions I
would expect every letter appearing only once.

I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
do not print out correctly.

After the following change:


the output is:

Boot HART ISA       : rv64imafdcsu

I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.

Do you have a suggestion how to analyze this further?

Best regards

Heinrich



OpenSBI v0.8-29-g7701ea1
   ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

Platform Name       : Kendryte K210
Platform Features   : timer
Platform HART Count : 2
Boot HART ID        : 0
Boot HART ISA       : rv64cicacsidcacsi
BOOT HART Features  : none
BOOT HART PMP Count : 0
BOOT HART MHPM Count: 0
Firmware Base       : 0x80000000
Firmware Size       : 100 KB
Runtime SBI Version : 0.2

MIDELEG : 0x0000000000000222
MEDELEG : 0x0000000000000109

Comments

Atish Patra Sept. 29, 2020, 7:05 p.m. UTC | #1
On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> Hello Atish,
>
> on the Kendryte 210 MaixDuino the OpenSBI output line
>
> Boot HART ISA       : rv64cicacsidcacsi
>
> looks a bit strange to me (see full output at the end of the mail).

Yeah. It doesn't make any sense.

> Assuming that the characters after rv64 are related to extensions I
> would expect every letter appearing only once.
>

That's correct. See 3.1.1. Machine ISA Register misa in priv spec.

> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> do not print out correctly.
>
> After the following change:
>
> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> index 8c54c11..a0c95a2 100644
> --- a/lib/sbi/riscv_asm.c
> +++ b/lib/sbi/riscv_asm.c
> @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>
>         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
> i++) {
>                 if (misa_extension_imp(valid_isa_order[i]))
> -                       out[pos++] = valid_isa_order[i];
> +                       out[pos++] = '0' + i;
>         }
>
>         if (pos < out_sz)
>
> the output becomes
>
> Boot HART ISA       : rv6403567;<@BCDHI
>
> I am clueless why valid_isa_order[i] is not evaluated correctly.
>
> When my changes are:
>
> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> index 8c54c11..b1bbfc4 100644
> --- a/lib/sbi/riscv_asm.c
> +++ b/lib/sbi/riscv_asm.c
> @@ -12,6 +12,8 @@
>  #include <sbi/sbi_error.h>
>  #include <sbi/sbi_platform.h>
>
> +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> +
>  /* determine CPU extension, return non-zero support */
>  int misa_extension_imp(char ext)
>  {
> @@ -52,7 +54,6 @@ int misa_xlen(void)
>  void misa_string(int xlen, char *out, unsigned int out_sz)
>  {
>         unsigned int i, pos = 0;
> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>
>         if (!out)
>                 return;
>
> the output is:
>
> Boot HART ISA       : rv64imafdcsu
>

That's odd. Why does declaring valid_isa_order as static solve the issue ?

Can you print the "misa" in misa_extension_imp () ?
Just a guess: What if misa is not read correctly ?

> I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
>
> Do you have a suggestion how to analyze this further?
>
> Best regards
>
> Heinrich
>
>
>
> OpenSBI v0.8-29-g7701ea1
>    ____                    _____ ____ _____
>   / __ \                  / ____|  _ \_   _|
>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>  | |__| | |_) |  __/ | | |____) | |_) || |_
>   \____/| .__/ \___|_| |_|_____/|____/_____|
>         | |
>         |_|
>
> Platform Name       : Kendryte K210
> Platform Features   : timer
> Platform HART Count : 2
> Boot HART ID        : 0
> Boot HART ISA       : rv64cicacsidcacsi
> BOOT HART Features  : none
> BOOT HART PMP Count : 0
> BOOT HART MHPM Count: 0
> Firmware Base       : 0x80000000
> Firmware Size       : 100 KB
> Runtime SBI Version : 0.2
>
> MIDELEG : 0x0000000000000222
> MEDELEG : 0x0000000000000109
>
>
> --
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi
Heinrich Schuchardt Sept. 29, 2020, 7:38 p.m. UTC | #2
On 9/29/20 9:05 PM, Atish Patra wrote:
> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> Hello Atish,
>>
>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>
>> Boot HART ISA       : rv64cicacsidcacsi
>>
>> looks a bit strange to me (see full output at the end of the mail).
>
> Yeah. It doesn't make any sense.
>
>> Assuming that the characters after rv64 are related to extensions I
>> would expect every letter appearing only once.
>>
>
> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>
>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>> do not print out correctly.
>>
>> After the following change:
>>
>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>> index 8c54c11..a0c95a2 100644
>> --- a/lib/sbi/riscv_asm.c
>> +++ b/lib/sbi/riscv_asm.c
>> @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>>
>>         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
>> i++) {
>>                 if (misa_extension_imp(valid_isa_order[i]))
>> -                       out[pos++] = valid_isa_order[i];
>> +                       out[pos++] = '0' + i;
>>         }
>>
>>         if (pos < out_sz)
>>
>> the output becomes
>>
>> Boot HART ISA       : rv6403567;<@BCDHI
>>
>> I am clueless why valid_isa_order[i] is not evaluated correctly.
>>
>> When my changes are:
>>
>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>> index 8c54c11..b1bbfc4 100644
>> --- a/lib/sbi/riscv_asm.c
>> +++ b/lib/sbi/riscv_asm.c
>> @@ -12,6 +12,8 @@
>>  #include <sbi/sbi_error.h>
>>  #include <sbi/sbi_platform.h>
>>
>> +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>> +
>>  /* determine CPU extension, return non-zero support */
>>  int misa_extension_imp(char ext)
>>  {
>> @@ -52,7 +54,6 @@ int misa_xlen(void)
>>  void misa_string(int xlen, char *out, unsigned int out_sz)
>>  {
>>         unsigned int i, pos = 0;
>> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>>
>>         if (!out)
>>                 return;
>>
>> the output is:
>>
>> Boot HART ISA       : rv64imafdcsu
>>
>
> That's odd. Why does declaring valid_isa_order as static solve the issue ?
>
> Can you print the "misa" in misa_extension_imp () ?
> Just a guess: What if misa is not read correctly ?

No matter which values misa_extension_imp() returns we should only see
extension ID letters in the correct sequence.

"rv64cicacsidcacsi" is incorrect for any value of misa.

The interesting thing is that printf statements for strings that I
placed for debugging in misa_string() did not print correctly. Something
is going wrong here with strings.

It might be that some function is overwriting the area where the strings
are stored, e.g via the stack.

Best regards

Heinrich

>
>> I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
>>
>> Do you have a suggestion how to analyze this further?
>>
>> Best regards
>>
>> Heinrich
>>
>>
>>
>> OpenSBI v0.8-29-g7701ea1
>>    ____                    _____ ____ _____
>>   / __ \                  / ____|  _ \_   _|
>>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>  | |__| | |_) |  __/ | | |____) | |_) || |_
>>   \____/| .__/ \___|_| |_|_____/|____/_____|
>>         | |
>>         |_|
>>
>> Platform Name       : Kendryte K210
>> Platform Features   : timer
>> Platform HART Count : 2
>> Boot HART ID        : 0
>> Boot HART ISA       : rv64cicacsidcacsi
>> BOOT HART Features  : none
>> BOOT HART PMP Count : 0
>> BOOT HART MHPM Count: 0
>> Firmware Base       : 0x80000000
>> Firmware Size       : 100 KB
>> Runtime SBI Version : 0.2
>>
>> MIDELEG : 0x0000000000000222
>> MEDELEG : 0x0000000000000109
>>
>>
>> --
>> opensbi mailing list
>> opensbi@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
>
>
Atish Patra Sept. 29, 2020, 8:38 p.m. UTC | #3
On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/29/20 9:05 PM, Atish Patra wrote:
> > On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> Hello Atish,
> >>
> >> on the Kendryte 210 MaixDuino the OpenSBI output line
> >>
> >> Boot HART ISA       : rv64cicacsidcacsi
> >>
> >> looks a bit strange to me (see full output at the end of the mail).
> >
> > Yeah. It doesn't make any sense.
> >
> >> Assuming that the characters after rv64 are related to extensions I
> >> would expect every letter appearing only once.
> >>
> >
> > That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> >
> >> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> >> do not print out correctly.
> >>
> >> After the following change:
> >>
> >> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> >> index 8c54c11..a0c95a2 100644
> >> --- a/lib/sbi/riscv_asm.c
> >> +++ b/lib/sbi/riscv_asm.c
> >> @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
> >>
> >>         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
> >> i++) {
> >>                 if (misa_extension_imp(valid_isa_order[i]))
> >> -                       out[pos++] = valid_isa_order[i];
> >> +                       out[pos++] = '0' + i;
> >>         }
> >>
> >>         if (pos < out_sz)
> >>
> >> the output becomes
> >>
> >> Boot HART ISA       : rv6403567;<@BCDHI
> >>
> >> I am clueless why valid_isa_order[i] is not evaluated correctly.
> >>
> >> When my changes are:
> >>
> >> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> >> index 8c54c11..b1bbfc4 100644
> >> --- a/lib/sbi/riscv_asm.c
> >> +++ b/lib/sbi/riscv_asm.c
> >> @@ -12,6 +12,8 @@
> >>  #include <sbi/sbi_error.h>
> >>  #include <sbi/sbi_platform.h>
> >>
> >> +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> >> +
> >>  /* determine CPU extension, return non-zero support */
> >>  int misa_extension_imp(char ext)
> >>  {
> >> @@ -52,7 +54,6 @@ int misa_xlen(void)
> >>  void misa_string(int xlen, char *out, unsigned int out_sz)
> >>  {
> >>         unsigned int i, pos = 0;
> >> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> >>
> >>         if (!out)
> >>                 return;
> >>
> >> the output is:
> >>
> >> Boot HART ISA       : rv64imafdcsu
> >>
> >
> > That's odd. Why does declaring valid_isa_order as static solve the issue ?
> >
> > Can you print the "misa" in misa_extension_imp () ?
> > Just a guess: What if misa is not read correctly ?
>
> No matter which values misa_extension_imp() returns we should only see
> extension ID letters in the correct sequence.
>

You are correct.

> "rv64cicacsidcacsi" is incorrect for any value of misa.
>
> The interesting thing is that printf statements for strings that I
> placed for debugging in misa_string() did not print correctly. Something
> is going wrong here with strings.
>

+ Damien

I just remembered Damien was facing a similar issue on Kendryte while
running the test payload where
it would not work if a string is used but worked fine if a single
character is printed in a loop.

@Damien : Did you figure out the cause for that issue you were seeing?

> It might be that some function is overwriting the area where the strings
> are stored, e.g via the stack.
>
> Best regards
>
> Heinrich
>
> >
> >> I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
> >>
> >> Do you have a suggestion how to analyze this further?
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >>
> >>
> >> OpenSBI v0.8-29-g7701ea1
> >>    ____                    _____ ____ _____
> >>   / __ \                  / ____|  _ \_   _|
> >>  | |  | |_ __   ___ _ __ | (___ | |_) || |
> >>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> >>  | |__| | |_) |  __/ | | |____) | |_) || |_
> >>   \____/| .__/ \___|_| |_|_____/|____/_____|
> >>         | |
> >>         |_|
> >>
> >> Platform Name       : Kendryte K210
> >> Platform Features   : timer
> >> Platform HART Count : 2
> >> Boot HART ID        : 0
> >> Boot HART ISA       : rv64cicacsidcacsi
> >> BOOT HART Features  : none
> >> BOOT HART PMP Count : 0
> >> BOOT HART MHPM Count: 0
> >> Firmware Base       : 0x80000000
> >> Firmware Size       : 100 KB
> >> Runtime SBI Version : 0.2
> >>
> >> MIDELEG : 0x0000000000000222
> >> MEDELEG : 0x0000000000000109
> >>
> >>
> >> --
> >> opensbi mailing list
> >> opensbi@lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/opensbi
> >
> >
> >
>
Damien Le Moal Sept. 30, 2020, 3:19 a.m. UTC | #4
On 2020/09/30 5:38, Atish Patra wrote:
> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> Hello Atish,
>>>>
>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>
>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>
>>>> looks a bit strange to me (see full output at the end of the mail).
>>>
>>> Yeah. It doesn't make any sense.
>>>
>>>> Assuming that the characters after rv64 are related to extensions I
>>>> would expect every letter appearing only once.
>>>>
>>>
>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>
>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>> do not print out correctly.
>>>>
>>>> After the following change:
>>>>
>>>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>>>> index 8c54c11..a0c95a2 100644
>>>> --- a/lib/sbi/riscv_asm.c
>>>> +++ b/lib/sbi/riscv_asm.c
>>>> @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>>>>
>>>>         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
>>>> i++) {
>>>>                 if (misa_extension_imp(valid_isa_order[i]))
>>>> -                       out[pos++] = valid_isa_order[i];
>>>> +                       out[pos++] = '0' + i;
>>>>         }
>>>>
>>>>         if (pos < out_sz)
>>>>
>>>> the output becomes
>>>>
>>>> Boot HART ISA       : rv6403567;<@BCDHI
>>>>
>>>> I am clueless why valid_isa_order[i] is not evaluated correctly.
>>>>
>>>> When my changes are:
>>>>
>>>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>>>> index 8c54c11..b1bbfc4 100644
>>>> --- a/lib/sbi/riscv_asm.c
>>>> +++ b/lib/sbi/riscv_asm.c
>>>> @@ -12,6 +12,8 @@
>>>>  #include <sbi/sbi_error.h>
>>>>  #include <sbi/sbi_platform.h>
>>>>
>>>> +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>>>> +
>>>>  /* determine CPU extension, return non-zero support */
>>>>  int misa_extension_imp(char ext)
>>>>  {
>>>> @@ -52,7 +54,6 @@ int misa_xlen(void)
>>>>  void misa_string(int xlen, char *out, unsigned int out_sz)
>>>>  {
>>>>         unsigned int i, pos = 0;
>>>> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>>>>
>>>>         if (!out)
>>>>                 return;
>>>>
>>>> the output is:
>>>>
>>>> Boot HART ISA       : rv64imafdcsu
>>>>
>>>
>>> That's odd. Why does declaring valid_isa_order as static solve the issue ?
>>>
>>> Can you print the "misa" in misa_extension_imp () ?
>>> Just a guess: What if misa is not read correctly ?
>>
>> No matter which values misa_extension_imp() returns we should only see
>> extension ID letters in the correct sequence.
>>
> 
> You are correct.
> 
>> "rv64cicacsidcacsi" is incorrect for any value of misa.
>>
>> The interesting thing is that printf statements for strings that I
>> placed for debugging in misa_string() did not print correctly. Something
>> is going wrong here with strings.
>>
> 
> + Damien
> 
> I just remembered Damien was facing a similar issue on Kendryte while
> running the test payload where
> it would not work if a string is used but worked fine if a single
> character is printed in a loop.
> 
> @Damien : Did you figure out the cause for that issue you were seeing?

I recall the problem, but really do not remember what we did about it, if
anything. Would need to inspect git log and try again. I am currently in full
Linux mode with the Kendryte and that does not use opensbi...

>> It might be that some function is overwriting the area where the strings
>> are stored, e.g via the stack.

Yes, something very weird is going on. Did you try to disassemble the
misa_string() function to see if something is wrong ? That may be worth looking at.

Let me try again on my end.

>>
>> Best regards
>>
>> Heinrich
>>
>>>
>>>> I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
>>>>
>>>> Do you have a suggestion how to analyze this further?
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>
>>>>
>>>> OpenSBI v0.8-29-g7701ea1
>>>>    ____                    _____ ____ _____
>>>>   / __ \                  / ____|  _ \_   _|
>>>>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>>>>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>>>  | |__| | |_) |  __/ | | |____) | |_) || |_
>>>>   \____/| .__/ \___|_| |_|_____/|____/_____|
>>>>         | |
>>>>         |_|
>>>>
>>>> Platform Name       : Kendryte K210
>>>> Platform Features   : timer
>>>> Platform HART Count : 2
>>>> Boot HART ID        : 0
>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>> BOOT HART Features  : none
>>>> BOOT HART PMP Count : 0
>>>> BOOT HART MHPM Count: 0
>>>> Firmware Base       : 0x80000000
>>>> Firmware Size       : 100 KB
>>>> Runtime SBI Version : 0.2
>>>>
>>>> MIDELEG : 0x0000000000000222
>>>> MEDELEG : 0x0000000000000109
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>>
>>>
>>>
>>
> 
>
Damien Le Moal Sept. 30, 2020, 3:26 a.m. UTC | #5
On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 9/29/20 9:05 PM, Atish Patra wrote:
> > > On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > Hello Atish,
> > > > 
> > > > on the Kendryte 210 MaixDuino the OpenSBI output line
> > > > 
> > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > 
> > > > looks a bit strange to me (see full output at the end of the mail).
> > > 
> > > Yeah. It doesn't make any sense.
> > > 
> > > > Assuming that the characters after rv64 are related to extensions I
> > > > would expect every letter appearing only once.
> > > > 
> > > 
> > > That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> > > 
> > > > I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> > > > do not print out correctly.
> > > > 
> > > > After the following change:
> > > > 
> > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > index 8c54c11..a0c95a2 100644
> > > > --- a/lib/sbi/riscv_asm.c
> > > > +++ b/lib/sbi/riscv_asm.c
> > > > @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
> > > > 
> > > >         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
> > > > i++) {
> > > >                 if (misa_extension_imp(valid_isa_order[i]))
> > > > -                       out[pos++] = valid_isa_order[i];
> > > > +                       out[pos++] = '0' + i;
> > > >         }
> > > > 
> > > >         if (pos < out_sz)
> > > > 
> > > > the output becomes
> > > > 
> > > > Boot HART ISA       : rv6403567;<@BCDHI
> > > > 
> > > > I am clueless why valid_isa_order[i] is not evaluated correctly.
> > > > 
> > > > When my changes are:
> > > > 
> > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > index 8c54c11..b1bbfc4 100644
> > > > --- a/lib/sbi/riscv_asm.c
> > > > +++ b/lib/sbi/riscv_asm.c
> > > > @@ -12,6 +12,8 @@
> > > >  #include <sbi/sbi_error.h>
> > > >  #include <sbi/sbi_platform.h>
> > > > 
> > > > +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > +
> > > >  /* determine CPU extension, return non-zero support */
> > > >  int misa_extension_imp(char ext)
> > > >  {
> > > > @@ -52,7 +54,6 @@ int misa_xlen(void)
> > > >  void misa_string(int xlen, char *out, unsigned int out_sz)
> > > >  {
> > > >         unsigned int i, pos = 0;
> > > > -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > 
> > > >         if (!out)
> > > >                 return;
> > > > 
> > > > the output is:
> > > > 
> > > > Boot HART ISA       : rv64imafdcsu
> > > > 
> > > 
> > > That's odd. Why does declaring valid_isa_order as static solve the issue ?
> > > 
> > > Can you print the "misa" in misa_extension_imp () ?
> > > Just a guess: What if misa is not read correctly ?
> > 
> > No matter which values misa_extension_imp() returns we should only see
> > extension ID letters in the correct sequence.
> > 
> 
> You are correct.
> 
> > "rv64cicacsidcacsi" is incorrect for any value of misa.
> > 
> > The interesting thing is that printf statements for strings that I
> > placed for debugging in misa_string() did not print correctly. Something
> > is going wrong here with strings.
> > 
> 
> + Damien
> 
> I just remembered Damien was facing a similar issue on Kendryte while
> running the test payload where
> it would not work if a string is used but worked fine if a single
> character is printed in a loop.
> 
> @Damien : Did you figure out the cause for that issue you were seeing?

Here is what I get:


OpenSBI v0.8-29-g7701ea1
   ____                    _____ ____ _____
  / __ \                  / ____|  _ \_   _|
 | |  | |_ __   ___ _ __ | (___ | |_) || |
 | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
 | |__| | |_) |  __/ | | |____) | |_) || |_
  \____/| .__/ \___|_| |_|_____/|____/_____|
        | |
        |_|

Platform Name       : Kendryte K210
Platform Features   : timer
Platform HART Count : 2
Boot HART ID        : 0
Boot HART ISA       : rv64fucicacsidc
BOOT HART Features  : none
BOOT HART PMP Count : 0
BOOT HART MHPM Count: 0
Firmware Base       : 0x80000000
Firmware Size       : 96 KB
Runtime SBI Version : 0.2

MIDELEG : 0x0000000000000222
MEDELEG : 0x0000000000000109

So different fuzziness for the MISA string, wrong too.
And no prints from the test payload either, so definitely wrong.

However, I am getting warnings on the DTS. Something wrong there that may
impact the serial. Looking into it.

> 
> > It might be that some function is overwriting the area where the strings
> > are stored, e.g via the stack.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > > I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
> > > > 
> > > > Do you have a suggestion how to analyze this further?
> > > > 
> > > > Best regards
> > > > 
> > > > Heinrich
> > > > 
> > > > 
> > > > 
> > > > OpenSBI v0.8-29-g7701ea1
> > > >    ____                    _____ ____ _____
> > > >   / __ \                  / ____|  _ \_   _|
> > > >  | |  | |_ __   ___ _ __ | (___ | |_) || |
> > > >  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > > >  | |__| | |_) |  __/ | | |____) | |_) || |_
> > > >   \____/| .__/ \___|_| |_|_____/|____/_____|
> > > >         | |
> > > >         |_|
> > > > 
> > > > Platform Name       : Kendryte K210
> > > > Platform Features   : timer
> > > > Platform HART Count : 2
> > > > Boot HART ID        : 0
> > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > BOOT HART Features  : none
> > > > BOOT HART PMP Count : 0
> > > > BOOT HART MHPM Count: 0
> > > > Firmware Base       : 0x80000000
> > > > Firmware Size       : 100 KB
> > > > Runtime SBI Version : 0.2
> > > > 
> > > > MIDELEG : 0x0000000000000222
> > > > MEDELEG : 0x0000000000000109
> > > > 
> > > > 
> > > > --
> > > > opensbi mailing list
> > > > opensbi@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> > > 
> > > 
> 
>
Damien Le Moal Sept. 30, 2020, 5:08 a.m. UTC | #6
On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > On 9/29/20 9:05 PM, Atish Patra wrote:
> > > On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > Hello Atish,
> > > > 
> > > > on the Kendryte 210 MaixDuino the OpenSBI output line
> > > > 
> > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > 
> > > > looks a bit strange to me (see full output at the end of the mail).
> > > 
> > > Yeah. It doesn't make any sense.
> > > 
> > > > Assuming that the characters after rv64 are related to extensions I
> > > > would expect every letter appearing only once.
> > > > 
> > > 
> > > That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> > > 
> > > > I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> > > > do not print out correctly.
> > > > 
> > > > After the following change:
> > > > 
> > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > index 8c54c11..a0c95a2 100644
> > > > --- a/lib/sbi/riscv_asm.c
> > > > +++ b/lib/sbi/riscv_asm.c
> > > > @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
> > > > 
> > > >         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
> > > > i++) {
> > > >                 if (misa_extension_imp(valid_isa_order[i]))
> > > > -                       out[pos++] = valid_isa_order[i];
> > > > +                       out[pos++] = '0' + i;
> > > >         }
> > > > 
> > > >         if (pos < out_sz)
> > > > 
> > > > the output becomes
> > > > 
> > > > Boot HART ISA       : rv6403567;<@BCDHI
> > > > 
> > > > I am clueless why valid_isa_order[i] is not evaluated correctly.
> > > > 
> > > > When my changes are:
> > > > 
> > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > index 8c54c11..b1bbfc4 100644
> > > > --- a/lib/sbi/riscv_asm.c
> > > > +++ b/lib/sbi/riscv_asm.c
> > > > @@ -12,6 +12,8 @@
> > > >  #include <sbi/sbi_error.h>
> > > >  #include <sbi/sbi_platform.h>
> > > > 
> > > > +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > +
> > > >  /* determine CPU extension, return non-zero support */
> > > >  int misa_extension_imp(char ext)
> > > >  {
> > > > @@ -52,7 +54,6 @@ int misa_xlen(void)
> > > >  void misa_string(int xlen, char *out, unsigned int out_sz)
> > > >  {
> > > >         unsigned int i, pos = 0;
> > > > -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > 
> > > >         if (!out)
> > > >                 return;
> > > > 
> > > > the output is:
> > > > 
> > > > Boot HART ISA       : rv64imafdcsu
> > > > 
> > > 
> > > That's odd. Why does declaring valid_isa_order as static solve the issue ?
> > > 
> > > Can you print the "misa" in misa_extension_imp () ?
> > > Just a guess: What if misa is not read correctly ?
> > 
> > No matter which values misa_extension_imp() returns we should only see
> > extension ID letters in the correct sequence.
> > 
> 
> You are correct.
> 
> > "rv64cicacsidcacsi" is incorrect for any value of misa.
> > 
> > The interesting thing is that printf statements for strings that I
> > placed for debugging in misa_string() did not print correctly. Something
> > is going wrong here with strings.
> > 
> 
> + Damien
> 
> I just remembered Damien was facing a similar issue on Kendryte while
> running the test payload where
> it would not work if a string is used but worked fine if a single
> character is printed in a loop.
> 
> @Damien : Did you figure out the cause for that issue you were seeing?

I see lots of problems, not sure if they are related yet:
1) On a clean build, make CROSS_COMPILE=riscv64-linux- PLATFORM=kendryte/k210
after a "make clean" or "git clone", the kendryte dts is *not* compiled. If I
modify the dts and run make again, then the dts gets compiled but throws out
warnings about #address-cells missing. The Kendryte dts is wrong and needs
updates. I updated with what I have for Linux which is what u-boot has too,
simplified, and still gets warnings while there are none on kernel compiles.
Since Kendryte is generic platform now, this may impact how initialization is
done.
2) I see the same problem as Heinrich with the misa string: garbage is output.
As a little experiment, I made this change:

diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
index 8c54c11..195b56b 100644
--- a/lib/sbi/riscv_asm.c
+++ b/lib/sbi/riscv_asm.c
@@ -11,6 +11,7 @@
 #include <sbi/riscv_encoding.h>
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_platform.h>
+#include <sbi/sbi_string.h>
 
 /* determine CPU extension, return non-zero support */
 int misa_extension_imp(char ext)
@@ -52,7 +53,7 @@ int misa_xlen(void)
 void misa_string(int xlen, char *out, unsigned int out_sz)
 {
        unsigned int i, pos = 0;
-       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
+       const char *valid_isa_order = "iemafdqclbjtpvnsuhkorwxyzg";
 
        if (!out)
                return;
@@ -79,10 +80,8 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
                }
        }
 
-       for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz); i++) {
-               if (misa_extension_imp(valid_isa_order[i]))
-                       out[pos++] = valid_isa_order[i];
-       }
+       for (i = 0; i < sbi_strlen(valid_isa_order) && (pos < out_sz); i++)
+               out[pos++] = valid_isa_order[i];
 
        if (pos < out_sz)
                out[pos++] = '\0';

so that all valid_isa_order characters are printed. And I get:

Boot HART ISA       : rv64terrupt-controller

Which looks like a piece of the dts...

So, I think we have a build/linking/loading problem on our hands. Memory
corruption of data basically. That also likely explain why there is no string
output from the test payload but I do get single character outputs if I do
them.

I tried changing the misa_string() code in different ways and each one gives a
different garbage but always the same garbage (rebooting several time gives the
same). So it looks like linking or loading problem.

And the dts compilation needs addressing too:
1) Erratic dtb build
2) wrong error messages on dtb compilation

Need to go back to linux-scsi for some patching. Will try to look into this
later if I have time. Anyone, feel free to tackle this !


> 
> > It might be that some function is overwriting the area where the strings
> > are stored, e.g via the stack.
> > 
> > Best regards
> > 
> > Heinrich
> > 
> > > > I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
> > > > 
> > > > Do you have a suggestion how to analyze this further?
> > > > 
> > > > Best regards
> > > > 
> > > > Heinrich
> > > > 
> > > > 
> > > > 
> > > > OpenSBI v0.8-29-g7701ea1
> > > >    ____                    _____ ____ _____
> > > >   / __ \                  / ____|  _ \_   _|
> > > >  | |  | |_ __   ___ _ __ | (___ | |_) || |
> > > >  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > > >  | |__| | |_) |  __/ | | |____) | |_) || |_
> > > >   \____/| .__/ \___|_| |_|_____/|____/_____|
> > > >         | |
> > > >         |_|
> > > > 
> > > > Platform Name       : Kendryte K210
> > > > Platform Features   : timer
> > > > Platform HART Count : 2
> > > > Boot HART ID        : 0
> > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > BOOT HART Features  : none
> > > > BOOT HART PMP Count : 0
> > > > BOOT HART MHPM Count: 0
> > > > Firmware Base       : 0x80000000
> > > > Firmware Size       : 100 KB
> > > > Runtime SBI Version : 0.2
> > > > 
> > > > MIDELEG : 0x0000000000000222
> > > > MEDELEG : 0x0000000000000109
> > > > 
> > > > 
> > > > --
> > > > opensbi mailing list
> > > > opensbi@lists.infradead.org
> > > > http://lists.infradead.org/mailman/listinfo/opensbi
> > > 
> > > 
> 
>
Damien Le Moal Sept. 30, 2020, 8:22 a.m. UTC | #7
On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> > On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > On 9/29/20 9:05 PM, Atish Patra wrote:
> > > > On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > Hello Atish,
> > > > > 
> > > > > on the Kendryte 210 MaixDuino the OpenSBI output line
> > > > > 
> > > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > > 
> > > > > looks a bit strange to me (see full output at the end of the mail).
> > > > 
> > > > Yeah. It doesn't make any sense.
> > > > 
> > > > > Assuming that the characters after rv64 are related to extensions I
> > > > > would expect every letter appearing only once.
> > > > > 
> > > > 
> > > > That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> > > > 
> > > > > I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> > > > > do not print out correctly.
> > > > > 
> > > > > After the following change:
> > > > > 
> > > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > > index 8c54c11..a0c95a2 100644
> > > > > --- a/lib/sbi/riscv_asm.c
> > > > > +++ b/lib/sbi/riscv_asm.c
> > > > > @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
> > > > > 
> > > > >         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
> > > > > i++) {
> > > > >                 if (misa_extension_imp(valid_isa_order[i]))
> > > > > -                       out[pos++] = valid_isa_order[i];
> > > > > +                       out[pos++] = '0' + i;
> > > > >         }
> > > > > 
> > > > >         if (pos < out_sz)
> > > > > 
> > > > > the output becomes
> > > > > 
> > > > > Boot HART ISA       : rv6403567;<@BCDHI
> > > > > 
> > > > > I am clueless why valid_isa_order[i] is not evaluated correctly.
> > > > > 
> > > > > When my changes are:
> > > > > 
> > > > > diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> > > > > index 8c54c11..b1bbfc4 100644
> > > > > --- a/lib/sbi/riscv_asm.c
> > > > > +++ b/lib/sbi/riscv_asm.c
> > > > > @@ -12,6 +12,8 @@
> > > > >  #include <sbi/sbi_error.h>
> > > > >  #include <sbi/sbi_platform.h>
> > > > > 
> > > > > +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > > +
> > > > >  /* determine CPU extension, return non-zero support */
> > > > >  int misa_extension_imp(char ext)
> > > > >  {
> > > > > @@ -52,7 +54,6 @@ int misa_xlen(void)
> > > > >  void misa_string(int xlen, char *out, unsigned int out_sz)
> > > > >  {
> > > > >         unsigned int i, pos = 0;
> > > > > -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> > > > > 
> > > > >         if (!out)
> > > > >                 return;
> > > > > 
> > > > > the output is:
> > > > > 
> > > > > Boot HART ISA       : rv64imafdcsu
> > > > > 
> > > > 
> > > > That's odd. Why does declaring valid_isa_order as static solve the issue ?
> > > > 
> > > > Can you print the "misa" in misa_extension_imp () ?
> > > > Just a guess: What if misa is not read correctly ?
> > > 
> > > No matter which values misa_extension_imp() returns we should only see
> > > extension ID letters in the correct sequence.
> > > 
> > 
> > You are correct.
> > 
> > > "rv64cicacsidcacsi" is incorrect for any value of misa.
> > > 
> > > The interesting thing is that printf statements for strings that I
> > > placed for debugging in misa_string() did not print correctly. Something
> > > is going wrong here with strings.
> > > 
> > 
> > + Damien
> > 
> > I just remembered Damien was facing a similar issue on Kendryte while
> > running the test payload where
> > it would not work if a string is used but worked fine if a single
> > character is printed in a loop.
> > 
> > @Damien : Did you figure out the cause for that issue you were seeing?
> 
> I see lots of problems, not sure if they are related yet:
> 1) On a clean build, make CROSS_COMPILE=riscv64-linux- PLATFORM=kendryte/k210
> after a "make clean" or "git clone", the kendryte dts is *not* compiled. If I
> modify the dts and run make again, then the dts gets compiled but throws out
> warnings about #address-cells missing. The Kendryte dts is wrong and needs
> updates. I updated with what I have for Linux which is what u-boot has too,
> simplified, and still gets warnings while there are none on kernel compiles.
> Since Kendryte is generic platform now, this may impact how initialization is
> done.
> 2) I see the same problem as Heinrich with the misa string: garbage is output.
> As a little experiment, I made this change:
> 
> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
> index 8c54c11..195b56b 100644
> --- a/lib/sbi/riscv_asm.c
> +++ b/lib/sbi/riscv_asm.c
> @@ -11,6 +11,7 @@
>  #include <sbi/riscv_encoding.h>
>  #include <sbi/sbi_error.h>
>  #include <sbi/sbi_platform.h>
> +#include <sbi/sbi_string.h>
>  
>  /* determine CPU extension, return non-zero support */
>  int misa_extension_imp(char ext)
> @@ -52,7 +53,7 @@ int misa_xlen(void)
>  void misa_string(int xlen, char *out, unsigned int out_sz)
>  {
>         unsigned int i, pos = 0;
> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
> +       const char *valid_isa_order = "iemafdqclbjtpvnsuhkorwxyzg";
>  
>         if (!out)
>                 return;
> @@ -79,10 +80,8 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>                 }
>         }
>  
> -       for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz); i++) {
> -               if (misa_extension_imp(valid_isa_order[i]))
> -                       out[pos++] = valid_isa_order[i];
> -       }
> +       for (i = 0; i < sbi_strlen(valid_isa_order) && (pos < out_sz); i++)
> +               out[pos++] = valid_isa_order[i];
>  
>         if (pos < out_sz)
>                 out[pos++] = '\0';
> 
> so that all valid_isa_order characters are printed. And I get:
> 
> Boot HART ISA       : rv64terrupt-controller
> 
> Which looks like a piece of the dts...
> 
> So, I think we have a build/linking/loading problem on our hands. Memory
> corruption of data basically. That also likely explain why there is no string
> output from the test payload but I do get single character outputs if I do
> them.
> 
> I tried changing the misa_string() code in different ways and each one gives a
> different garbage but always the same garbage (rebooting several time gives the
> same). So it looks like linking or loading problem.
> 
> And the dts compilation needs addressing too:
> 1) Erratic dtb build
> 2) wrong error messages on dtb compilation
> 
> Need to go back to linux-scsi for some patching. Will try to look into this
> later if I have time. Anyone, feel free to tackle this !

Playing again with this, I tried removing the embedded dtb for kendryte. And as
expected, the misa_string becomes correct. Clearly, the embedded dtb is the
root cause. It is overwriting data/bss. Still no output from test payload
though.

Looking at fw_base.S, I see the call to fw_platform_init that gets the address
of the embedded dtb, but not sure if there is a relocation going on. If there
is, the dtb size symbol (dt_k210_size) is unused. Without it, I do not see how
relocation can work correctly.

Assembler not being my thing, if someone can have a look...

Cheers.

> 
> 
> > > It might be that some function is overwriting the area where the strings
> > > are stored, e.g via the stack.
> > > 
> > > Best regards
> > > 
> > > Heinrich
> > > 
> > > > > I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
> > > > > 
> > > > > Do you have a suggestion how to analyze this further?
> > > > > 
> > > > > Best regards
> > > > > 
> > > > > Heinrich
> > > > > 
> > > > > 
> > > > > 
> > > > > OpenSBI v0.8-29-g7701ea1
> > > > >    ____                    _____ ____ _____
> > > > >   / __ \                  / ____|  _ \_   _|
> > > > >  | |  | |_ __   ___ _ __ | (___ | |_) || |
> > > > >  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
> > > > >  | |__| | |_) |  __/ | | |____) | |_) || |_
> > > > >   \____/| .__/ \___|_| |_|_____/|____/_____|
> > > > >         | |
> > > > >         |_|
> > > > > 
> > > > > Platform Name       : Kendryte K210
> > > > > Platform Features   : timer
> > > > > Platform HART Count : 2
> > > > > Boot HART ID        : 0
> > > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > > BOOT HART Features  : none
> > > > > BOOT HART PMP Count : 0
> > > > > BOOT HART MHPM Count: 0
> > > > > Firmware Base       : 0x80000000
> > > > > Firmware Size       : 100 KB
> > > > > Runtime SBI Version : 0.2
> > > > > 
> > > > > MIDELEG : 0x0000000000000222
> > > > > MEDELEG : 0x0000000000000109
> > > > > 
> > > > > 
> > > > > --
> > > > > opensbi mailing list
> > > > > opensbi@lists.infradead.org
> > > > > http://lists.infradead.org/mailman/listinfo/opensbi
Heinrich Schuchardt Sept. 30, 2020, 8:45 a.m. UTC | #8
On 30.09.20 10:22, Damien Le Moal wrote:
> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>> Hello Atish,
>>>>>>
>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>>>
>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>>
>>>>>> looks a bit strange to me (see full output at the end of the mail).
>>>>>
>>>>> Yeah. It doesn't make any sense.
>>>>>
>>>>>> Assuming that the characters after rv64 are related to extensions I
>>>>>> would expect every letter appearing only once.
>>>>>>
>>>>>
>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>>>
>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>>>> do not print out correctly.
>>>>>>
>>>>>> After the following change:
>>>>>>
>>>>>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>>>>>> index 8c54c11..a0c95a2 100644
>>>>>> --- a/lib/sbi/riscv_asm.c
>>>>>> +++ b/lib/sbi/riscv_asm.c
>>>>>> @@ -81,7 +81,7 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>>>>>>
>>>>>>         for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
>>>>>> i++) {
>>>>>>                 if (misa_extension_imp(valid_isa_order[i]))
>>>>>> -                       out[pos++] = valid_isa_order[i];
>>>>>> +                       out[pos++] = '0' + i;
>>>>>>         }
>>>>>>
>>>>>>         if (pos < out_sz)
>>>>>>
>>>>>> the output becomes
>>>>>>
>>>>>> Boot HART ISA       : rv6403567;<@BCDHI
>>>>>>
>>>>>> I am clueless why valid_isa_order[i] is not evaluated correctly.
>>>>>>
>>>>>> When my changes are:
>>>>>>
>>>>>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>>>>>> index 8c54c11..b1bbfc4 100644
>>>>>> --- a/lib/sbi/riscv_asm.c
>>>>>> +++ b/lib/sbi/riscv_asm.c
>>>>>> @@ -12,6 +12,8 @@
>>>>>>  #include <sbi/sbi_error.h>
>>>>>>  #include <sbi/sbi_platform.h>
>>>>>>
>>>>>> +static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>>>>>> +
>>>>>>  /* determine CPU extension, return non-zero support */
>>>>>>  int misa_extension_imp(char ext)
>>>>>>  {
>>>>>> @@ -52,7 +54,6 @@ int misa_xlen(void)
>>>>>>  void misa_string(int xlen, char *out, unsigned int out_sz)
>>>>>>  {
>>>>>>         unsigned int i, pos = 0;
>>>>>> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>>>>>>
>>>>>>         if (!out)
>>>>>>                 return;
>>>>>>
>>>>>> the output is:
>>>>>>
>>>>>> Boot HART ISA       : rv64imafdcsu
>>>>>>
>>>>>
>>>>> That's odd. Why does declaring valid_isa_order as static solve the issue ?
>>>>>
>>>>> Can you print the "misa" in misa_extension_imp () ?
>>>>> Just a guess: What if misa is not read correctly ?
>>>>
>>>> No matter which values misa_extension_imp() returns we should only see
>>>> extension ID letters in the correct sequence.
>>>>
>>>
>>> You are correct.
>>>
>>>> "rv64cicacsidcacsi" is incorrect for any value of misa.
>>>>
>>>> The interesting thing is that printf statements for strings that I
>>>> placed for debugging in misa_string() did not print correctly. Something
>>>> is going wrong here with strings.
>>>>
>>>
>>> + Damien
>>>
>>> I just remembered Damien was facing a similar issue on Kendryte while
>>> running the test payload where
>>> it would not work if a string is used but worked fine if a single
>>> character is printed in a loop.
>>>
>>> @Damien : Did you figure out the cause for that issue you were seeing?
>>
>> I see lots of problems, not sure if they are related yet:
>> 1) On a clean build, make CROSS_COMPILE=riscv64-linux- PLATFORM=kendryte/k210
>> after a "make clean" or "git clone", the kendryte dts is *not* compiled. If I
>> modify the dts and run make again, then the dts gets compiled but throws out
>> warnings about #address-cells missing. The Kendryte dts is wrong and needs
>> updates. I updated with what I have for Linux which is what u-boot has too,
>> simplified, and still gets warnings while there are none on kernel compiles.
>> Since Kendryte is generic platform now, this may impact how initialization is
>> done.
>> 2) I see the same problem as Heinrich with the misa string: garbage is output.
>> As a little experiment, I made this change:
>>
>> diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
>> index 8c54c11..195b56b 100644
>> --- a/lib/sbi/riscv_asm.c
>> +++ b/lib/sbi/riscv_asm.c
>> @@ -11,6 +11,7 @@
>>  #include <sbi/riscv_encoding.h>
>>  #include <sbi/sbi_error.h>
>>  #include <sbi/sbi_platform.h>
>> +#include <sbi/sbi_string.h>
>>
>>  /* determine CPU extension, return non-zero support */
>>  int misa_extension_imp(char ext)
>> @@ -52,7 +53,7 @@ int misa_xlen(void)
>>  void misa_string(int xlen, char *out, unsigned int out_sz)
>>  {
>>         unsigned int i, pos = 0;
>> -       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
>> +       const char *valid_isa_order = "iemafdqclbjtpvnsuhkorwxyzg";
>>
>>         if (!out)
>>                 return;
>> @@ -79,10 +80,8 @@ void misa_string(int xlen, char *out, unsigned int out_sz)
>>                 }
>>         }
>>
>> -       for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz); i++) {
>> -               if (misa_extension_imp(valid_isa_order[i]))
>> -                       out[pos++] = valid_isa_order[i];
>> -       }
>> +       for (i = 0; i < sbi_strlen(valid_isa_order) && (pos < out_sz); i++)
>> +               out[pos++] = valid_isa_order[i];
>>
>>         if (pos < out_sz)
>>                 out[pos++] = '\0';
>>
>> so that all valid_isa_order characters are printed. And I get:
>>
>> Boot HART ISA       : rv64terrupt-controller
>>
>> Which looks like a piece of the dts...
>>
>> So, I think we have a build/linking/loading problem on our hands. Memory
>> corruption of data basically. That also likely explain why there is no string
>> output from the test payload but I do get single character outputs if I do
>> them.
>>
>> I tried changing the misa_string() code in different ways and each one gives a
>> different garbage but always the same garbage (rebooting several time gives the
>> same). So it looks like linking or loading problem.
>>
>> And the dts compilation needs addressing too:
>> 1) Erratic dtb build
>> 2) wrong error messages on dtb compilation
>>
>> Need to go back to linux-scsi for some patching. Will try to look into this
>> later if I have time. Anyone, feel free to tackle this !
>
> Playing again with this, I tried removing the embedded dtb for kendryte. And as
> expected, the misa_string becomes correct. Clearly, the embedded dtb is the
> root cause. It is overwriting data/bss. Still no output from test payload
> though.
>
> Looking at fw_base.S, I see the call to fw_platform_init that gets the address
> of the embedded dtb, but not sure if there is a relocation going on. If there
> is, the dtb size symbol (dt_k210_size) is unused. Without it, I do not see how
> relocation can work correctly.
>
> Assembler not being my thing, if someone can have a look...

git bisect point to my patch

d7f87d99a33b71b20af527c62e7ef95dcb61ee22
platform: kendryte/k210: fixup FDT

Where in the code is the memory allocated allowing for the FDT fixups?

Best regards

Heinrich


>
> Cheers.
>
>>
>>
>>>> It might be that some function is overwriting the area where the strings
>>>> are stored, e.g via the stack.
>>>>
>>>> Best regards
>>>>
>>>> Heinrich
>>>>
>>>>>> I disassembled build/lib/sbi/riscv_asm.o but could not find a problem.
>>>>>>
>>>>>> Do you have a suggestion how to analyze this further?
>>>>>>
>>>>>> Best regards
>>>>>>
>>>>>> Heinrich
>>>>>>
>>>>>>
>>>>>>
>>>>>> OpenSBI v0.8-29-g7701ea1
>>>>>>    ____                    _____ ____ _____
>>>>>>   / __ \                  / ____|  _ \_   _|
>>>>>>  | |  | |_ __   ___ _ __ | (___ | |_) || |
>>>>>>  | |  | | '_ \ / _ \ '_ \ \___ \|  _ < | |
>>>>>>  | |__| | |_) |  __/ | | |____) | |_) || |_
>>>>>>   \____/| .__/ \___|_| |_|_____/|____/_____|
>>>>>>         | |
>>>>>>         |_|
>>>>>>
>>>>>> Platform Name       : Kendryte K210
>>>>>> Platform Features   : timer
>>>>>> Platform HART Count : 2
>>>>>> Boot HART ID        : 0
>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>> BOOT HART Features  : none
>>>>>> BOOT HART PMP Count : 0
>>>>>> BOOT HART MHPM Count: 0
>>>>>> Firmware Base       : 0x80000000
>>>>>> Firmware Size       : 100 KB
>>>>>> Runtime SBI Version : 0.2
>>>>>>
>>>>>> MIDELEG : 0x0000000000000222
>>>>>> MEDELEG : 0x0000000000000109
>>>>>>
>>>>>>
>>>>>> --
>>>>>> opensbi mailing list
>>>>>> opensbi@lists.infradead.org
>>>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>
Heinrich Schuchardt Sept. 30, 2020, 11:12 a.m. UTC | #9
On 30.09.20 10:45, Heinrich Schuchardt wrote:
> On 30.09.20 10:22, Damien Le Moal wrote:
>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> Hello Atish,
>>>>>>>
>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>>>>
>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>>>
>>>>>>> looks a bit strange to me (see full output at the end of the mail).
>>>>>>
>>>>>> Yeah. It doesn't make any sense.
>>>>>>
>>>>>>> Assuming that the characters after rv64 are related to extensions I
>>>>>>> would expect every letter appearing only once.
>>>>>>>
>>>>>>
>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>>>>
>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>>>>> do not print out correctly.
>>>>>>>

<snip />

In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
the extensions

valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"

directly follows the embedded device tree:

.......................n........................
....#address-cells.#size-cells.compatible.bootar
gs.device_type.clock-frequency.i-cache-size.d-ca
che-size.mmu-type.reg.riscv,isa.status.#interrup
cells.interrupt-controller.linux,phandle.inter
rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
.*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..

When running fixups the device-tree becomes longer.

Whenever we do device-tree fixups we have to copy the device tree to a
different memory location with enough space for the fixups or we need
the linker script to leave enough space.

If OpenSBI is run from a NOR flash anyway we first have to copy the
device-tree to RAM before we can do any fixup.

Which way should we go:
- allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
- change the linker script to leave 1 KiB free space after the dtb

Best regards

Heinrich
Damien Le Moal Sept. 30, 2020, 11:21 a.m. UTC | #10
On Wed, 2020-09-30 at 13:12 +0200, Heinrich Schuchardt wrote:
> On 30.09.20 10:45, Heinrich Schuchardt wrote:
> > On 30.09.20 10:22, Damien Le Moal wrote:
> > > On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
> > > > On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> > > > > On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > On 9/29/20 9:05 PM, Atish Patra wrote:
> > > > > > > On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > Hello Atish,
> > > > > > > > 
> > > > > > > > on the Kendryte 210 MaixDuino the OpenSBI output line
> > > > > > > > 
> > > > > > > > Boot HART ISA       : rv64cicacsidcacsi
> > > > > > > > 
> > > > > > > > looks a bit strange to me (see full output at the end of the mail).
> > > > > > > 
> > > > > > > Yeah. It doesn't make any sense.
> > > > > > > 
> > > > > > > > Assuming that the characters after rv64 are related to extensions I
> > > > > > > > would expect every letter appearing only once.
> > > > > > > > 
> > > > > > > 
> > > > > > > That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> > > > > > > 
> > > > > > > > I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> > > > > > > > do not print out correctly.
> > > > > > > > 
> 
> <snip />
> 
> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
> the extensions
> 
> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
> 
> directly follows the embedded device tree:
> 
> .......................n........................
> ....#address-cells.#size-cells.compatible.bootar
> gs.device_type.clock-frequency.i-cache-size.d-ca
> che-size.mmu-type.reg.riscv,isa.status.#interrup
> cells.interrupt-controller.linux,phandle.inter
> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
> 
> When running fixups the device-tree becomes longer.
> 
> Whenever we do device-tree fixups we have to copy the device tree to a
> different memory location with enough space for the fixups or we need
> the linker script to leave enough space.
> 
> If OpenSBI is run from a NOR flash anyway we first have to copy the
> device-tree to RAM before we can do any fixup.
> 
> Which way should we go:
> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
> - change the linker script to leave 1 KiB free space after the dtb

You actually do not need to change the linker script I think. All you need is
to change scripts/d2c.sh to have awk add a bunch of zeroes at the end of the
array. Easy, but how much is "enough" is a hard question. This would be a very
weak fix. We likely will stumble on this bug again in the future if more fixups
are added...

The cleaner solution would be to do the fdt relocation exactly like on a normal
board which give an fdt, then do the fixups over there as there is enough room,
normally. That means tweaking fw_base.S.
Heinrich Schuchardt Sept. 30, 2020, 1:16 p.m. UTC | #11
On 30.09.20 13:21, Damien Le Moal wrote:
> On Wed, 2020-09-30 at 13:12 +0200, Heinrich Schuchardt wrote:
>> On 30.09.20 10:45, Heinrich Schuchardt wrote:
>>> On 30.09.20 10:22, Damien Le Moal wrote:
>>>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
>>>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
>>>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>> Hello Atish,
>>>>>>>>>
>>>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>>>>>>
>>>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>>>>>
>>>>>>>>> looks a bit strange to me (see full output at the end of the mail).
>>>>>>>>
>>>>>>>> Yeah. It doesn't make any sense.
>>>>>>>>
>>>>>>>>> Assuming that the characters after rv64 are related to extensions I
>>>>>>>>> would expect every letter appearing only once.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>>>>>>
>>>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>>>>>>> do not print out correctly.
>>>>>>>>>
>>
>> <snip />
>>
>> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
>> the extensions
>>
>> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
>>
>> directly follows the embedded device tree:
>>
>> .......................n........................
>> ....#address-cells.#size-cells.compatible.bootar
>> gs.device_type.clock-frequency.i-cache-size.d-ca
>> che-size.mmu-type.reg.riscv,isa.status.#interrup
>> cells.interrupt-controller.linux,phandle.inter
>> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
>> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
>>
>> When running fixups the device-tree becomes longer.
>>
>> Whenever we do device-tree fixups we have to copy the device tree to a
>> different memory location with enough space for the fixups or we need
>> the linker script to leave enough space.
>>
>> If OpenSBI is run from a NOR flash anyway we first have to copy the
>> device-tree to RAM before we can do any fixup.
>>
>> Which way should we go:
>> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
>> - change the linker script to leave 1 KiB free space after the dtb
>
> You actually do not need to change the linker script I think. All you need is
> to change scripts/d2c.sh to have awk add a bunch of zeroes at the end of the
> array. Easy, but how much is "enough" is a hard question. This would be a very
> weak fix. We likely will stumble on this bug again in the future if more fixups
> are added...

This works for me:

diff --git a/firmware/fw_payload.S b/firmware/fw_payload.S
index 9805d8c..8266f9d 100644
--- a/firmware/fw_payload.S
+++ b/firmware/fw_payload.S
@@ -108,6 +108,8 @@ fw_options:
        .globl fdt_bin
 fdt_bin:
        .incbin FW_PAYLOAD_FDT_PATH
+       /* 1 KiB of space for device tree updates */
+       . = . + 0x400;
 #endif

        .section .payload, "ax", %progbits
diff --git a/scripts/d2c.sh b/scripts/d2c.sh
index 821a995..8a28352 100755
--- a/scripts/d2c.sh
+++ b/scripts/d2c.sh
@@ -62,6 +62,9 @@ printf "const char __attribute__((aligned(%s)))
%s_start[] = {\n" "${OUTPUT_C_AL

 od -v -t x1 -An ${INPUT_PATH} | awk '{for (i=1; i<=NF; i++) printf "
0x%s,", $i; printf "\n"; }'

+# Add 1 KiB of unused space for device tree fixups
+echo dummy | awk '{for (j=1; j<=0x40; j++) {for (i=1; i<=0x10; i++)
printf " 0x00,"; printf "\n"}; }'
+
 printf "};\n"

 printf "const unsigned long %s_size = sizeof(%s_start);\n"
"${OUTPUT_C_PREFIX}" "${OUTPUT_C_PREFIX}"


The bad thing about this solution is that it does not cover a device
tree passed at a fixed memory location FW_PAYLOAD_FDT_ADDR.

>
> The cleaner solution would be to do the fdt relocation exactly like on a normal
> board which give an fdt, then do the fixups over there as there is enough room,
> normally. That means tweaking fw_base.S.
>

It is unclear to me what you mean by normal board.

The normal thing is that OpenSBI starts a payload and passes its
device-tree including all fixups.

You could instead have OpenSBI called with a devicetree in a fixed
memory position FW_PAYLOAD_FDT_ADDR. In this case you should not expect
any space for fixups. You must copy the device tree before fixups.

So having OpenSBI copy the devicetree before fixup is the cleanest solution.

I could not find any malloc() in OpenSBI. That is why I asked if I can
reserve the memory for the relocated device-tree with
sbi_scratch_alloc_offset().

Best regards

Heinrich
Atish Patra Sept. 30, 2020, 7:12 p.m. UTC | #12
On Wed, Sep 30, 2020 at 4:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 30.09.20 10:45, Heinrich Schuchardt wrote:
> > On 30.09.20 10:22, Damien Le Moal wrote:
> >> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
> >>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> >>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
> >>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>> Hello Atish,
> >>>>>>>
> >>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
> >>>>>>>
> >>>>>>> Boot HART ISA       : rv64cicacsidcacsi
> >>>>>>>
> >>>>>>> looks a bit strange to me (see full output at the end of the mail).
> >>>>>>
> >>>>>> Yeah. It doesn't make any sense.
> >>>>>>
> >>>>>>> Assuming that the characters after rv64 are related to extensions I
> >>>>>>> would expect every letter appearing only once.
> >>>>>>>
> >>>>>>
> >>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> >>>>>>
> >>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> >>>>>>> do not print out correctly.
> >>>>>>>
>
> <snip />
>
> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
> the extensions
>
> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
>
> directly follows the embedded device tree:
>
> .......................n........................
> ....#address-cells.#size-cells.compatible.bootar
> gs.device_type.clock-frequency.i-cache-size.d-ca
> che-size.mmu-type.reg.riscv,isa.status.#interrup
> cells.interrupt-controller.linux,phandle.inter
> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
>
> When running fixups the device-tree becomes longer.
>
> Whenever we do device-tree fixups we have to copy the device tree to a
> different memory location with enough space for the fixups or we need
> the linker script to leave enough space.
>
Thanks for getting to the bottom of this issue. But does this issue
solve the test payload issue Damien was facing ?

Looking at the fw_payload.bin, DT ends at 0x10d10 and payload starts
at 0x14000. The .rodata sections of the payload
starts at 0x15000. This is where the string in the payload resides. We
expand the DT by 1056 bytes. The payload section
is almost after 3KiB.

Do we still have another issue that corrupts data/bass sections of
payload running in S-mode?

> If OpenSBI is run from a NOR flash anyway we first have to copy the
> device-tree to RAM before we can do any fixup.
>
> Which way should we go:
> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
> - change the linker script to leave 1 KiB free space after the dtb
>

Currently, we expand the DT by 1024 + 32. So it has to be more than 1KB.

> Best regards
>
> Heinrich
Heinrich Schuchardt Sept. 30, 2020, 11:27 p.m. UTC | #13
On 9/30/20 9:12 PM, Atish Patra wrote:
> On Wed, Sep 30, 2020 at 4:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 30.09.20 10:45, Heinrich Schuchardt wrote:
>>> On 30.09.20 10:22, Damien Le Moal wrote:
>>>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
>>>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
>>>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>> Hello Atish,
>>>>>>>>>
>>>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>>>>>>
>>>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>>>>>
>>>>>>>>> looks a bit strange to me (see full output at the end of the mail).
>>>>>>>>
>>>>>>>> Yeah. It doesn't make any sense.
>>>>>>>>
>>>>>>>>> Assuming that the characters after rv64 are related to extensions I
>>>>>>>>> would expect every letter appearing only once.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>>>>>>
>>>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>>>>>>> do not print out correctly.
>>>>>>>>>
>>
>> <snip />
>>
>> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
>> the extensions
>>
>> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
>>
>> directly follows the embedded device tree:
>>
>> .......................n........................
>> ....#address-cells.#size-cells.compatible.bootar
>> gs.device_type.clock-frequency.i-cache-size.d-ca
>> che-size.mmu-type.reg.riscv,isa.status.#interrup
>> cells.interrupt-controller.linux,phandle.inter
>> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
>> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
>>
>> When running fixups the device-tree becomes longer.
>>
>> Whenever we do device-tree fixups we have to copy the device tree to a
>> different memory location with enough space for the fixups or we need
>> the linker script to leave enough space.
>>
> Thanks for getting to the bottom of this issue. But does this issue
> solve the test payload issue Damien was facing ?
>
> Looking at the fw_payload.bin, DT ends at 0x10d10 and payload starts
> at 0x14000. The .rodata sections of the payload
> starts at 0x15000. This is where the string in the payload resides. We
> expand the DT by 1056 bytes. The payload section
> is almost after 3KiB.
>
> Do we still have another issue that corrupts data/bass sections of
> payload running in S-mode?
>
>> If OpenSBI is run from a NOR flash anyway we first have to copy the
>> device-tree to RAM before we can do any fixup.
>>
>> Which way should we go:
>> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
>> - change the linker script to leave 1 KiB free space after the dtb
>>
>
> Currently, we expand the DT by 1024 + 32. So it has to be more than 1KB.

Atish, thanks for that hint. Yes 1024 fdt_reserved_memory_fixup() and 32
in fdt_cpu_fixup(). Aren't those fdt_open_into() calls bound to fail
because fdt == buf and bufsize < 2 * fdtsize?

/* First attempt to build converted tree at beginning of buffer */
tmp = buf;
/* But if that overlaps with the old tree... */
if (((tmp + newsize) > fdtstart) && (tmp < fdtend)) {
        /* Try right after the old tree instead */
        tmp = (char *)(uintptr_t)fdtend;
        if ((tmp + newsize) > ((char *)buf + bufsize))
               return -FDT_ERR_NOSPACE;  <<<<<<<<<<<<<<<<<<<<<<<<<<<<
}

My questions was if we should make a full copy or if we should try to
reserve memory inside the OpenSBI binary. What is your preference?

Best regards

Heinrich

>
>> Best regards
>>
>> Heinrich
>
>
>
Damien Le Moal Oct. 1, 2020, 2:04 a.m. UTC | #14
On 2020/10/01 4:12, Atish Patra wrote:
> On Wed, Sep 30, 2020 at 4:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>
>> On 30.09.20 10:45, Heinrich Schuchardt wrote:
>>> On 30.09.20 10:22, Damien Le Moal wrote:
>>>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
>>>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
>>>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
>>>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>>>>> Hello Atish,
>>>>>>>>>
>>>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
>>>>>>>>>
>>>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
>>>>>>>>>
>>>>>>>>> looks a bit strange to me (see full output at the end of the mail).
>>>>>>>>
>>>>>>>> Yeah. It doesn't make any sense.
>>>>>>>>
>>>>>>>>> Assuming that the characters after rv64 are related to extensions I
>>>>>>>>> would expect every letter appearing only once.
>>>>>>>>>
>>>>>>>>
>>>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
>>>>>>>>
>>>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
>>>>>>>>> do not print out correctly.
>>>>>>>>>
>>
>> <snip />
>>
>> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
>> the extensions
>>
>> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
>>
>> directly follows the embedded device tree:
>>
>> .......................n........................
>> ....#address-cells.#size-cells.compatible.bootar
>> gs.device_type.clock-frequency.i-cache-size.d-ca
>> che-size.mmu-type.reg.riscv,isa.status.#interrup
>> cells.interrupt-controller.linux,phandle.inter
>> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
>> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
>>
>> When running fixups the device-tree becomes longer.
>>
>> Whenever we do device-tree fixups we have to copy the device tree to a
>> different memory location with enough space for the fixups or we need
>> the linker script to leave enough space.
>>
> Thanks for getting to the bottom of this issue. But does this issue
> solve the test payload issue Damien was facing ?
> 
> Looking at the fw_payload.bin, DT ends at 0x10d10 and payload starts
> at 0x14000. The .rodata sections of the payload
> starts at 0x15000. This is where the string in the payload resides. We
> expand the DT by 1056 bytes. The payload section
> is almost after 3KiB.
> 
> Do we still have another issue that corrupts data/bass sections of
> payload running in S-mode?

Kendryte does not go to S-mode: no MMU ! That may be the cause of the payload
problem: M-mode ecalls may not be working as expected, resulting in the console
printk to fail. I can write individual characters (putc), but not normal printk.

> 
>> If OpenSBI is run from a NOR flash anyway we first have to copy the
>> device-tree to RAM before we can do any fixup.
>>
>> Which way should we go:
>> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
>> - change the linker script to leave 1 KiB free space after the dtb
>>
> 
> Currently, we expand the DT by 1024 + 32. So it has to be more than 1KB.

Where is the expansion done ? In the relocated resting place or inplace where
the previous stage placed the dtb ? If it is the latter, then the equivalent for
Kendryte would be to add 1024+32 to the dtb array created by d2c.sh. Simple but
ugly in my  opinion. The better method I think would be to use this array
generated by d2c.sh exactly as if this is a dtb provided by the previous boot
stage and do everything with it normally. That means changing fw_base.S. I
looked at it but understanding that assembler would take me too long. Please
have a look if you can. fw_platform_init() returns the embedded dtb address,
that is the address of the array generated by d2c.sh.

Cheers.
Atish Patra Oct. 1, 2020, 6:05 p.m. UTC | #15
On Wed, Sep 30, 2020 at 4:27 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 9/30/20 9:12 PM, Atish Patra wrote:
> > On Wed, Sep 30, 2020 at 4:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 30.09.20 10:45, Heinrich Schuchardt wrote:
> >>> On 30.09.20 10:22, Damien Le Moal wrote:
> >>>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
> >>>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> >>>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
> >>>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>> Hello Atish,
> >>>>>>>>>
> >>>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
> >>>>>>>>>
> >>>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
> >>>>>>>>>
> >>>>>>>>> looks a bit strange to me (see full output at the end of the mail).
> >>>>>>>>
> >>>>>>>> Yeah. It doesn't make any sense.
> >>>>>>>>
> >>>>>>>>> Assuming that the characters after rv64 are related to extensions I
> >>>>>>>>> would expect every letter appearing only once.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> >>>>>>>>
> >>>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> >>>>>>>>> do not print out correctly.
> >>>>>>>>>
> >>
> >> <snip />
> >>
> >> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
> >> the extensions
> >>
> >> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
> >>
> >> directly follows the embedded device tree:
> >>
> >> .......................n........................
> >> ....#address-cells.#size-cells.compatible.bootar
> >> gs.device_type.clock-frequency.i-cache-size.d-ca
> >> che-size.mmu-type.reg.riscv,isa.status.#interrup
> >> cells.interrupt-controller.linux,phandle.inter
> >> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
> >> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
> >>
> >> When running fixups the device-tree becomes longer.
> >>
> >> Whenever we do device-tree fixups we have to copy the device tree to a
> >> different memory location with enough space for the fixups or we need
> >> the linker script to leave enough space.
> >>
> > Thanks for getting to the bottom of this issue. But does this issue
> > solve the test payload issue Damien was facing ?
> >
> > Looking at the fw_payload.bin, DT ends at 0x10d10 and payload starts
> > at 0x14000. The .rodata sections of the payload
> > starts at 0x15000. This is where the string in the payload resides. We
> > expand the DT by 1056 bytes. The payload section
> > is almost after 3KiB.
> >
> > Do we still have another issue that corrupts data/bass sections of
> > payload running in S-mode?
> >
> >> If OpenSBI is run from a NOR flash anyway we first have to copy the
> >> device-tree to RAM before we can do any fixup.
> >>
> >> Which way should we go:
> >> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
> >> - change the linker script to leave 1 KiB free space after the dtb
> >>
> >
> > Currently, we expand the DT by 1024 + 32. So it has to be more than 1KB.
>
> Atish, thanks for that hint. Yes 1024 fdt_reserved_memory_fixup() and 32
> in fdt_cpu_fixup(). Aren't those fdt_open_into() calls bound to fail
> because fdt == buf and bufsize < 2 * fdtsize?
>

In the current cases, it just fdt_move gets called before this.
Even if it reaches this check, it will not fail as bufsize > newsize.

> /* First attempt to build converted tree at beginning of buffer */
> tmp = buf;
> /* But if that overlaps with the old tree... */
> if (((tmp + newsize) > fdtstart) && (tmp < fdtend)) {
>         /* Try right after the old tree instead */
>         tmp = (char *)(uintptr_t)fdtend;
>         if ((tmp + newsize) > ((char *)buf + bufsize))
>                return -FDT_ERR_NOSPACE;  <<<<<<<<<<<<<<<<<<<<<<<<<<<<
> }
>
> My questions was if we should make a full copy or if we should try to
> reserve memory inside the OpenSBI binary. What is your preference?
>

I see you have already sent that patch. I prefer the binary approach as well.

> Best regards
>
> Heinrich
>
> >
> >> Best regards
> >>
> >> Heinrich
> >
> >
> >
>
Atish Patra Oct. 1, 2020, 6:15 p.m. UTC | #16
On Wed, Sep 30, 2020 at 7:04 PM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>
> On 2020/10/01 4:12, Atish Patra wrote:
> > On Wed, Sep 30, 2020 at 4:12 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>
> >> On 30.09.20 10:45, Heinrich Schuchardt wrote:
> >>> On 30.09.20 10:22, Damien Le Moal wrote:
> >>>> On Wed, 2020-09-30 at 14:08 +0900, Damien Le Moal wrote:
> >>>>> On Tue, 2020-09-29 at 13:38 -0700, Atish Patra wrote:
> >>>>>> On Tue, Sep 29, 2020 at 12:38 PM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>> On 9/29/20 9:05 PM, Atish Patra wrote:
> >>>>>>>> On Mon, Sep 28, 2020 at 3:18 AM Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>>>>>> Hello Atish,
> >>>>>>>>>
> >>>>>>>>> on the Kendryte 210 MaixDuino the OpenSBI output line
> >>>>>>>>>
> >>>>>>>>> Boot HART ISA       : rv64cicacsidcacsi
> >>>>>>>>>
> >>>>>>>>> looks a bit strange to me (see full output at the end of the mail).
> >>>>>>>>
> >>>>>>>> Yeah. It doesn't make any sense.
> >>>>>>>>
> >>>>>>>>> Assuming that the characters after rv64 are related to extensions I
> >>>>>>>>> would expect every letter appearing only once.
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> That's correct. See 3.1.1. Machine ISA Register misa in priv spec.
> >>>>>>>>
> >>>>>>>>> I tried to add sbi_printf() statements to lib/sbi/riscv_asm.c but they
> >>>>>>>>> do not print out correctly.
> >>>>>>>>>
> >>
> >> <snip />
> >>
> >> In build/platform/kendryte/k210/firmware/fw_payload.bin the string for
> >> the extensions
> >>
> >> valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg"
> >>
> >> directly follows the embedded device tree:
> >>
> >> .......................n........................
> >> ....#address-cells.#size-cells.compatible.bootar
> >> gs.device_type.clock-frequency.i-cache-size.d-ca
> >> che-size.mmu-type.reg.riscv,isa.status.#interrup
> >> cells.interrupt-controller.linux,phandle.inter
> >> rupts-extended......iemafdqclbjtpvnsuhkorwxyzg..
> >> .*..z*..t*..n*..h*..b*..\*..V*..P*..J*..D*..>*..
> >>
> >> When running fixups the device-tree becomes longer.
> >>
> >> Whenever we do device-tree fixups we have to copy the device tree to a
> >> different memory location with enough space for the fixups or we need
> >> the linker script to leave enough space.
> >>
> > Thanks for getting to the bottom of this issue. But does this issue
> > solve the test payload issue Damien was facing ?
> >
> > Looking at the fw_payload.bin, DT ends at 0x10d10 and payload starts
> > at 0x14000. The .rodata sections of the payload
> > starts at 0x15000. This is where the string in the payload resides. We
> > expand the DT by 1056 bytes. The payload section
> > is almost after 3KiB.
> >
> > Do we still have another issue that corrupts data/bass sections of
> > payload running in S-mode?
>
> Kendryte does not go to S-mode: no MMU ! That may be the cause of the payload
> problem: M-mode ecalls may not be working as expected, resulting in the console
> printk to fail. I can write individual characters (putc), but not normal printk.
>
I was just asking about the test payload. I don't think there is any
issue with ecalls
as you were able to run sbi_ecall_console_putc.

The only difference between sbi_ecall_console_puts &
sbi_ecall_console_putc is that
it accesses a string from the .rodata section.

> >
> >> If OpenSBI is run from a NOR flash anyway we first have to copy the
> >> device-tree to RAM before we can do any fixup.
> >>
> >> Which way should we go:
> >> - allocate space for a copy of the fdt with sbi_scratch_alloc_offset, or
> >> - change the linker script to leave 1 KiB free space after the dtb
> >>
> >
> > Currently, we expand the DT by 1024 + 32. So it has to be more than 1KB.
>
> Where is the expansion done ? In the relocated resting place or inplace where
> the previous stage placed the dtb ? If it is the latter, then the equivalent for
> Kendryte would be to add 1024+32 to the dtb array created by d2c.sh. Simple but
> ugly in my  opinion. The better method I think would be to use this array
> generated by d2c.sh exactly as if this is a dtb provided by the previous boot
> stage and do everything with it normally. That means changing fw_base.S. I
> looked at it but understanding that assembler would take me too long. Please
> have a look if you can. fw_platform_init() returns the embedded dtb address,
> that is the address of the array generated by d2c.sh.
>
> Cheers.
>
> --
> Damien Le Moal
> Western Digital Research
diff mbox series

Patch

diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
index 8c54c11..a0c95a2 100644
--- a/lib/sbi/riscv_asm.c
+++ b/lib/sbi/riscv_asm.c
@@ -81,7 +81,7 @@  void misa_string(int xlen, char *out, unsigned int out_sz)

        for (i = 0; i < array_size(valid_isa_order) && (pos < out_sz);
i++) {
                if (misa_extension_imp(valid_isa_order[i]))
-                       out[pos++] = valid_isa_order[i];
+                       out[pos++] = '0' + i;
        }

        if (pos < out_sz)

the output becomes

Boot HART ISA       : rv6403567;<@BCDHI

I am clueless why valid_isa_order[i] is not evaluated correctly.

When my changes are:

diff --git a/lib/sbi/riscv_asm.c b/lib/sbi/riscv_asm.c
index 8c54c11..b1bbfc4 100644
--- a/lib/sbi/riscv_asm.c
+++ b/lib/sbi/riscv_asm.c
@@ -12,6 +12,8 @@ 
 #include <sbi/sbi_error.h>
 #include <sbi/sbi_platform.h>

+static const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";
+
 /* determine CPU extension, return non-zero support */
 int misa_extension_imp(char ext)
 {
@@ -52,7 +54,6 @@  int misa_xlen(void)
 void misa_string(int xlen, char *out, unsigned int out_sz)
 {
        unsigned int i, pos = 0;
-       const char valid_isa_order[] = "iemafdqclbjtpvnsuhkorwxyzg";

        if (!out)
                return;