On Thu, 2019-11-07 at 11:48 +0530, Anup Patel wrote: > 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...
Thanks for looking into this issue! I successfully tested it on QEMU, I had to add a short delay between sending the IPIs to trigger the problem. We might still run into problems however. Right now, we are assuming that the main hart is the last one to enter OpenSBI. If this is not the case (some delay when handling the IPI), we will have the same problem again. To fix this we could pass the hart mask, containing all harts that have entered U-Boot, to OpenSBI and wait for all harts to be running in OpenSBI. I am not sure how realistic this scenario is, so this might not be needed. Regards, Lukas _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
