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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 4/2/26 1:58 PM, Jan Beulich wrote:
> 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 {
> > +    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?
It is because of how vAPLIC emulation load and store is working.

> > --- /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.
v it is stale name when vpalic() was per vCPU. (what looks incorrect as 
on real h/w APLIC isn't belongs to one pCPU).
> Perhaps best to have a struct domain * passed into here?
struct domain * will be better.

> > +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.)
It should be a part of another patch. I will drop them for now.

> > +};
> > +
> > +struct vaplic {
> > +    struct vintc base;
> How does "base" fit with the type of the field?
The field name base is a idiom for embedding a "base class" struct as 
the first member, enabling a form of inheritance.
Any suggestion how to rename it better?

> > --- 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?
Incorrect rebase. I will drop that.

> > --- /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.)
Technically it can't fail (except some bug of course), this function 
should in general return 0 (when there aren't left h/w IDs) or something 
> 0 (when there are some h/w IDs). ASSERT() inside it was added only 
because of ...
> But then - aren't you limiting the number of vCPU-s a host can handle by the
> number vgein IDs?
... At the moment, I am limiting because S/W interrutps guest files 
(IDs) aren't supported.
> > +    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.
Sure, I will use it as 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.
vaplic_alloc() return struct vintc *, which is then used by to_vaplic() 
to get struct vaplic *.
> > +    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?
At the moment, no I don't see any other kinds of ops struct. It was just 
convenient way to group them and then easier to initialize them - just 
one assignment instead of addinng a separate line in domain_vaplic_init().
> > +    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.)
Agree, it could be xvfree() for now.

Thanks.

~ Oleksii