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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/14/25 12:04 PM, Jan Beulich wrote:

> 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?
>     
Low bits of target register contains Interrupt Priority bits which can't be zero according to
AIA spec:
```
4.5.16.1. Active source, direct delivery mode
For an active interrupt source , if the domain is configured in direct delivery mode (domaincfg.DM = 0),
then register target[ ] has this format:
  bits 31:18 Hart Index (WLRL)
  bits 7:0 IPRIO (WARL)

All other register bits are reserved and read as zeros.
Hart Index is a WLRL field that specifies the hart to which interrupts from this source will be delivered.
Field IPRIO (Interrupt Priority) specifies the priority number for the interrupt source. This field is
a WARL unsigned integer of IPRIOLEN bits, where IPRIOLEN is a constant parameter for the given APLIC, in
the range of 1 to 8. Only values 1 through are allowed for IPRIO, not zero. A write to a target register
sets IPRIO equal to bits :0 of the 32-bit value written, unless those bits are all zeros, in which case
the priority number is set to 1 instead. (If IPRIOLEN = 1, these rules cause IPRIO to be effectively
read-only with value 1.)
```

> > +    }
> > +
> > +    /* Clear APLIC domaincfg */
> > +    aplic.regs->domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;
> >       
> The statement doesn't like like there was any "clearing" here.
>     
But all other bits, except APLIC_DOMAINCFG_{IE, DM}, are set to zero.
I think we can remove this comment to avoid confusion.

> > +    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?
>     
QEMU generates two IMSIC device tree nodes: one for M-mode and one for S-mode.
For the hypervisor, we don’t really care about the M-mode IMSIC node — we're only
interested in the S-mode IMSIC node.
The IMSIC node includes this information in the "interrupts-extended" property,
which has the following format:
  interrupt-extended = {<interrupt-controller-phandle>, <machine_mode>},...
The number of such <phandle, mode> pairs depends on the number of CPUs the platform has.
For our purposes, to determine whether the IMSIC node corresponds to M-mode or not, it’s sufficient to read only the first pair and check the mode like this:
  if ( irq_range[1] == IRQ_M_EXT )

Thereby dt_property_read_u32_array() will return -EOVERFLOW in the case when a platfrom
has more then one CPU as we passed irq_range[2] as an argument but the amount of values
in "interrupts-extended" property will be (2 * CPUS_NUM).

I can update the comment above dt_property_read_u32_array() for more clearness.

> > +    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)?
>     
(start,size) will be enough. I'll drop aplic.paddr_end.

> > +    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?
>     
What’s wrong with using both? const ensures the variable can't be changed at compile time,
while __ro_after_init makes it read-only at runtime after initialization is complete.
Probably, __initconst would be a better fit:
  static const struct intc_hw_operations __initconst aplic_ops = {

Or even __initconstrel, since the struct intc_hw_operations contains pointers.
> > --- /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?
>     
struct aplic_priv is used in different files:
- in aplic.c to define `aplic` variable.
- in vaplic.c (which isn't intoduced yet) is used in several places: https://gitlab.com/xen-project/people/olkur/xen/-/blob/latest/xen/arch/riscv/vaplic.c#L41

struct aplic_regs is used only in aplic.c (at least, at the moment) so could be moved to
aplic.c, but I don't see too much sense.

> > --- 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).
>     
I missed that it is used in xen/common/domain.c when CONFIG_HAS_PIRQ=y, but this
config isn't selected for RISC-V.
I think that I have to revert this change.

~ Oleksii