[PATCH v2] lib: utils/irqchip: always parse msi information for each aplic device
Inochi Amaoto
inochiama at gmail.com
Tue May 20 01:29:25 PDT 2025
On Tue, May 20, 2025 at 10:34:49AM +0530, Anup Patel wrote:
> On Tue, Apr 29, 2025 at 4:23 AM Inochi Amaoto <inochiama at gmail.com> wrote:
> >
> > OpenSBI only parses MSI information of the first next level subdomain
> > for now, which makes the root domain misconfigured in some case:
> > 1. the msi is not enabled on the first subdomain of the root domain,
> > but other subdomains enable MSI.
> > 2. the root domain is set as direct mode, but its subdomains enable MSI.
> >
> > So it is needed to parse all child of the root domain, Otherwise, the
> > some non-root domains are broken. As the specification says, it is
> > safe to parse the MSI information of all its subdomain and write the
> > msiaddrcfg register of the non root domain as they are read only.
> >
> > Parse the aplic MSI information recursively for all aplic device.
> >
> > Signed-off-by: Inochi Amaoto <inochiama at gmail.com>
> > ---
> > Changed from v1:
> > 1. update comment
> > 2. remove unnecessary header import
> > ---
> > lib/utils/fdt/fdt_helper.c | 136 ++++++++++++++++++-------------------
> > 1 file changed, 67 insertions(+), 69 deletions(-)
> >
> > diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> > index 8939d90..eeab326 100644
> > --- a/lib/utils/fdt/fdt_helper.c
> > +++ b/lib/utils/fdt/fdt_helper.c
> > @@ -602,13 +602,56 @@ int fdt_parse_xlnx_uartlite_node(const void *fdt, int nodeoffset,
> > return fdt_parse_uart_node_common(fdt, nodeoffset, uart, 0, 0);
> > }
> >
> > +static int fdt_aplic_find_imsic_node(const void *fdt, int nodeoff, struct imsic_data *imsic, bool mmode)
> > +{
> > + const fdt32_t *val;
> > + int i, len, noff, rc;
> > +
> > + val = fdt_getprop(fdt, nodeoff, "msi-parent", &len);
> > + if (val && len >= sizeof(fdt32_t)) {
> > + noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val));
> > + if (noff < 0)
> > + return noff;
> > +
> > + rc = fdt_parse_imsic_node(fdt, noff, imsic);
> > + if (rc)
> > + return rc;
> > +
> > + rc = imsic_data_check(imsic);
> > + if (rc)
> > + return rc;
> > +
> > + if (imsic->targets_mmode == mmode) {
> > + return 0;
> > + }
> > + }
>
> this should be:
>
> if (val && len >= sizeof(fdt32_t)) {
> ...
> } else {
> return SBI_ENODEV;
> }
>
> > +
> > + val = fdt_getprop(fdt, nodeoff, "riscv,children", &len);
> > + if (!val || len < sizeof(fdt32_t))
> > + return SBI_ENODEV;
> > +
> > + len /= sizeof(fdt32_t);
> > +
> > + for (i = 0; i < len; i++) {
> > + noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(val[i]));
> > + if (noff < 0)
> > + return noff;
> > +
> > + rc = fdt_aplic_find_imsic_node(fdt, noff, imsic, mmode);
> > + if (!rc)
> > + break;
> > + }
> > +
> > + return rc;
> > +}
> > +
> > int fdt_parse_aplic_node(const void *fdt, int nodeoff, struct aplic_data *aplic)
> > {
> > bool child_found;
> > const fdt32_t *val;
> > const fdt32_t *del;
> > struct imsic_data imsic = { 0 };
> > - int i, j, d, dcnt, len, noff, rc;
> > + int i, j, d, dcnt, len, rc;
> > uint64_t reg_addr, reg_size;
> > struct aplic_delegate_data *deleg;
> >
> > @@ -635,78 +678,33 @@ int fdt_parse_aplic_node(const void *fdt, int nodeoff, struct aplic_data *aplic)
> > }
> > }
> > aplic->num_idc = len / 2;
> > - goto aplic_msi_parent_done;
> > }
> >
> > - val = fdt_getprop(fdt, nodeoff, "msi-parent", &len);
> > - if (val && len >= sizeof(fdt32_t)) {
> > - noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val));
> > - if (noff < 0)
> > - return noff;
> > -
> > - rc = fdt_parse_imsic_node(fdt, noff, &imsic);
> > - if (rc)
> > - return rc;
> > -
> > - rc = imsic_data_check(&imsic);
> > - if (rc)
> > - return rc;
> > -
> > - aplic->targets_mmode = imsic.targets_mmode;
>
> I don't see "aplic->targets_mmode" being set below.
>
I forgot to add this, I think I should add a "aplic->targets_mmode"
in the m-mode fdt_aplic_find_imsic_node check.
Regards,
Inochi
> > -
> > - if (imsic.targets_mmode) {
> > - aplic->has_msicfg_mmode = true;
> > - aplic->msicfg_mmode.lhxs = imsic.guest_index_bits;
> > - aplic->msicfg_mmode.lhxw = imsic.hart_index_bits;
> > - aplic->msicfg_mmode.hhxw = imsic.group_index_bits;
> > - aplic->msicfg_mmode.hhxs = imsic.group_index_shift;
> > - if (aplic->msicfg_mmode.hhxs <
> > - (2 * IMSIC_MMIO_PAGE_SHIFT))
> > - return SBI_EINVAL;
> > - aplic->msicfg_mmode.hhxs -= 24;
> > - aplic->msicfg_mmode.base_addr = imsic.regs[0].addr;
> > - } else {
> > - goto aplic_msi_parent_done;
> > - }
> > -
> > - val = fdt_getprop(fdt, nodeoff, "riscv,children", &len);
> > - if (!val || len < sizeof(fdt32_t))
> > - goto aplic_msi_parent_done;
> > -
> > - noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val));
> > - if (noff < 0)
> > - return noff;
> > -
> > - val = fdt_getprop(fdt, noff, "msi-parent", &len);
> > - if (!val || len < sizeof(fdt32_t))
> > - goto aplic_msi_parent_done;
> > -
> > - noff = fdt_node_offset_by_phandle(fdt, fdt32_to_cpu(*val));
> > - if (noff < 0)
> > - return noff;
> > -
> > - rc = fdt_parse_imsic_node(fdt, noff, &imsic);
> > - if (rc)
> > - return rc;
> > -
> > - rc = imsic_data_check(&imsic);
> > - if (rc)
> > - return rc;
> > + rc = fdt_aplic_find_imsic_node(fdt, nodeoff, &imsic, true);
> > + if (!rc) {
> > + aplic->has_msicfg_mmode = true;
> > + aplic->msicfg_mmode.lhxs = imsic.guest_index_bits;
> > + aplic->msicfg_mmode.lhxw = imsic.hart_index_bits;
> > + aplic->msicfg_mmode.hhxw = imsic.group_index_bits;
> > + aplic->msicfg_mmode.hhxs = imsic.group_index_shift;
> > + if (aplic->msicfg_mmode.hhxs < (2 * IMSIC_MMIO_PAGE_SHIFT))
> > + return SBI_EINVAL;
> > + aplic->msicfg_mmode.hhxs -= 24;
> > + aplic->msicfg_mmode.base_addr = imsic.regs[0].addr;
> > + }
> >
> > - if (!imsic.targets_mmode) {
> > - aplic->has_msicfg_smode = true;
> > - aplic->msicfg_smode.lhxs = imsic.guest_index_bits;
> > - aplic->msicfg_smode.lhxw = imsic.hart_index_bits;
> > - aplic->msicfg_smode.hhxw = imsic.group_index_bits;
> > - aplic->msicfg_smode.hhxs = imsic.group_index_shift;
> > - if (aplic->msicfg_smode.hhxs <
> > - (2 * IMSIC_MMIO_PAGE_SHIFT))
> > - return SBI_EINVAL;
> > - aplic->msicfg_smode.hhxs -= 24;
> > - aplic->msicfg_smode.base_addr = imsic.regs[0].addr;
> > - }
> > + rc = fdt_aplic_find_imsic_node(fdt, nodeoff, &imsic, false);
> > + if (!rc) {
> > + aplic->has_msicfg_smode = true;
> > + aplic->msicfg_smode.lhxs = imsic.guest_index_bits;
> > + aplic->msicfg_smode.lhxw = imsic.hart_index_bits;
> > + aplic->msicfg_smode.hhxw = imsic.group_index_bits;
> > + aplic->msicfg_smode.hhxs = imsic.group_index_shift;
> > + if (aplic->msicfg_smode.hhxs < (2 * IMSIC_MMIO_PAGE_SHIFT))
> > + return SBI_EINVAL;
> > + aplic->msicfg_smode.hhxs -= 24;
> > + aplic->msicfg_smode.base_addr = imsic.regs[0].addr;
> > }
> > -aplic_msi_parent_done:
> >
> > for (d = 0; d < APLIC_MAX_DELEGATE; d++) {
> > deleg = &aplic->delegate[d];
> > --
> > 2.49.0
> >
> >
> > --
> > opensbi mailing list
> > opensbi at lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/opensbi
>
> Regards,
> Anup
More information about the opensbi
mailing list