On 21.05.2025 18:03, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic-priv.h
> + *
> + * Private part of aplic.h header.
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates.
> + */
> +
> +#ifndef ASM__RISCV_PRIV_APLIC_H
> +#define ASM__RISCV_PRIV_APLIC_H
Nit: This one conforms to neither prior nor current rules.
> --- 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?
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.
> +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)?
> + 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?
> +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.
> --- /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?
Jan
