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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 5/22/25 5:26 PM, Jan Beulich wrote:

> On 21.05.2025 18:03, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -9,19 +9,113 @@
>   * 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"
>  
>  #include <asm/device.h>
> +#include <asm/imsic.h>
>  #include <asm/intc.h>
> +#include <asm/riscv_encoding.h>
> +
> +#define APLIC_DEFAULT_PRIORITY  1
> +
> +/* The maximum number of wired interrupt sources supported by APLIC domain. */
> +#define APLIC_MAX_NUM_WIRED_IRQ_SOURCES 1023
> Wait - what's "wired" here? There's only MSI you said elsewhere?
>     
This what was mentioned in DT binding:
  riscv,num-sources:
    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 1
    maximum: 1023
    description:
      Specifies the number of wired interrupt sources supported by this
      APLIC domain.

But 'wired' isn't really mention in AIA spec:
  For each possible interrupt source , register sourcecfg[ ] controls the source
  mode for source in this interrupt domain as well as any delegation of the source
  to a child domain.

So IMO it isn't connected to wired or not interrupts. So ...
> Further - how's this 1023 related to any of the other uses of that number?
> Is this by chance ARRAY_SIZE(aplic.regs->sourcecfg)? If so, it wants
> expressing like that, to allow making the connection.
>     
... ARRAY_SIZE(aplic.regs->sourcecfg) perfectly match and will be used instead
of APLIC_MAX_NUM_WIRED_IRQ_SOURCES.

> > +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(~0U, &aplic.regs->clrie[i]);
> > +
> > +    /* Set interrupt type and default priority for all interrupts */
> > +    for ( i = 0; i < aplic_info.num_irqs; i++ )
> > +    {
> > +        writel(0, &aplic.regs->sourcecfg[i]);
> > +        /*
> > +         * 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]);
> > +    }
> > +
> > +    writel(APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM, &aplic.regs->domaincfg);
> > +}
> > +
> > +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;
> > +    else if ( rc )
> >       
> As before: No "else" please when the earlier if() ends in an unconditional
> control flow change.
>     
> +        panic("%s: Failded 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 > APLIC_MAX_NUM_WIRED_IRQ_SOURCES )
> +        panic("%s: too big number of riscv,num-source: %u\n",
> +               __func__, aplic_info.num_irqs);
>     
> >  
> Is it actually necessary to panic() in this case? Can't you just lower
> .num_irqs instead (rendering higher IRQs,if any, non-functional)?
>     
Good point. I think we can just use 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);
> > +
> > +    aplic.paddr_start = paddr;
> > +    aplic.size = size;
> > +
> > +    aplic.regs = ioremap(paddr, size);
> >       
> Doesn't size need to match certain constraints? If too low, you may
> need to panic(), while if too high you may not need to map the entire
> range?
>
> Does paddr perhaps also need to match certain contraints, like having
> the low so many bits clear?
>     
Good question. According to AIA spec:
  4.5. Memory-mapped control region for an interrupt domain
  For each interrupt domain that an APLIC supports, there is a dedicated memory-mapped control
  region for managing interrupts in that domain. This control region is a multiple of 4 KiB in size and
  aligned to a 4-KiB address boundary. The smallest valid control region is 16 KiB. An interrupt domain’s
  control region is populated by a set of 32-bit registers. The first 16 KiB contains the registers listed in
  Table 6
The best what I can do is:
1. Check that the size is a multiple of 4KiB is size and is not less then 16Kib. But nothing I can do with
   the high boundary.
2. Regarding paddr the best I can do it is to check that it is a 4-KiB aligned.

I added the following:
    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",
              __func__, size);

Would it be enough?

> > +static struct intc_hw_operations __ro_after_init aplic_ops = {
> > +    .info                = &aplic_info,
> > +    .init                = aplic_init,
> > +};
> >       
> Why's this __ro_after_init and not simply const? I can't spot any write
> to it.
>     
It could be const. I added __ro_after_init when I misinterpreted a correct usage of it.
I will return back const instead of __ro_after_init.

> > --- /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
> >       
> Wants updating again.
>
> > +#include <xen/types.h>
> > +
> > +#include <asm/imsic.h>
> > +
> > +#define APLIC_DOMAINCFG_IE      BIT(8, UL)
> > +#define APLIC_DOMAINCFG_DM      BIT(2, UL)
> >       
> Why UL when ...
>
> > +struct aplic_regs {
> > +    uint32_t domaincfg;
> >       
> ... this is just 32 bits wide?
>     
Agree, BIT(x,U) would be more correct.

Thanks.

~ Oleksii