Re: [PATCH v1 09/14] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.04.2025 17:57, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -9,19 +9,112 @@
>   * 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 <asm/aplic.h>
>  #include <asm/device.h>
> +#include <asm/imsic.h>
>  #include <asm/intc.h>
> +#include <asm/riscv_encoding.h>
> +
> +#define APLIC_DEFAULT_PRIORITY  1
> +
> +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)
> +{
> +    int i;
> +
> +    /* Disable all interrupts */
> +    for ( i = 0; i <= aplic_info.nr_irqs; i += 32 )
> +        aplic.regs->clrie[i] = -1U;
> +
> +    /* Set interrupt type and default priority for all interrupts */
> +    for ( i = 1; i <= aplic_info.nr_irqs; i++ )
> +    {
> +        aplic.regs->sourcecfg[i - 1] = 0;
> +        aplic.regs->target[i - 1] = APLIC_DEFAULT_PRIORITY;

A field named "target" is written with a priority value?

> +    }
> +
> +    /* Clear APLIC domaincfg */
> +    aplic.regs->domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;

The statement doesn't like like there was any "clearing" here.

> +}
> +
> +static int __init aplic_init(void)
> +{
> +    int rc;
> +    dt_phandle imsic_phandle;
> +    uint32_t irq_range[2];
> +    const __be32 *prop;
> +    uint64_t size, paddr;
> +    struct dt_device_node *imsic_node;
> +    const struct dt_device_node *node = aplic_info.node;
> +
> +    /* check for associated imsic node */

Nit: Comment style (also elsewhere).

> +    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);
> +
> +    /* check imsic mode */
> +    rc = dt_property_read_u32_array(imsic_node, "interrupts-extended",
> +                                    irq_range, ARRAY_SIZE(irq_range));
> +    if ( rc && (rc != -EOVERFLOW) )
> +        panic("%s: unable to find interrupt-extended in %s node\n",
> +               node->full_name, imsic_node->full_name);

Why exactly is EOVERFLOW tolerable here?

> +    if ( irq_range[1] == IRQ_M_EXT )
> +        /* machine mode imsic node, ignore this aplic node */
> +        return 0;
> +
> +    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.nr_irqs);
> +    if ( !rc )
> +        panic("%s: failed to get number of interrupt sources\n",
> +              node->full_name);
> +
> +    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.paddr_end = paddr + size;
> +    aplic.size = size;

Why do all three need recording? Isn't a (start,size) tuple sufficient
(and unambiguous)?

> +    aplic.regs = ioremap(paddr, size);
> +    if ( !aplic.regs )
> +        panic("%s: unable to map\n", node->full_name);
> +
> +    /* Setup initial state APLIC interrupts */
> +    aplic_init_hw_interrupts();
> +
> +    return 0;
> +}
> +
> +static const struct intc_hw_operations __ro_after_init aplic_ops = {

const or __ro_after_init?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/aplic.h
> @@ -0,0 +1,77 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic.h
> + *
> + * RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) 2023 Microchip.
> + */
> +
> +#ifndef ASM__RISCV__APLIC_H
> +#define ASM__RISCV__APLIC_H
> +
> +#include <xen/types.h>
> +
> +#include <asm/imsic.h>
> +
> +#define APLIC_DOMAINCFG_IE      BIT(8, UL)
> +#define APLIC_DOMAINCFG_DM      BIT(2, UL)
> +
> +struct aplic_regs {
> +    uint32_t domaincfg;
> +    uint32_t sourcecfg[1023];
> +    uint8_t _reserved1[0xBC0];
> +
> +    uint32_t mmsiaddrcfg;
> +    uint32_t mmsiaddrcfgh;
> +    uint32_t smsiaddrcfg;
> +    uint32_t smsiaddrcfgh;
> +    uint8_t _reserved2[0x30];
> +
> +    uint32_t setip[32];
> +    uint8_t _reserved3[92];
> +
> +    uint32_t setipnum;
> +    uint8_t _reserved4[0x20];
> +
> +    uint32_t in_clrip[32];
> +    uint8_t _reserved5[92];
> +
> +    uint32_t clripnum;
> +    uint8_t _reserved6[32];
> +
> +    uint32_t setie[32];
> +    uint8_t _reserved7[92];
> +
> +    uint32_t setienum;
> +    uint8_t _reserved8[32];
> +
> +    uint32_t clrie[32];
> +    uint8_t _reserved9[92];
> +
> +    uint32_t clrienum;
> +    uint8_t _reserved10[32];
> +
> +    uint32_t setipnum_le;
> +    uint32_t setipnum_be;
> +    uint8_t _reserved11[4088];
> +
> +    uint32_t genmsi;
> +    uint32_t target[1023];
> +};
> +
> +struct aplic_priv {
> +    /* base physical address and size */
> +    paddr_t paddr_start;
> +    paddr_t paddr_end;
> +    size_t  size;
> +
> +    /* registers */
> +    volatile struct aplic_regs *regs;
> +
> +    /* imsic configuration */
> +    const struct imsic_config *imsic_cfg;
> +};
> +
> +#endif /* ASM__RISCV__APLIC_H */

Does all of this really need to live in a non-private header?

> --- a/xen/arch/riscv/include/asm/irq.h
> +++ b/xen/arch/riscv/include/asm/irq.h
> @@ -27,7 +27,6 @@
>  #define IRQ_TYPE_INVALID        DT_IRQ_TYPE_INVALID
>  
>  /* TODO */
> -#define nr_irqs 0U

How come this is simply no longer needed, i.e. without any replacement?
Hmm, looks like the only use in common code has gone away. Yet then this
still doesn't really look to belong here (especially if not mentioned in
the description).

Jan