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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 9080 bytes --]


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

[-- Attachment #2: Type: text/html, Size: 12222 bytes --]