On 5/22/25 5:26 PM, Jan Beulich wrote:
On 21.05.2025 18:03, Oleksii Kurochko wrote:
--- a/xen/arch/riscv/aplic.c
+++ b/xen/arch/riscv/aplic.c
@@ -9,19 +9,113 @@
* 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"
#include <asm/device.h>
+#include <asm/imsic.h>
#include <asm/intc.h>
+#include <asm/riscv_encoding.h>
+
+#define APLIC_DEFAULT_PRIORITY 1
+
+/* The maximum number of wired interrupt sources supported by APLIC domain. */
+#define APLIC_MAX_NUM_WIRED_IRQ_SOURCES 1023
Wait - what's "wired" here? There's only MSI you said elsewhere?
This what was mentioned in DT binding:
riscv,num-sources:
$ref: /schemas/types.yaml#/definitions/uint32
minimum: 1
maximum: 1023
description:
Specifies the number of wired interrupt sources supported by this
APLIC domain.
But 'wired' isn't really mention in AIA spec:
For each possible interrupt source , register sourcecfg[ ] controls the source
mode for source in this interrupt domain as well as any delegation of the
source
to a child domain.
So IMO it isn't connected to wired or not interrupts. So ...
Further - how's this 1023 related to any of the other uses of that number?
Is this by chance ARRAY_SIZE(aplic.regs->sourcecfg)? If so, it wants
expressing like that, to allow making the connection.
...ARRAY_SIZE(aplic.regs->sourcecfg) perfectly match and will be used
instead of APLIC_MAX_NUM_WIRED_IRQ_SOURCES.
+static struct aplic_priv aplic;
static struct intc_info __ro_after_init aplic_info = {
.hw_version = INTC_APLIC,
};
+static void __init aplic_init_hw_interrupts(void)
+{
+ unsigned int i;
+
+ /* Disable all interrupts */
+ for ( i = 0; i < ARRAY_SIZE(aplic.regs->clrie); i++)
+ writel(~0U, &aplic.regs->clrie[i]);
+
+ /* Set interrupt type and default priority for all interrupts */
+ for ( i = 0; i < aplic_info.num_irqs; i++ )
+ {
+ writel(0, &aplic.regs->sourcecfg[i]);
+ /*
+ * 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]);
+ }
+
+ writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
+}
+
+static int __init cf_check aplic_init(void)
+{
+ dt_phandle imsic_phandle;
+ const __be32 *prop;
+ uint64_t size, paddr;
+ const struct dt_device_node *imsic_node;
+ const struct dt_device_node *node = aplic_info.node;
+ int rc;
+
+ /* Check for associated imsic node */
+ if ( !dt_property_read_u32(node, "msi-parent", &imsic_phandle) )
+ 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 = imsic_init(imsic_node);
+ if ( rc == IRQ_M_EXT )
+ /* Machine mode imsic node, ignore this aplic node */
+ return 0;
+ else if ( rc )
As before: No "else" please when the earlier if() ends in an unconditional
control flow change.
+ panic("%s: Failded to initialize IMSIC\n", node->full_name);
+
+ /* Find out number of interrupt sources */
+ if ( !dt_property_read_u32(node, "riscv,num-sources",
+ &aplic_info.num_irqs) )
+ panic("%s: failed to get number of interrupt sources\n",
+ node->full_name);
+
+ if ( aplic_info.num_irqs > APLIC_MAX_NUM_WIRED_IRQ_SOURCES )
+ panic("%s: too big number of riscv,num-source: %u\n",
+ __func__, aplic_info.num_irqs);
Is it actually necessary to panic() in this case? Can't you just lower
.num_irqs instead (rendering higher IRQs,if any, non-functional)?
Good point. I think we can just use aplic_info.num_irqs
=ARRAY_SIZE(aplic.regs->sourcecfg);
+ prop = dt_get_property(node, "reg", NULL);
+ dt_get_range(&prop, node, &paddr, &size);
+ if ( !paddr )
+ panic("%s: first MMIO resource not found\n", node->full_name);
+
+ aplic.paddr_start = paddr;
+ aplic.size = size;
+
+ aplic.regs = ioremap(paddr, size);
Doesn't size need to match certain constraints? If too low, you may
need to panic(), while if too high you may not need to map the entire
range?
Does paddr perhaps also need to match certain contraints, like having
the low so many bits clear?
Good question. According to AIA spec:
4.5. Memory-mapped control region for an interrupt domain
For each interrupt domain that an APLIC supports, there is a dedicated
memory-mapped control
region for managing interrupts in that domain. This control region is a
multiple of 4 KiB in size and
aligned to a 4-KiB address boundary. The smallest valid control region is 16
KiB. An interrupt domain’s
control region is populated by a set of 32-bit registers. The first 16 KiB
contains the registers listed in
Table 6
The best what I can do is:
1. Check that the size is a multiple of 4KiB is size and is not less then
16Kib. But nothing I can do with
the high boundary.
2. Regarding paddr the best I can do it is to check that it is a 4-KiB aligned.
I added the following:
if ( !IS_ALIGNED(paddr, KB(4)) )
panic("%s: paddr of memory-mapped control region should be 4Kb "
"aligned:%#lx\n", __func__, paddr);
if ( !IS_ALIGNED(size, KB(4)) && (size < KB(16)) )
panic("%s: memory-mapped control region should be a multiple of 4 KiB "
"in size and the smallest valid control is 16Kb: %#lx\n",
__func__, size);
Would it be enough?
+static struct intc_hw_operations __ro_after_init aplic_ops = {
+ .info = &aplic_info,
+ .init = aplic_init,
+};
Why's this __ro_after_init and not simply const? I can't spot any write
to it.
It could be const. I added __ro_after_init when I misinterpreted a correct
usage of it.
I will return back const instead of __ro_after_init.
--- /dev/null
+++ b/xen/arch/riscv/include/asm/aplic.h
@@ -0,0 +1,64 @@
+/* SPDX-License-Identifier: MIT */
+
+/*
+ * xen/arch/riscv/asm/include/aplic.h
+ *
+ * RISC-V Advanced Platform-Level Interrupt Controller support
+ *
+ * Copyright (c) Microchip.
+ */
+
+#ifndef ASM__RISCV__APLIC_H
+#define ASM__RISCV__APLIC_H
Wants updating again.
+#include <xen/types.h>
+
+#include <asm/imsic.h>
+
+#define APLIC_DOMAINCFG_IE BIT(8, UL)
+#define APLIC_DOMAINCFG_DM BIT(2, UL)
Why UL when ...
+struct aplic_regs {
+ uint32_t domaincfg;
... this is just 32 bits wide?
Agree, BIT(x,U) would be more correct.
Thanks.
~ Oleksii
