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

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

On 4/16/26 2:42 PM, Jan Beulich wrote:
> 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.

Sorry for that. Let me add some extra details where I think that pointer 
to physical APLIC regs are needed.

When APLIC tries to access TARGET register it is necessary to update 
real APLIC as inside this register it is coded information about Hart 
index, Guest Index (guest interrupt file id) and EIID (External 
Interrupt Identity). So to do that vintc should have access to physical 
APLIC registers.

The similar things I expected to be with some of other register, for 
example, that one which are stands for turning on/of interrupts (SETIE, 
CLRIE). If vAPLIC is requesting an enablement of an interrupt then I 
expect that correspondent physical APLIC register should be updated too 
as otherwise how then device interrupt will start to occur. So again it
is needed a pointer to physical APLIC to access these registers.

Does it make sense at least a little bit now?

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

I will return error code on the caller side of vgein_assign() if it 
returns 0.


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

Agree, this function could return struct vaplic *. I will do that.

> 
>> which is then used by to_vaplic()
>> to get struct vaplic *.
> 
> And which is what I'm saying can be avoided.

'struct vintc *vintc;' is still needed in arch_domain struct as it is 
needed to call vintc->ops->... in the case like during vCPU creation:
     if ( (rc = v->domain->arch.vintc->ops->vcpu_init(v)) )
         goto fail;

And then if 'struct vintc *vintc;' is still present in arch_domain 
struct to_vaplic() is still needed in domain_vaplic_deinit(struct domain 
*d) function
to get struct vaplic *. (All it is true for other vAPLIC functions which 
take as an argument struct domain).

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

It isn't really needed now. I can just embed ops into vintc explicitly 
without grouping them into structure.

Except the case if we want to have 'vintc_ops *ops;' field in 
arch_domain structure and separately 'void *vintc;' (which futhure could 
be casted to struct vaplic *) to drop fully to_vaplic() macros.

~ Oleksii