[PATCH v5] lib:sbi:platform:generic: Improved mlevel imsic check and init.
Anup Patel
anup at brainfault.org
Wed May 8 04:54:03 PDT 2024
On Tue, May 7, 2024 at 8:34 PM Cheng Yang <yangcheng.work at foxmail.com> wrote:
>
> >On Wed, Apr 10, 2024 at 2:58 PM Cheng Yang <yangcheng.work at foxmail.com> wrote:
> >>
> >> The current mlevel imsic check is only for the platform, which
> >> may cause hart without imsic in the platform to trigger an
> >> illegal instruction exception when initializing imsic. For
> >> example, the platform contains a management hart that only
> >> supports wired interrupts.
> >>
> >> This patch will check whether each hart has imsic according
> >> to fdt and record it in a bitmap, and only allow harts with
> >> imsic to initialize imsic to avoid triggering illegal instruction
> >> exceptions.
> >>
> >> Signed-off-by: Cheng Yang <yangcheng.work at foxmail.com>
> >
> >We don't need such a complex solution using bitmap because we
> >can simply extend imsic_local_irqchip_init() as follows:
> >
> >diff --git a/lib/utils/irqchip/imsic.c b/lib/utils/irqchip/imsic.c
> >index f2a35c6..02e3a33 100644
> >--- a/lib/utils/irqchip/imsic.c
> >+++ b/lib/utils/irqchip/imsic.c
> >@@ -12,6 +12,7 @@
> >#include <sbi riscv_io.h="">
> >#include <sbi riscv_encoding.h="">
> >#include <sbi sbi_console.h="">
> >+#include <sbi sbi_csr_detect.h="">
> >#include <sbi sbi_domain.h="">
> >#include <sbi sbi_ipi.h="">
> >#include <sbi sbi_irqchip.h="">
> >@@ -222,6 +223,8 @@ static void imsic_local_eix_update(unsigned long base_id,
> >
> >void imsic_local_irqchip_init(void)
> >{
> >+ struct sbi_trap_info trap = { 0 };
> >+
> >/*
> >* This function is expected to be called from:
> >* 1) nascent_init() platform callback which is called
> >@@ -231,6 +234,11 @@ void imsic_local_irqchip_init(void)
> >* in boot-up path.
> >*/
> >
> >+ /* If Smaia not available then do nothing */
> >+ csr_read_allowed(CSR_MTOPI, (ulong)&trap);
> >+ if (trap.cause)
> >+ return;
> >+
> >/* Setup threshold to allow all enabled interrupts */
> >imsic_csr_write(IMSIC_EITHRESHOLD, IMSIC_ENABLE_EITHRESHOLD);
> >
> >Regards,
> >Anup
>
> Hi Anup,
>
> Thanks for the great suggestion, it's certainly much easier than my method.
> However, I was thinking that we could maybe use sbi_hart_has_extension() to
> detect if the current hart has the smaia extension instead of using csr_read_allowed(),
> which method do you think is better?
The imsic_local_irqchip_init() is called via generic_nascent_init()
very early in the boot sequence even before HART extensions
are detected.
Regards,
Anup
>
> Best regards,
> Cheng Yang
>
> >
> >> Changes V4 -> V5:
> >> - Fixed the issue where patch cannot be applied due to the editor automatically removing trailing spaces.
> >> Changes V3 -> V4:
> >> - Rebase to the latest master branch.
> >> Changes V2 -> V3:
> >> - Trailing whitespace.
> >> - Replace platform.hart_index2id to generic_hart_index2id.
> >> Changes V1 -> V2:
> >> - Add the processing of plat->hart_index2id be NULL in fdt_check_imsic_mlevel.
> >> ---
> >> include/sbi/sbi_platform.h | 8 ++++--
> >> include/sbi_utils/fdt/fdt_helper.h | 4 ++-
> >> lib/sbi/sbi_init.c | 2 +-
> >> lib/utils/fdt/fdt_helper.c | 45 ++++++++++++++++++++++++------
> >> platform/generic/platform.c | 26 ++++++++++++-----
> >> 5 files changed, 64 insertions(+), 21 deletions(-)
> >>
> >> diff --git a/include/sbi/sbi_platform.h b/include/sbi/sbi_platform.h
> >> index 581935a..f62d23d 100644
> >> --- a/include/sbi/sbi_platform.h
> >> +++ b/include/sbi/sbi_platform.h
> >> @@ -75,7 +75,7 @@ struct sbi_platform_operations {
> >> bool (*cold_boot_allowed)(u32 hartid);
> >>
> >> /* Platform nascent initialization */
> >> - int (*nascent_init)(void);
> >> + int (*nascent_init)(u32 hartid);
> >>
> >> /** Platform early initialization */
> >> int (*early_init)(bool cold_boot);
> >> @@ -395,10 +395,12 @@ static inline bool sbi_platform_cold_boot_allowed(
> >> *
> >> * @return 0 on success and negative error code on failure
> >> */
> >> -static inline int sbi_platform_nascent_init(const struct sbi_platform *plat)
> >> +static inline int sbi_platform_nascent_init(
> >> + const struct sbi_platform *plat,
> >> + u32 hartid)
> >> {
> >> if (plat && sbi_platform_ops(plat)->nascent_init)
> >> - return sbi_platform_ops(plat)->nascent_init();
> >> + return sbi_platform_ops(plat)->nascent_init(hartid);
> >> return 0;
> >> }
> >>
> >> diff --git a/include/sbi_utils/fdt/fdt_helper.h b/include/sbi_utils/fdt/fdt_helper.h
> >> index ab4a80f..1b7fdeb 100644
> >> --- a/include/sbi_utils/fdt/fdt_helper.h
> >> +++ b/include/sbi_utils/fdt/fdt_helper.h
> >> @@ -12,6 +12,7 @@
> >>
> >> #include <sbi sbi_types.h="">
> >> #include <sbi sbi_domain.h="">
> >> +#include <sbi sbi_platform.h="">
> >>
> >> struct fdt_match {
> >> const char *compatible;
> >> @@ -89,7 +90,8 @@ int fdt_parse_aplic_node(void *fdt, int nodeoff, struct aplic_data *aplic);
> >>
> >> struct imsic_data;
> >>
> >> -bool fdt_check_imsic_mlevel(void *fdt);
> >> +int fdt_check_imsic_mlevel(void *fdt, struct sbi_platform *plat,
> >> + unsigned long *imsic_bitmap);
> >>
> >> int fdt_parse_imsic_node(void *fdt, int nodeoff, struct imsic_data *imsic);
> >>
> >> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
> >> index 389172a..96f71a4 100644
> >> --- a/lib/sbi/sbi_init.c
> >> +++ b/lib/sbi/sbi_init.c
> >> @@ -545,7 +545,7 @@ void __noreturn sbi_init(struct sbi_scratch *scratch)
> >> * that platform can initialize platform specific per-HART CSRs
> >> * or per-HART devices.
> >> */
> >> - if (sbi_platform_nascent_init(plat))
> >> + if (sbi_platform_nascent_init(plat, hartid))
> >> sbi_hart_hang();
> >>
> >> if (coldboot)
> >> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> >> index a0e93b9..ae5d9cc 100644
> >> --- a/lib/utils/fdt/fdt_helper.c
> >> +++ b/lib/utils/fdt/fdt_helper.c
> >> @@ -764,27 +764,54 @@ skip_delegate_parse:
> >> return 0;
> >> }
> >>
> >> -bool fdt_check_imsic_mlevel(void *fdt)
> >> +int fdt_check_imsic_mlevel(void *fdt, struct sbi_platform *plat, unsigned long *imsic_bitmap)
> >> {
> >> const fdt32_t *val;
> >> - int i, len, noff = 0;
> >> + int i, h, len, err, cpu_offset, cpu_intc_offset, noff = 0;
> >> + u32 phandle, hwirq, hartid;
> >> +
> >> + bitmap_zero(imsic_bitmap, SBI_HARTMASK_MAX_BITS);
> >>
> >> if (!fdt)
> >> - return false;
> >> + return SBI_ENODEV;
> >>
> >> while ((noff = fdt_node_offset_by_compatible(fdt, noff,
> >> "riscv,imsics")) >= 0) {
> >> val = fdt_getprop(fdt, noff, "interrupts-extended", &len);
> >> - if (val && len > sizeof(fdt32_t)) {
> >> - len = len / sizeof(fdt32_t);
> >> - for (i = 0; i < len; i += 2) {
> >> - if (fdt32_to_cpu(val[i + 1]) == IRQ_M_EXT)
> >> - return true;
> >> + if (!val || len < sizeof(fdt32_t))
> >> + continue;
> >> +
> >> + len = len / sizeof(fdt32_t);
> >> + for (i = 0; i < len; i += 2) {
> >> + phandle = fdt32_to_cpu(val[i]);
> >> + hwirq = fdt32_to_cpu(val[i + 1]);
> >> +
> >> + if (hwirq != IRQ_M_EXT)
> >> + continue;
> >> +
> >> + cpu_intc_offset = fdt_node_offset_by_phandle(fdt, phandle);
> >> + if (cpu_intc_offset < 0)
> >> + continue;
> >> +
> >> + cpu_offset = fdt_parent_offset(fdt, cpu_intc_offset);
> >> + if (cpu_offset < 0)
> >> + continue;
> >> +
> >> + err = fdt_parse_hart_id(fdt, cpu_offset, &hartid);
> >> + if (err)
> >> + return SBI_EINVAL;
> >> +
> >> + for (int i = 0; i < plat->hart_count; i++) {
> >> + h = plat->hart_index2id ? plat->hart_index2id[i] : i;
> >> + if (hartid == h) {
> >> + bitmap_set(imsic_bitmap, i, 1);
> >> + break;
> >> + }
> >> }
> >> }
> >> }
> >>
> >> - return false;
> >> + return SBI_OK;
> >> }
> >>
> >> int fdt_parse_imsic_node(void *fdt, int nodeoff, struct imsic_data *imsic)
> >> diff --git a/platform/generic/platform.c b/platform/generic/platform.c
> >> index 1f46b76..00f8f86 100644
> >> --- a/platform/generic/platform.c
> >> +++ b/platform/generic/platform.c
> >> @@ -70,15 +70,15 @@ static u32 fw_platform_calculate_heap_size(u32 hart_count)
> >> }
> >>
> >> extern struct sbi_platform platform;
> >> -static bool platform_has_mlevel_imsic = false;
> >> static u32 generic_hart_index2id[SBI_HARTMASK_MAX_BITS] = { 0 };
> >>
> >> static DECLARE_BITMAP(generic_coldboot_harts, SBI_HARTMASK_MAX_BITS);
> >> +static DECLARE_BITMAP(generic_hart_has_mlevel_imsic, SBI_HARTMASK_MAX_BITS);
> >>
> >> /*
> >> - * The fw_platform_coldboot_harts_init() function is called by fw_platform_init()
> >> + * The fw_platform_coldboot_harts_init() function is called by fw_platform_init()
> >> * function to initialize the cold boot harts allowed by the generic platform
> >> - * according to the DT property "cold-boot-harts" in "/chosen/opensbi-config"
> >> + * according to the DT property "cold-boot-harts" in "/chosen/opensbi-config"
> >> * DT node. If there is no "cold-boot-harts" in DT, all harts will be allowed.
> >> */
> >> static void fw_platform_coldboot_harts_init(void *fdt)
> >> @@ -186,7 +186,11 @@ unsigned long fw_platform_init(unsigned long arg0, unsigned long arg1,
> >>
> >> platform.hart_count = hart_count;
> >> platform.heap_size = fw_platform_calculate_heap_size(hart_count);
> >> - platform_has_mlevel_imsic = fdt_check_imsic_mlevel(fdt);
> >> +
> >> + rc = fdt_check_imsic_mlevel(fdt, &platform,
> >> + generic_hart_has_mlevel_imsic);
> >> + if (rc)
> >> + goto fail;
> >>
> >> fw_platform_coldboot_harts_init(fdt);
> >>
> >> @@ -212,10 +216,18 @@ static bool generic_cold_boot_allowed(u32 hartid)
> >> return false;
> >> }
> >>
> >> -static int generic_nascent_init(void)
> >> +static int generic_nascent_init(u32 hartid)
> >> {
> >> - if (platform_has_mlevel_imsic)
> >> - imsic_local_irqchip_init();
> >> + for (int i = 0; i < platform.hart_count; i++) {
> >> + if (hartid != generic_hart_index2id[i])
> >> + continue;
> >> +
> >> + if (bitmap_test(generic_hart_has_mlevel_imsic, i)) {
> >> + imsic_local_irqchip_init();
> >> + break;
> >> + }
> >> + }
> >> +
> >> return 0;
> >> }
> >>
> >> --
> >> 2.34.1
> >>
> >>
> >> --
> >> opensbi mailing list
> >> opensbi at lists.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/opensbi</sbi></sbi></sbi></sbi></sbi></sbi></sbi></sbi></sbi></sbi></yangcheng.work at foxmail.com></yangcheng.work at foxmail.com>
More information about the opensbi
mailing list