diff mbox series

lib: utils/irqchip: always parse msi information for each aplic device

Message ID 20250423080915.1101707-1-inochiama@gmail.com
State New
Headers show
Series lib: utils/irqchip: always parse msi information for each aplic device | expand

Commit Message

Inochi Amaoto April 23, 2025, 8:09 a.m. UTC 
For a root domain, it may keep direct delivery mode as its subdomains
use msi to delivery interrupt. The currect parsing logic may lost this
information and break the system if the domain is not set as MSI mode
or the MSI domain is not a direct children of the root domain.

For a non-root domain, it is safe to parse the MSI information of all
its domain and write the msiaddrcfg register. The bus should ignore the
write on this readonly address.

Parse the aplic MSI information recursively for all aplic device.

Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
---
 lib/utils/fdt/fdt_helper.c | 138 ++++++++++++++++++-------------------
 1 file changed, 69 insertions(+), 69 deletions(-)

Comments

Yao Zi April 23, 2025, 11:31 a.m. UTC | #1 
On Wed, Apr 23, 2025 at 04:09:14PM +0800, Inochi Amaoto wrote:
> For a root domain, it may keep direct delivery mode as its subdomains
> use msi to delivery interrupt. The currect parsing logic may lost this

Grammar: may lost -> may lose

> information and break the system if the domain is not set as MSI mode
> or the MSI domain is not a direct children of the root domain.
> 
> For a non-root domain, it is safe to parse the MSI information of all
> its domain and write the msiaddrcfg register. The bus should ignore the

Should "all its domain" be "all its subdomains"? This sentence is a
little confusing.

> write on this readonly address.
> 
> Parse the aplic MSI information recursively for all aplic device.
> 
> Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> ---
>  lib/utils/fdt/fdt_helper.c | 138 ++++++++++++++++++-------------------
>  1 file changed, 69 insertions(+), 69 deletions(-)
> 
> diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> index 8939d90..0150725 100644
> --- a/lib/utils/fdt/fdt_helper.c
> +++ b/lib/utils/fdt/fdt_helper.c
> @@ -6,6 +6,8 @@
>   * Copyright (C) 2020 Bin Meng <bmeng.cn@gmail.com>
>   */
>  
> +#include "sbi/sbi_error.h"
> +#include "sbi/sbi_types.h"

I'd like to keep the including style consistent and sorted.

>  #include <libfdt.h>
>  #include <sbi/riscv_asm.h>
>  #include <sbi/sbi_console.h>

...

Cheers,
Yao Zi
Inochi Amaoto April 23, 2025, 11:40 a.m. UTC | #2 
On Wed, Apr 23, 2025 at 11:31:22AM +0000, Yao Zi wrote:
> On Wed, Apr 23, 2025 at 04:09:14PM +0800, Inochi Amaoto wrote:
> > For a root domain, it may keep direct delivery mode as its subdomains
> > use msi to delivery interrupt. The currect parsing logic may lost this
> 
> Grammar: may lost -> may lose
> 
> > information and break the system if the domain is not set as MSI mode
> > or the MSI domain is not a direct children of the root domain.
> > 
> > For a non-root domain, it is safe to parse the MSI information of all
> > its domain and write the msiaddrcfg register. The bus should ignore the
> 
> Should "all its domain" be "all its subdomains"? This sentence is a
> little confusing.
> 

Yeah, it should be subdomains.

> > write on this readonly address.
> > 
> > Parse the aplic MSI information recursively for all aplic device.
> > 
> > Signed-off-by: Inochi Amaoto <inochiama@gmail.com>
> > ---
> >  lib/utils/fdt/fdt_helper.c | 138 ++++++++++++++++++-------------------
> >  1 file changed, 69 insertions(+), 69 deletions(-)
> > 
> > diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
> > index 8939d90..0150725 100644
> > --- a/lib/utils/fdt/fdt_helper.c
> > +++ b/lib/utils/fdt/fdt_helper.c
> > @@ -6,6 +6,8 @@
> >   * Copyright (C) 2020 Bin Meng <bmeng.cn@gmail.com>
> >   */
> >  
> > +#include "sbi/sbi_error.h"
> > +#include "sbi/sbi_types.h"
> 
> I'd like to keep the including style consistent and sorted.
> 

Oh, I miss this. I think this is inserted by clangd plugin.
And they should be removed.

Regards,
Inochi
diff mbox series

Patch

diff --git a/lib/utils/fdt/fdt_helper.c b/lib/utils/fdt/fdt_helper.c
index 8939d90..0150725 100644
--- a/lib/utils/fdt/fdt_helper.c
+++ b/lib/utils/fdt/fdt_helper.c
@@ -6,6 +6,8 @@ 
  * Copyright (C) 2020 Bin Meng <bmeng.cn@gmail.com>
  */
 
+#include "sbi/sbi_error.h"
+#include "sbi/sbi_types.h"
 #include <libfdt.h>
 #include <sbi/riscv_asm.h>
 #include <sbi/sbi_console.h>
@@ -602,13 +604,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;
+		}
+	}
+
+	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 +680,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;
-
-		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];