On Thu, Nov 7, 2019 at 11:40 AM Rick Chen <rickche...@gmail.com> wrote:
>
> Hi Anup
>
> >
> > On Thu, Nov 7, 2019 at 10:45 AM Anup Patel <a...@brainfault.org> wrote:
> > >
> > > On Thu, Nov 7, 2019 at 7:04 AM Rick Chen <rickche...@gmail.com> wrote:
> > > >
> > > > Hi Anup
> > > >
> > > > > On Wed, Nov 6, 2019 at 2:51 PM Rick Chen <rickche...@gmail.com> wrote:
> > > > > >
> > > > > > Hi Anup
> > > > > >
> > > > > > >
> > > > > > > On Wed, Nov 6, 2019 at 2:18 PM Anup Patel <a...@brainfault.org>
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Wed, Nov 6, 2019 at 12:14 PM Rick Chen
> > > > > > > > <rickche...@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi Anup
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 5, 2019 at 7:19 AM Rick Chen
> > > > > > > > > > <rickche...@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Anup
> > > > > > > > > > >
> > > > > > > > > > > > > On Thu, Oct 31, 2019 at 1:42 PM Anup Patel
> > > > > > > > > > > > > <a...@brainfault.org> wrote:
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > On Thu, Oct 31, 2019 at 6:30 AM Alan Kao
> > > > > > > > > > > > > > <alan...@andestech.com> wrote:
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Hi Bin,
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Thanks for the critics. Comments below.
> > > > > > > > > > > > > > > On Wed, Oct 30, 2019 at 06:38:00PM +0800, Bin
> > > > > > > > > > > > > > > Meng wrote:
> > > > > > > > > > > > > > > > Hi Rick,
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Wed, Oct 30, 2019 at 10:50 AM Rick Chen
> > > > > > > > > > > > > > > > <rickche...@gmail.com> wrote:
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Hi Bin
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Hi Rick,
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > On Fri, Oct 25, 2019 at 2:18 PM Andes
> > > > > > > > > > > > > > > > > > <ub...@andestech.com> wrote:
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > From: Rick Chen <r...@andestech.com>
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > It will work fine due to hart 0 always
> > > > > > > > > > > > > > > > > > > will be main
> > > > > > > > > > > > > > > > > > > hart coincidentally. When develop SPL
> > > > > > > > > > > > > > > > > > > flow, I try to
> > > > > > > > > > > > > > > > > > > force other harts to be main hart. And it
> > > > > > > > > > > > > > > > > > > will go
> > > > > > > > > > > > > > > > > > > wrong in sending IPI flow. So fix it.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > Fix what? Does this commit contain 2 fixes,
> > > > > > > > > > > > > > > > > > or just 1 fix?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > Yes, it include two fixs. But they will cause
> > > > > > > > > > > > > > > > > one negative result
> > > > > > > > > > > > > > > > > that only hart 0 can send ipi to other harts.
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > Having this fix, any hart can be main
> > > > > > > > > > > > > > > > > > > hart in U-Boot SPL
> > > > > > > > > > > > > > > > > > > theoretically, but it still fail
> > > > > > > > > > > > > > > > > > > somewhere. After dig in
> > > > > > > > > > > > > > > > > > > and found there is an assumption that
> > > > > > > > > > > > > > > > > > > hart 0 shall be
> > > > > > > > > > > > > > > > > > > main hart in OpenSbi.
> > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > So does this mean there is a bug in OpenSBI
> > > > > > > > > > > > > > > > > > too?
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > I am not sure if it is a bug. Maybe it is a
> > > > > > > > > > > > > > > > > compatible issue.
> > > > > > > > > > > > > > > > > There is a limitation that only hart 0 can be
> > > > > > > > > > > > > > > > > main hart in OpenSBI.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > I don't think OpenSBI has such limitation.
> > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Please check the source.
> > > > > > > > > > > > > > > https://github.com/riscv/opensbi/blob/master/firmware/fw_base.S#L54
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > Apparently, the FIRST TWO LINEs of the
> > > > > > > > > > > > > > > initialization are the
> > > > > > > > > > > > > > > 1. get hart ID.
> > > > > > > > > > > > > > > 2. determine which route to take based on their
> > > > > > > > > > > > > > > ID respectively.
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > So, I do think OpenSBI has this signature, if you
> > > > > > > > > > > > > > > are not willing to call it
> > > > > > > > > > > > > > > a limitation.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > This dependency on hart id #0 was not there until
> > > > > > > > > > > > > > we added self-relocation
> > > > > > > > > > > > > > in OpenSBI for FW_DYNAMIC.
> > > > > > > > > > > > > >
> > > > > > > > > > > > > > I will try to fix this in OpenSBI but we might
> > > > > > > > > > > > > > end-up having boot_lottery.
> > > > > > > > > > > > >
> > > > > > > > > > > > > I have send a patch to fix this OpenSBI:
> > > > > > > > > > > > > "[PATCH] firmware: Introduce relocation lottery"
> > > > > > > > > > > > >
> > > > > > > > > > > > > Can you try above patch and see if that helps ?
> > > > > > > > > > > > >
> > > > > > > > > > > > > It will be great if you can provide Tested-by to my
> > > > > > > > > > > > > patch as well.
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I can not find this patch in mailing list.
> > > > > > > > > > > Can you provide a hyperlink ?
> > > > > > > > > >
> > > > > > > > > > You can try latest riscv/opensbi master.
> > > > > > > > > >
> > > > > > > > > > I have tested the patch on SiFive Unleashed multiple times.
> > > > > > > > >
> > > > > > > > > I have tried this patch, but it fail
> > > > > > > > > firmware: Introduce relocation lottery(
> > > > > > > > > 98f4a208995b027662a7b04a25e4fa5df5f3eefe)
> > > > > > > > >
> > > > > > > > > The scenario was as below:
> > > > > > > > > There are 4 harts run in U-Boot SPL, hart 0 play as main hart.
> > > > > > > > > The hart 1 will receive ipi and come into OpenSBI(0x1000000)
> > > > > > > > > from
> > > > > > > > > U-Boot SPL(0x0), meanwhile hart 0,2,3 still run in U-Boot SPL.
> > > > > > > > > Then hart 1 will do _relocate_copy_to_lower which will copy
> > > > > > > > > data from
> > > > > > > > > 0x1000000 to 0x0.
> > > > > > > > > And it will corrupt U-Boot SPL.
> > > > > > > >
> > > > > > > > The self-relocation in OpenSBI firmwares ensures that OpenSBI
> > > > > > > > firmware
> > > > > > > > are moved to the FW_TEXT_START before entering C code. This
> > > > > > > > helps
> > > > > > > > us load OpenSBI firmwares anywhere in RAM.
> > > > > > > >
> > > > > > > > However, OpenSBI firmwares don't know where the U-Boot SPL is
> > > > > > > > running.
> > > > > > > >
> > > > > > > > In your case, both OpenSBI FW_DYNAMIC and U-Boot SPL are linked
> > > > > > > > to
> > > > > > > > address same address 0x0. This means secondary HARTs cannot
> > > > > > > > safely
> > > > > > > > wait while primary HART enters OpenSBI. You should hold
> > > > > > > > secondary HARTs
> > > > > > > > in U-Boot SPL only till OpenSBI FW_DYNAMIC and U-Boot proper are
> > > > > > > > loaded in RAM by primary HART. All your HARTs should jump to
> > > > > > > > OpenSBI
> > > > > > > > at the same time after everything is loaded in RAM.
> > > > > > >
> > > > > > > I see the issue now.
> > > > > > >
> > > > > > > The U-Boot SPL is first letting secondary HART jump to OpenSBI
> > > > > > > and primary
> > > > > > > HART jumps to OpenSBI at the end.
> > > > > > > (Refer, jump_to_image_no_args() in arch/riscv/lib/spl.c)
> > > > > > >
> > > > > > > The real issue is FW_TEXT_START being same as U-Boot SPL
> > > > > > > TEXT_START.
> > > > > > >
> > > > > > > If possible please change TEXT base for U-Boot SPL or OpenSBI. I
> > > > > > > think
> > > > > > > changing U-Boot SPL TEXT_START would be convenient since this
> > > > > > > series
> > > > > > > is under review. Thoughts ?
> > > > > >
> > > > > > Yes.
> > > > > > I know it can avoid corrupting issue with changing U-Boot SPL
> > > > > > TEXT_START not equal to OpenSBI TEXT base.
> > > > >
> > > > > I think this issue will be seen on U-Boot SPL running on QEMU as well.
> > > > >
> > > > > >
> > > > > > With the following changes, U-Boot SPL text base can equal to
> > > > > > OpenSBI text base
> > > > > > 1 U-Boot pass main hart information (a2) when jumping to OpenSBI
> > > > > > 2 OpenSBI pick up $a2 to keep playing as main hart, other harts go
> > > > > > to
> > > > > > _wait_relocate_copy_done
> > > > >
> > > > > Overall it's a good suggestion but we cannot use a2 register because
> > > > > this
> > > > > will break FW_JUMP and FW_PAYLOAD. Instead, we should pass preferred
> > > > > boot HART id in struct fw_dynamic_info.
> > > >
> > > > Sorry, what I want to say shall be a3.
> > > >
> > > > >
> > > > > I have a patch for this in preferred_boot_hart_v1 branch of
> > > > > https://github.com/avpatel/opensbi.git
> > > > >
> > > > > Can you try OpenSBI from above branch ?
> > > > >
> > > > > You will have to update the "struct fw_dynamic_info" passed to
> > > > > OpenSBI by U-Boot SPL.
> > > >
> > > > Main hart will pass struct "fw_dynamic_info" to OpenSBI by U-Boot SPL.
> > > > But other harts will NOT pass struct "fw_dynamic_info" to OpenSBI by
> > > > U-Boot SPL.
> > >
> > > That's wrong in U-Boot SPL.
> > >
> > > All HARTs have to follow FW_DYNAMIC protocol and pass
> > > "struct fw_dynamic_info" pointer in 'a2' register.
> > >
> > > >
> > > > So if U-Boot SPL can pass main hart information via a3, OpenSBI just
> > > > have the following change
> > > > blt zero, a6, _wait_relocate_copy_done
> > > > change to
> > > > bne a3, a6, _wait_relocate_copy_done
> > > > before this commit
> > > > 98f4a208995b027662a7b04a25e4fa5df5f3eefe
> > > > firmware: Introduce relocation lottery
> > >
> > > What about FW_JUMP and FW_PAYLOAD? We have no way of passing
> > > value in a3 for these firmwares because these are not booted by U-Boot
> > > SPL.
> > >
> > > Also, U-Boot-2019.10 already uses U-Boot SPL support which does not
> > > pass anything in 'a3' register.
> > >
> > > We should definitely use "struct fw_dynamic_info" for this so that we can
> > > maintain backward compatibility as well.
> > >
> > > Please make sure that U-Boot SPL passes "struct fw_dynamic_info"
> > > pointer in 'a2' register for all HARTs.
> > >
> > > >
> > > > But after this commit 98f4a, main hart become chosen from lottery
> > > > mechanism.
> > > > Maybe I will prefer to change U-Boot SPL text base not overlap with
> > > > OpenSBI text start. :)
> > >
> > > Like I mentioned, we have this issue for U-Boot SPL on QEMU as well. It's
> > > just that most of us did not notice it for U-Boot SPL on QEMU.
> > >
> > > Let's fix this in the right way from start itself.
> >
> > I double checked spl_invoke_opensbi() and it is doing the right thing
> > by passing "struct fw_dyanmic_info" pointer in 'a2' register.
> > (Refer, common/spl/spl_opensbi.c)
> >
> > Not sure, why it is not passing 'a2' register correctly for you ??
> >
>
> Yes, you are right. I reply too quickly.
> Other harts will pass struct fw_dyanmic_info in a2 to OpenSBI.
>
> Thanks for your corrections
No problem, I am happy to help.
BTW, I tried to play around with U-Boot SPL on QEMU.
Maybe below changes can help you...
diff --git a/common/spl/spl_opensbi.c b/common/spl/spl_opensbi.c
index a6b4480ed2..79ee7edcf9 100644
--- a/common/spl/spl_opensbi.c
+++ b/common/spl/spl_opensbi.c
@@ -69,6 +69,7 @@ void spl_invoke_opensbi(struct spl_image_info *spl_image)
opensbi_info.next_addr = uboot_entry;
opensbi_info.next_mode = FW_DYNAMIC_INFO_NEXT_MODE_S;
opensbi_info.options = SBI_SCRATCH_NO_BOOT_PRINTS;
+ opensbi_info.boot_hart = gd->arch.boot_hart;
opensbi_entry = (void (*)(ulong, ulong, ulong))spl_image->entry_point;
invalidate_icache_all();
diff --git a/include/opensbi.h b/include/opensbi.h
index 9f1d62e7dd..29a95fdfb6 100644
--- a/include/opensbi.h
+++ b/include/opensbi.h
@@ -11,7 +11,7 @@
#define FW_DYNAMIC_INFO_MAGIC_VALUE 0x4942534f
/** Maximum supported info version */
-#define FW_DYNAMIC_INFO_VERSION 0x1
+#define FW_DYNAMIC_INFO_VERSION 0x2
/** Possible next mode values */
#define FW_DYNAMIC_INFO_NEXT_MODE_U 0x0
@@ -35,6 +35,8 @@ struct fw_dynamic_info {
unsigned long next_mode;
/** Options for OpenSBI library */
unsigned long options;
+ /** Preferred boot HART id */
+ unsigned long boot_hart;
} __packed;
#endif
Regards,
Anup
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
