Re: [PATCH v2 11/16] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.05.2025 18:51, Oleksii Kurochko wrote:
> --- /dev/null
> +++ b/xen/arch/riscv/aplic-priv.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +
> +/*
> + * xen/arch/riscv/aplic.h

If already you have such in a comment, please have it be correct.

> + * 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
> +
> +#include <xen/types.h>
> +
> +#include <asm/aplic.h>
> +#include <asm/imsic.h>
> +
> +struct aplic_priv {
> +    /* base physical address and size */
> +    paddr_t paddr_start;
> +    size_t  size;
> +
> +    /* registers */
> +    volatile struct aplic_regs *regs;

This looks to also want __iomem.

> --- 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?

>  #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)
> +{
> +    unsigned int i;
> +
> +    /* Disable all interrupts */
> +    for ( i = 0; i < ARRAY_SIZE(aplic.regs->clrie); i++)
> +        writel(-1U, &aplic.regs->clrie[i]);

Imo it's better to use ~0U.

> +    /* 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?

> +        /*
> +         * 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)?

> +    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?

> +    const __be32 *prop;
> +    uint64_t size, paddr;
> +    struct dt_device_node *imsic_node;

Pointer-to-const?

> +    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.)

I'm also puzzled by there not being any further use of the values later
in the function.

> +    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.

> --- /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
> +
> +#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];

I think you want to be consistent with the dimensions of at least all the
_reserved*[] fields - use decimal or use hex everywhere. Even better would
be if that was consistent across all array dimensions.

Jan