[U-Boot] [PATCH 4/8] riscv: andes_plic: Fix some wrong configurations

Rick Chen rickchen36 at gmail.com
Fri Nov 8 07:27:12 UTC 2019


Hi Atish

>
> Hi Atish
>
> >
> > On Thu, 2019-11-07 at 19:41 +0800, Rick Chen wrote:
> > > Hi Anup & Lukas
> > >
> > > Anup Patel <anup at brainfault.org> 於 2019年11月7日 週四 下午6:44寫道:
> > > > On Thu, Nov 7, 2019 at 3:11 PM Auer, Lukas
> > > > <lukas.auer at 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 <rickchen36 at gmail.com
> > > > > > > wrote:
> > > > > > > Hi Anup
> > > > > > >
> > > > > > > > On Thu, Nov 7, 2019 at 10:45 AM Anup Patel <
> > > > > > > > anup at brainfault.org> wrote:
> > > > > > > > > On Thu, Nov 7, 2019 at 7:04 AM Rick Chen <
> > > > > > > > > rickchen36 at gmail.com> wrote:
> > > > > > > > > > Hi Anup
> > > > > > > > > >
> > > > > > > > > > > On Wed, Nov 6, 2019 at 2:51 PM Rick Chen <
> > > > > > > > > > > rickchen36 at gmail.com> wrote:
> > > > > > > > > > > > Hi Anup
> > > > > > > > > > > >
> > > > > > > > > > > > > On Wed, Nov 6, 2019 at 2:18 PM Anup Patel <
> > > > > > > > > > > > > anup at brainfault.org> wrote:
> > > > > > > > > > > > > > On Wed, Nov 6, 2019 at 12:14 PM Rick Chen <
> > > > > > > > > > > > > > rickchen36 at gmail.com> wrote:
> > > > > > > > > > > > > > > Hi Anup
> > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > On Tue, Nov 5, 2019 at 7:19 AM Rick Chen <
> > > > > > > > > > > > > > > > rickchen36 at gmail.com> wrote:
> > > > > > > > > > > > > > > > > Hi Anup
> > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > On Thu, Oct 31, 2019 at 1:42 PM Anup
> > > > > > > > > > > > > > > > > > > Patel <anup at brainfault.org> wrote:
> > > > > > > > > > > > > > > > > > > > On Thu, Oct 31, 2019 at 6:30 AM
> > > > > > > > > > > > > > > > > > > > Alan Kao <alankao at 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 <
> > > > > > > > > > > > > > > > > > > > > > rickchen36 at gmail.com> wrote:
> > > > > > > > > > > > > > > > > > > > > > > Hi Bin
> > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > Hi Rick,
> > > > > > > > > > > > > > > > > > > > > > > >
> > > > > > > > > > > > > > > > > > > > > > > > On Fri, Oct 25, 2019 at
> > > > > > > > > > > > > > > > > > > > > > > > 2:18 PM Andes <
> > > > > > > > > > > > > > > > > > > > > > > > uboot at andestech.com> wrote:
> > > > > > > > > > > > > > > > > > > > > > > > > From: Rick Chen <
> > > > > > > > > > > > > > > > > > > > > > > > > rick at 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 ?
> > > >
> > > > 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 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 ?
> > >
> > > I have try your changes about boot_hart for U-Boot SPL with OpenSBI,
> > > preferred_boot_hart_v2 branch
> > > It still encounter some booting problems. I try to find out the root
> > > cause but in vain.
> > >
> >
> > Just wanted to make sure that you have tried this patch.
> >
> > http://lists.infradead.org/pipermail/opensbi/2019-November/000672.html
> >
> > We should investigate the issue why it did not work for you if this
> > patch did not work for you.
>
> Yes, I try with this
> commit 831aa3c1ad2546a2b35ddf5b1aa0ce91cdc7fe89
> firmware: Add preferred boot HART field in struct fw_dynamic_info
>
> It fail randomly yesterday, but this morning I try several times it will pass.
> I will keep trying.
>

I have figure out one fail case which is belong to main hart of U-Boot
SPL is not the last hart while entering OpenSBI

case 1 (fail)

hb *0x1000000

Thread 2 hit Breakpoint 3, 0x0000000001000000 in ?? ()
(gdb) info threads
  Id   Target Id         Frame
  1    Thread 1 (hart 1) 0x0000000001000000 in ?? ()
* 2    Thread 2 (hart 2) 0x0000000001000000 in ?? ()
  3    Thread 3 (hart 3) secondary_hart_loop () at arch/riscv/cpu/start.S:416
  4    Thread 4 (hart 4) secondary_hart_loop () at arch/riscv/cpu/start.S:416

(gdb) p/x gd->arch.boot_hart
$3 = 0x1

main hart is hart 1 in U-Boot SPL, hart 1 have arrived OpenSBI, but
hart 3, 4 still run in U-Boot SPL

c

U-Boot console print message

U-Boot SPL 2019.10-00603-g850848e-dirty (Nov 08 2019 - 14:37:54 +0800)
Trying to boot from RAM

U-Boot 2019.10-00603-g850848e-dirty (Nov 08 2019 - 14:37:54 +0800)

DRAM:  1 GiB
Flash:

<== hang here

Thanks
Rick


More information about the U-Boot mailing list