Re: [PATCH v2 14/26] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 08.05.2026 16:43, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/aplic.c
> +++ b/xen/arch/riscv/aplic.c
> @@ -295,6 +295,11 @@ static void cf_check aplic_set_irq_type(struct irq_desc 
> *desc,
>      spin_unlock(&aplic.lock);
>  }
>  
> +static unsigned int cf_check aplic_irq_num(void)
> +{
> +    return aplic_info.num_irqs;
> +}
> +
>  static const hw_irq_controller aplic_xen_irq_type = {
>      .typename     = "aplic",
>      .startup      = aplic_irq_startup,
> @@ -309,6 +314,7 @@ static const struct intc_hw_operations aplic_ops = {
>      .host_irq_type       = &aplic_xen_irq_type,
>      .handle_interrupt    = aplic_handle_interrupt,
>      .set_irq_type        = aplic_set_irq_type,
> +    .irq_nums            = aplic_irq_num,

Hook handler names and respective field names would preferably match up. It's
unclear why the field uses some kind of plural(?), while the function uses
singular.

> --- a/xen/arch/riscv/domain.c
> +++ b/xen/arch/riscv/domain.c
> @@ -11,6 +11,7 @@
>  #include <asm/bitops.h>
>  #include <asm/cpufeature.h>
>  #include <asm/csr.h>
> +#include <asm/intc.h>
>  #include <asm/riscv_encoding.h>
>  #include <asm/vtimer.h>
>  
> @@ -155,14 +156,10 @@ int arch_vcpu_create(struct vcpu *v)
>      if ( (rc = vcpu_vtimer_init(v)) )
>          goto fail;
>  
> -    /*
> -     * As interrupt controller (IC) is not yet implemented,
> -     * return an error.
> -     *
> -     * TODO: Drop this once IC is implemented.
> -     */
> -    rc = -EOPNOTSUPP;
> -    goto fail;
> +    ASSERT(v->domain->arch.vintc->ops->vcpu_init);
> +
> +    if ( (rc = v->domain->arch.vintc->ops->vcpu_init(v)) )

I don't understand this model of the use of ASSERT(). As previously said
(more than once) - you'll crash anyway if any of the involved pointers is
NULL. If you really think an up-front check is better, then why would you
check only the leaf of the pointer chain, and not also vintc and ops?
(Once you do you'll then likely notice that there are more assertions
than actual code.)

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/vaplic.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * xen/arch/riscv/vaplic.c
> + *
> + * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + */
> +
> +#ifndef ASM__RISCV__VAPLIC_H
> +#define ASM__RISCV__VAPLIC_H
> +
> +#include <xen/kernel.h>
> +#include <xen/types.h>
> +
> +#include <asm/intc.h>
> +
> +struct domain;
> +
> +#define to_vaplic(d) container_of(d->arch.vintc, struct vaplic, vintc)

d wants parenthesizing here.

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -78,6 +78,13 @@ void intc_route_irq_to_xen(struct irq_desc *desc, unsigned 
> int priority)
>      intc_set_irq_priority(desc, priority);
>  }
>  
> +unsigned int intc_irq_nums(void)
> +{
> +    ASSERT(intc_hw_ops && intc_hw_ops->irq_nums);
> +
> +    return intc_hw_ops->irq_nums();
> +}

You use this to set domains' properties. As indicated before, I view it as
wrong to do so for any domain, besides perhaps Dom0 / hwdom. If you want to
do so nevertheless, at the very least I'd expect something to be said about
such a decision in the description.

> --- /dev/null
> +++ b/xen/arch/riscv/vaplic.c
> @@ -0,0 +1,78 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * xen/arch/riscv/vaplic.c
> + *
> + * Virtual RISC-V Advanced Platform-Level Interrupt Controller support
> + *
> + * Copyright (c) Microchip.
> + * Copyright (c) Vates
> + */
> +
> +#include <xen/errno.h>
> +#include <xen/sched.h>
> +#include <xen/xvmalloc.h>
> +
> +#include <asm/aia.h>
> +#include <asm/imsic.h>
> +#include <asm/intc.h>
> +#include <asm/vaplic.h>
> +
> +#include "aplic-priv.h"
> +
> +#define VAPLIC_NUM_SOURCES 96
> +
> +static int cf_check vcpu_vaplic_init(struct vcpu *v)
> +{
> +    int rc = 0;

Nit: Pointless initializer.

> +    unsigned int vgein_id;
> +
> +    rc = vcpu_imsic_init(v);
> +    if ( rc )
> +        return rc;
> +
> +    if ( !(vgein_id = vgein_assign(v)) )
> +    {
> +        printk("Software interrupt files aren't supported\n");
> +        rc = -EOPNOTSUPP;
> +        goto fail;
> +    }
> +
> +    imsic_set_guest_file_id(v, vgein_id);
> +
> +    return rc;
> +
> + fail:
> +    vcpu_imsic_deinit(v);
> +
> +    return rc;
> +}
> +
> +static const struct vintc_ops vintc_ops = {
> +    .vcpu_init = vcpu_vaplic_init,
> +};
> +
> +int __init domain_vaplic_init(struct domain *d)

Why __init, and why is there no caller? Plus why is the vCPU-init a hook,
but the domain init is not? Either you mean to allow for other ICs, or
you you don't.

> +{
> +    struct vaplic *vaplic = xvzalloc(struct vaplic);
> +
> +    if ( !vaplic )
> +        return -ENOMEM;
> +
> +    d->arch.vintc = &vaplic->vintc;
> +    d->arch.vintc->ops = &vintc_ops;
> +
> +    vaplic->regs.domaincfg = APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;
> +
> +    d->arch.vintc->irq_nums = min(intc_irq_nums(),
> +                                  VAPLIC_NUM_SOURCES + 0U);
> +
> +
> +    return 0;
> +}
> +
> +void __init domain_vaplic_deinit(struct domain *d)
> +{
> +    struct vaplic *vaplic = to_vaplic(d);
> +
> +    xvfree(vaplic);
> +}

And d->arch.vintc turns into a dangling pointer. The way you arrange data
types, you can't use XVFREE() here, but imo you really want to make sure
the function is idempotent.

Jan