[PATCH 3/7] riscv: Use NULL as a sentinel value for smp_call_function
Rick Chen
rickchen36 at gmail.com
Thu Sep 10 10:09:57 CEST 2020
Hi Sean
> On 9/9/20 5:01 AM, Rick Chen wrote:
> > Hi Sean
> >
> >> Hi Sean
> >>
> >>> Some IPIs may already be pending when U-Boot is started. This could be a
> >>> problem if a secondary hart tries to handle an IPI before the boot hart has
> >>> initialized the IPI device.
> >>>
> >>> This commit uses NULL as a sentinel for secondary harts so they know when
> >>> the IPI is initialized, and it is safe to use the IPI API. The smp addr
> >>> parameter is initialized to NULL by board_init_f_init_reserve. Before this,
> >>> secondary harts wait in wait_for_gd_init.
> >>>
> >>> This imposes a minor restriction because harts may no longer jump to NULL.
> >>> However, given that the RISC-V debug device is likely to be located at
> >>> 0x400, it is unlikely for any RISC-V implementation to have usable ram
> >>> located at 0x0.
> >>
> >> The ram location of AE350 is at 0x0.
>
> Huh. Does it not have a debug device?
No, our debug device is not in here.
>
> >>
> >>>
> >>> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> >>> ---
> >>>
> >>> arch/riscv/lib/smp.c | 26 ++++++++++++++++++++++----
> >>> 1 file changed, 22 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/lib/smp.c b/arch/riscv/lib/smp.c
> >>> index ab6d8bd7fa..8c25755330 100644
> >>> --- a/arch/riscv/lib/smp.c
> >>> +++ b/arch/riscv/lib/smp.c
> >>> @@ -18,6 +18,12 @@ static int send_ipi_many(struct ipi_data *ipi, int wait)
> >>> u32 reg;
> >>> int ret, pending;
> >>>
> >>> + /* NULL is used as a sentinel value */
> >>> + if (!ipi->addr) {
> >>> + pr_err("smp_function cannot be set to 0x0\n");
> >>> + return -EINVAL;
> >>> + }
> >>> +
> >>
> >> This conflict with memory configurations of AE350.
> >> Please check about doc\board\AndesTech\ax25-ae350.rst, and you can
> >> find BBL is configured as zero address on AE350 platform.
>
> Ok, that is a strange choice because any accidental NULL-pointer
> dereference turns into code modification. In the next revision, I will
> add an arch.ipi[reg].valid variable for the same prupose, instead of
> re-using addr.
>
> >>> cpus = ofnode_path("/cpus");
> >>> if (!ofnode_valid(cpus)) {
> >>> pr_err("Can't find cpus node!\n");
> >>> @@ -50,11 +56,16 @@ static int send_ipi_many(struct ipi_data *ipi, int wait)
> >>> continue;
> >>> #endif
> >>>
> >>> - gd->arch.ipi[reg].addr = ipi->addr;
> >>> gd->arch.ipi[reg].arg0 = ipi->arg0;
> >>> gd->arch.ipi[reg].arg1 = ipi->arg1;
> >>>
> >>> - __smp_mb();
> >
> > Why do you add this in [PATCH 2/7] but remove it in [PATCH 3/7] ?
>
> Because conceptually, patch 2 is independent of this patch. It is still
> a bug even if this patch is not applied. I think by making this change
> over two patches, it is more obvious why the barrier was added, and then
> weakened, as opposed to if I made the change in one patch.
OK.
Thanks for explanations.
>
> >
> > Thanks,
> > Rick
> >
> >>> + /*
> >>> + * Ensure addr only becomes non-NULL when arg0 and arg1 are
> >>> + * valid. An IPI may already be pending on other harts, so we
> >>> + * need a way to signal that the IPI device has been
> >>> + * initialized, and that it is ok to call the function.
> >>> + */
> >>> + __smp_store_release(&gd->arch.ipi[reg].addr, ipi->addr);
> >>
> >> It is too tricky and hack by using zero address to be a signal for the
> >> other pending harts waiting the IPI device been initialized.
> >>
> >>>
> >>> ret = riscv_send_ipi(reg);
> >>> if (ret) {
> >>> @@ -83,9 +94,16 @@ void handle_ipi(ulong hart)
> >>> if (hart >= CONFIG_NR_CPUS)
> >>> return;
> >>>
> >>> - __smp_mb();
> >>> + smp_function = (void (*)(ulong, ulong, ulong))
> >>> + __smp_load_acquire(&gd->arch.ipi[hart].addr);
> >>> + /*
> >>> + * If the function is NULL, then U-Boot has not requested the IPI. The
> >>> + * IPI device may not be initialized, so all we can do is wait for
> >>> + * U-Boot to initialize it and send an IPI
> >>> + */
> >>> + if (!smp_function)
> >>> + return;
> >>
> >> It will boot BBL+Kernel payload fail here on AE350 platforms with this check.
> >>
> >> Thanks,
> >> Rick
> >>
> >>>
> >>> - smp_function = (void (*)(ulong, ulong, ulong))gd->arch.ipi[hart].addr;
> >>> invalidate_icache_all();
> >>>
> >>> /*
> >>> --
> >>> 2.28.0
> >>>
>
More information about the U-Boot
mailing list