Re: [PATCH v1 15/27] xen/riscv: add very early virtual APLIC (vAPLIC) initialization support

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.03.2026 18:08, Oleksii Kurochko wrote:
> @@ -47,6 +48,19 @@ struct intc_hw_operations {
>                              const struct dt_device_node *intc);
>  };
>  
> +struct vintc_ops {
> +    /* Initialize some vINTC-related stuff for a vCPU */
> +    int (*vcpu_init)(struct vcpu *vcpu);

v as the parameter name, to fit our convention? (Same below for the other
hook.)

> +    /* Check if a register is virtual interrupt controller MMIO */
> +    int (*is_access)(const struct vcpu *vcpu, const unsigned long addr);

What does "register" in the comment refer to. All I see is an address.
(The const will also want dropping from the parameter in this declaration.)

> +};
> +
> +struct vintc {
> +    const struct intc_info *info;

Isn't this referencing a physical INTC's structure? Why would the virtual
one's properties have to match that of the physical one?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/vaplic.h
> @@ -0,0 +1,36 @@
> +/* 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(v) container_of(v, struct vaplic, base)

I'm confused here, maybe first of all because of the use of v. v is our
common identified for struct vcpu * instances. Using it in a macro like
this one suggests a struct vcpu * needs passing into the macro. Yet from
the two uses of the macro that doesn't look to be the case.

Perhaps best to have a struct domain * passed into here?

> +struct vaplic_regs {
> +    uint32_t domaincfg;
> +    uint32_t smsiaddrcfg;
> +    uint32_t smsiaddrcfgh;

The latter two aren't used, and generally I'd expect a h-suffixed field to
exist only for RV32. (The un-suffixed field then would need to be unsigned
long, of course.)

> +};
> +
> +struct vaplic {
> +    struct vintc base;

How does "base" fit with the type of the field?

> --- a/xen/arch/riscv/intc.c
> +++ b/xen/arch/riscv/intc.c
> @@ -6,6 +6,7 @@
>  #include <xen/init.h>
>  #include <xen/irq.h>
>  #include <xen/lib.h>
> +#include <xen/sched.h>
>  #include <xen/spinlock.h>

Why is this change needed all of the sudden?

> --- /dev/null
> +++ b/xen/arch/riscv/vaplic.c
> @@ -0,0 +1,74 @@
> +/* 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"
> +
> +static int __init cf_check vcpu_vaplic_init(struct vcpu *v)
> +{
> +    int rc = 0;
> +
> +    rc = vcpu_imsic_init(v);
> +    if ( rc )
> +        return rc;
> +
> +    imsic_set_guest_file_id(v, vgein_assign(v));

And vgein_assign() can't fail? (Rhetorical question - of course it can. That
function shouldn't assert that it can fine a valid ID.)

But then - aren't you limiting the number of vCPU-s a host can handle by the
number vgein IDs?

> +    return rc;
> +}
> +
> +static const struct vintc_ops vaplic_ops = {
> +    .vcpu_init = vcpu_vaplic_init,
> +};
> +
> +static struct vintc * __init vaplic_alloc(void)
> +{
> +    struct vaplic *v = NULL;

Onve again - why the initializer? In fact, ...

> +    v = xvzalloc(struct vaplic);

... this could be the initializer.

> +    if ( !v )
> +        return NULL;
> +
> +    return &v->base;
> +}

If you returned and ...

> +int __init domain_vaplic_init(struct domain *d)
> +{
> +    int ret = 0;
> +
> +    d->arch.vintc = vaplic_alloc();

... stored struct vaplic *, the slightly odd to_vaplic() macro wouldn't
be needed.

> +    if ( !d->arch.vintc )
> +    {
> +        ret = -ENOMEM;
> +        goto fail;

Nit: goto when simply return could be used.

> +    }
> +
> +    d->arch.vintc->ops = &vaplic_ops;

Are other kinds of ops structures going to appear? If not, why the extra
indirection?

> +    to_vaplic(d->arch.vintc)->regs.domaincfg =
> +        APLIC_DOMAINCFG_IE | APLIC_DOMAINCFG_DM;
> +
> + fail:
> +    return ret;
> +}
> +
> +void __init domain_vaplic_deinit(struct domain *d)
> +{
> +    struct vaplic *vaplic = to_vaplic(d->arch.vintc);
> +
> +    XVFREE(vaplic);

If this cleared the struct domain field, then yes. But the way it is, just
xvfree() will suffice. (Re-work following other remarks may want it to
become XVFREE() again, though.)

Jan