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

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

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