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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.04.2026 12:27, Oleksii Kurochko wrote:
> 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.

Thank you very much. This fully explains things, the more that of course
emulation of loads and stores comes earlier in this series. Oleksii,
really, please.

>>> +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?

vintc?

>>> --- /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)

Which is "failure".

> or something > 0 (when there are some h/w IDs).

Which is "success".

> 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.

As before - return error codes when errors occur.

>>> +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 what I'm putting under question. Why would a function of this name
return anything else than struct vaplic *?

> which is then used by to_vaplic() 
> to get struct vaplic *.

And which is what I'm saying can be avoided.

>>> +    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().

Maybe I wasn't as clear as I should have been: Why the indirection when it
doesn't abstract anything? I.e. why the "ops" field in the first place,
when everyone could access the global (until such time that abstraction
becomes necessary)?

Jan