Re: [PATCH v2 09/39] xen/riscv: implement virtual APLIC MMIO emulation

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.09.2026 12:37, Oleksii Kurochko wrote:
> On 9/9/26 4:26 PM, Jan Beulich wrote:
>> On 27.08.2026 17:20, Oleksii Kurochko wrote:
>>> +uint32_t aplic_msi_target_gen(const struct vcpu *target_vcpu,
>>> +                              uint32_t base_val)
>>> +{
>>> +    unsigned int guest_id = vcpu_guest_file_id(target_vcpu);
>>> +    unsigned long hart_field = aplic_hart_field(target_vcpu->processor);
>>
>> What guarantees target_vcpu's ->processor field to be meaningful at this
>> point? 
> 
> Good question. Considering the places where it is called, I would expect 
> target_vcpu->processor to have something meaningful, since it is called 
> from a place that can only be reached when the guest is running.
> 
> Even without that, I don't think there is a big issue here, as 
> ->processor is initialized to 0 at allocation time. This means that the 
> target register will be configured in such a way that CPU 0 will handle 
> such IRQs.

I may not have been explicit enough then: Whether the guest (as a whole)
is running is of no interest. If the specific vCPU is running, all is fine.
If the specific vCPU is in the process of being moved to a different CPU,
and if you read ->processor just before the new value is put there, is
all going to be fine as well? I doubt that.

>>> --- a/xen/arch/riscv/include/asm/vaplic.h
>>> +++ b/xen/arch/riscv/include/asm/vaplic.h
>>> @@ -21,11 +21,16 @@ struct domain;
>>>   
>>>   struct vaplic_regs {
>>>       uint32_t domaincfg;
>>> +
>>> +    uint32_t *target;
>>>   };
>>
>> A pointer in this structure is odd, as this (supposedly) is a set of
>> guest register values. The field name also doesn't clarify its purpose.
>> All in all: Likely a comment is needed here.
> 
> It is a pointer because I tried to save some memory and allocate 
> register target based on how many irqs a guest really needed.
> 
> As an option I can allocate that statically and use the value from AIA
> spec: target[1]-target[1023]. Would it better?

No, I think dynamic allocation is justified here. (Question is whether it
should be struct vaplic_regs as a whole, but that depends on whether
another runtime-sized array may need adding to it later on.)

Jan