Re: [PATCH v5 4/9] xen/riscv: aplic_init() implementation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.06.2025 17:48, 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
> +
> +#include <xen/types.h>
> +
> +#include <asm/aplic.h>
> +#include <asm/imsic.h>
> +
> +struct aplic_priv {
> +    /* base physical address and size */

I'm sure I did ask for this before, and such a request really is meant to apply
globally: Please can you abide by the comment style set forth in ./CODING_STYLE.

> +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;
> +
> +    if ( rc )
> +        panic("%s: Failed 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 > ARRAY_SIZE(aplic.regs->sourcecfg) )
> +        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);
> +
> +    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",

The line having grown so long should have served as an indication to abbreviate 
the
text somewhat.

Also note the consmetic difference between this and the earlier message, as to a
blank (or not) after the latter colon. Please try to be consistent at least 
within
a patch / function / whatever other unit.

> --- /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, U)
> +#define APLIC_DOMAINCFG_DM      BIT(2, U)
> +
> +struct aplic_regs {
> +    uint32_t domaincfg;
> +    uint32_t sourcecfg[1023];
> +    uint8_t _reserved1[3008];
> +
> +    uint32_t mmsiaddrcfg;
> +    uint32_t mmsiaddrcfgh;
> +    uint32_t smsiaddrcfg;
> +    uint32_t smsiaddrcfgh;
> +    uint8_t _reserved2[48];
> +
> +    uint32_t setip[32];
> +    uint8_t _reserved3[92];
> +
> +    uint32_t setipnum;
> +    uint8_t _reserved4[32];
> +
> +    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];
> +};

Each time I see this I wonder whether it wouldn't be helpful if, at least for
the non-reserved fields, there would be comments clarifying their hex offset.
That way it would be easier to (a) compare with the spec and (b) cross-check
the array dimensions used.

Jan