On Thu, 2019-11-07 at 17:57 +0530, Anup Patel wrote: > On Thu, Nov 7, 2019 at 5:14 PM Auer, Lukas > <lukas.a...@aisec.fraunhofer.de> wrote: > > On Thu, 2019-11-07 at 16:14 +0530, Anup Patel wrote: > > > On Thu, Nov 7, 2019 at 3:11 PM Auer, Lukas > > > <lukas.a...@aisec.fraunhofer.de> wrote: > > > > 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. > > > > > > I agree that we might still run into this issue if primary HART enters > > > OpenSBI before secondary HARTs. I think this situation can only > > > happen on QEMU where each CPU is a thread running on host but > > > very unlikely/impossible on real HW. > > > > > > Maybe a delay on primary HART in U-Boot SPL after SMP calls to > > > secondary HARTs and before jumping to OpenSBI ? > > > > > > > You are right, I don't think we will ever see this on real hardware. I > > can add a check to ensure all harts have processed the IPI before > > jumping to OpenSBI on the main hart. A simple delay is probably already > > sufficient however. > > Yes, please change spl_opensbi like you described. > > Maybe also add some comments about the issues we discussed here.
Good idea, I will add the changes and describe the issue in a comment. Regards, Lukas > > > > Regarding hart_mask in fw_dynamic_info, I think the issue will be the > > > size of the hart_mask. It is possible in-future SOC vendors come-up > > > with SOC having huge number of HARTs OR SOC with discontinuous > > > HART IDs which can cause a 64bit hart_mask to be not sufficient for > > > all HARTs. > > > > > > Also, waiting for all HARTs to enter OpenSBI will be one more wait-loop > > > in fw_base.S which will add to the boot-time as well. > > > > > > > I agree, it's best to keep everything simple here. > > Cool, I guess we are in-sync. > > > > I still think the root cause of the issue is that TEXT_START of > > > U-Boot SPL and OpenSBI FW_DYNAMIC is same. Maybe we can > > > insist SOC vendors to not use same TEXT_START ? > > > > > > > That is definitely the best solution. I am not familiar with the > > details of the Andes platform, so unsure if that would work on it. On > > the SiFive Unleashed board this will be possible as soon as we have a > > working DRAM driver. U-Boot SPL could then run from L2 scratchpad > > memory instead of DRAM. > > Yes, this won't be a problem on SiFive Unleashed. > > Regards, > Anup _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
