| Message ID | 3e977253-0836-e76f-964b-49b7f2541fde@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | OpenSBI: Boot HART ISA display | expand |
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
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 > > >
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 > > > > > > >
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 >>> >>> >>> >> > >
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 > > > > > > > >
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 > > > > > > > >
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
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 >
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
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.
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
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
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 > > >
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.
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 > > > > > > >
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 --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;
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