On 5/15/25 11:06 AM, Jan Beulich wrote:
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -9,19 +9,121 @@
* Copyright (c) 2024-2025 Vates
*/
+#include <xen/device_tree.h>
#include <xen/errno.h>
#include <xen/init.h>
#include <xen/irq.h>
+#include <xen/mm.h>
#include <xen/sections.h>
#include <xen/types.h>
+#include <xen/vmap.h>
+
+#include "aplic-priv.h"
Besides this, are there going to be any other files including this private
header? If not, why have the header in the first place?
Yes, structure aplic_priv is going to be reused in vaplic.c (which isn't
introduce yet):
https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/vaplic.c#L56
+ /* Set interrupt type and default priority for all interrupts */
+ for ( i = 1; i <= aplic_info.num_irqs; i++ )
+ {
+ writel(0, &aplic.regs->sourcecfg[i - 1]);
What guarantees the loop to not run past the array's size?
riscv,aplic binding:
https://github.com/torvalds/linux/blob/a5806cd506af5a7c19bcd596e4708b5c464bfd21/Documentation/devicetree/bindings/interrupt-controller/riscv%2Caplic.yaml#L57
Should I add an ASSERT() or panic() at the moment where num_irqs are
initialized to double check a binding?
+ /*
+ * Low bits of target register contains Interrupt Priority bits which
+ * can't be zero according to AIA spec.
+ * Thereby they are initialized to APLIC_DEFAULT_PRIORITY.
+ */
+ writel(APLIC_DEFAULT_PRIORITY, &aplic.regs->target[i - 1]);
+ }
Seeing the subtractions of 1 here, why's the loop header not simply
for ( i = 0; i < aplic_info.num_irqs; i++ )
(i.e. the more conventional form)?
To follow the definition of aplic's binding mentioned about where minimum is 1.
But I think we can use convention form.
+ writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
+}
+
+static int __init cf_check aplic_init(void)
+{
+ int rc;
+ dt_phandle imsic_phandle;
+ uint32_t irq_range[num_possible_cpus() * 2];
Are you going to have enough stack space when num_possible_cpus() is pretty
large?
When num_possible_cpus() will be pretty large then it will better to allocate
irq_range[]
dynamically.
Does it make sense to re-write now?
+ const struct dt_device_node *node = aplic_info.node;
+
+ /* Check for associated imsic node */
+ rc = dt_property_read_u32(node, "msi-parent", &imsic_phandle);
+ if ( !rc )
+ panic("%s: IDC mode not supported\n", node->full_name);
+
+ imsic_node = dt_find_node_by_phandle(imsic_phandle);
+ if ( !imsic_node )
+ panic("%s: unable to find IMSIC node\n", node->full_name);
+
+ rc = dt_property_read_u32_array(imsic_node, "interrupts-extended",
+ irq_range, ARRAY_SIZE(irq_range));
+ if ( rc )
+ panic("%s: unable to find interrupt-extended in %s node\n",
+ __func__, imsic_node->full_name);
+
+ if ( irq_range[1] == IRQ_M_EXT )
How do you know the array has had 2 or ...
+ /* Machine mode imsic node, ignore this aplic node */
+ return 0;
+
+ for ( unsigned int i = 0; i < ARRAY_SIZE(irq_range); i += 2 )
+ {
+ if ( irq_range[i + 1] != irq_range[1] )
+ panic("%s: mode[%d] != %d\n", __func__, i + 1, irq_range[1]);
+ }
... or even all of the slots populated? Anything not populated by the DT
function will still have the stack contents that had been left by earlier
call chains. (The loop also makes little sense to start from 0.)
Do you mean I asked allocated irq_range[8*2] but DT node has only
irq_range[4*2]?
Then it will be really an issue. And out-of-range access will occur in
dt_property_read_variable_u32_array(). I need another way to check then...
I'm also puzzled by there not being any further use of the values later
in the function.
Yes, because we need this values only to check that DT node's property is
correctly created.
+ rc = imsic_init(imsic_node);
+ if ( rc )
+ panic("%s: Failded to initialize IMSIC\n", node->full_name);
+
+ /* Find out number of interrupt sources */
+ rc = dt_property_read_u32(node, "riscv,num-sources", &aplic_info.num_irqs);
+ if ( !rc )
Assigning a bool return value to an int local var, which generally hold
error codes, is confusing. I don't think you really need to use a local
variable here.
Considering that I am panicing for all the case where rc is used and rc isn't
printed
then we can really just drop rc.
~ Oleksii
