Re: [PATCH v1 02/17] xen/riscv: add basic VGEIN management for AIA guests

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.07.2026 17:46, Oleksii Kurochko wrote:
> On 7/30/26 9:42 AM, Jan Beulich wrote:
>> On 29.07.2026 16:55, Oleksii Kurochko wrote:
>>> On 7/27/26 5:41 PM, Jan Beulich wrote:
>>>> On 20.07.2026 18:02, Oleksii Kurochko wrote:
>>>>> It was decided to add support for IMSIC from the start instead of having 
>>>>> APLIC
>>>>> operate in direct delivery mode, as it requires a trap-and-emulation 
>>>>> approach,
>>>>> which is not optimal from a performance standpoint.
>>>>>
>>>>> AIA provides a hardware-accelerated mechanism for delivering external
>>>>> interrupts to domains via "guest interrupt files" located in IMSIC.
>>>>> A single physical hart can implement multiple such files (up to GEILEN),
>>>>> allowing several virtual harts to receive interrupts directly from 
>>>>> hardware.
>>>>>
>>>>> Introduce per-CPU tracking of guest interrupt file identifiers (VGEIN)
>>>>> for systems implementing AIA specification. Each CPU maintains
>>>>> a bitmap describing which guest interrupt files are currently in use.
>>>>>
>>>>> Add helpers to initialize the bitmap based on the number of available
>>>>> guest interrupt files (GEILEN), assign a VGEIN to a vCPU, and release it
>>>>> when no longer needed. When assigning a VGEIN, the corresponding value
>>>>> is written to the VGEIN field of the guest hstatus register so that
>>>>> VS-level external interrupts are delivered from the selected interrupt
>>>>> file.
>>>>
>>>> And when exactly is this "assignment" intended to occur? vgein_assign() and
>>>> vgein_release() have no callers here, so this remains entirely unclear.
>>>
>>> [A] Agreed, I should have added that information to the commit message:
>>>
>>> VGEIN is assigned (via vgein_assign()) before jumping to the new vCPU
>>> execution context (in continue_new_vcpu()) and is re-assigned during
>>> vCPU migration from one pCPU to another.
>>>
>>> VGEIN is released (via vgein_release()) on the old pCPU during migration.
>>
>> That is, state of that vCPU is held in hardware for perhaps an extended
>> period of time after the vCPU was last de-scheduled. That's a fair
>> optimization (we do something similar on x86, albeit that has been
>> increasingly under question lately). However, doesn't this then require
>> sync_local_execstate() to become non-empty?
> 
> IIUC, sync_local_execstate() is needed for the lazy context switch case 
> when switching from vCPUA to the idle vCPU.

Or when full state is to be obtained for a vCPU, for example.

> The idea is that if, after 
> running the idle vCPU, the next scheduled vCPU is again vCPUA, then 
> nothing needs to be done because no real context switch has occurred yet.
> 
> IMO, this is not the case for VGEIN. It is perfectly fine for a vCPU to 
> keep its previously assigned VGEIN even if the next scheduled vCPU is 
> different. In fact, it is necessary to preserve VGEIN because it will be 
> needed later, for example, to wake up the vCPU if an interrupt for that 
> vCPU occurs.
> 
> The idea is that if a vCPU uses a hardware interrupt file, then when the 
> vCPU is descheduled, the corresponding CSR_HGEIE bit, where the bit 
> number corresponds to the VGEIN value, is set. If an IRQ_S_GEXT trap 
> then occurs, the hgei_interrupt() handler can determine which vCPU 
> should be woken up:
> 
> void hgei_interrupt(void)
> {
>      unsigned long hgei_mask, flags;
>      struct vgein_ctrl *vgein_ = &this_cpu(vgein);
> 
>      hgei_mask = csr_read(CSR_HGEIP) & csr_read(CSR_HGEIE);
>      csr_clear(CSR_HGEIE, hgei_mask);
> 
>      spin_lock_irqsave(&vgein_->lock, flags);
> 
>      for_each_set_bit ( vs_guest_file_id, hgei_mask )
>      {
>         ...
>         /* do some logic to call vcpu_kick */
>         ...
>      }
> 
>      spin_unlock_irqrestore(&vgein_->lock, flags);
> }

Ah, that's pretty helpful extra information.

>> Furthermore, rather than having vgein_assign() fail when
>> find_next_zero_bit() fails to find an available ID, shouldn't you release
>> some other vCPU's ID, making it available for re-use?
> 
> This is a good question, and it requires a separate investigation to 
> determine whether such an approach would actually be beneficial. It 
> would require not only changing the VGEIN field in vcpu->hstatus, but 
> also synchronizing at least the pending interrupts from one IMSIC 
> interrupt file to another, which would also consume time and further 
> complicate the logic.
> 
> If find_next_zero_bit() fails, the vCPU will simply receive VGEIN=0, 
> which means that the software interrupt file will be used. Therefore, 
> everything should continue to work correctly, although it will be slower 
> than using a hardware interrupt file.

Plus there may end up being subtly different behavior. Imo you want to
let the hardware do what it can do for you.

> Considering that the maximum value of GEILEN is 31 for RV32 and 63 for 
> RV64 (although there is no guarantee that an implementation will support 
> the maximum value), let's assume a GEILEN value of 31 for RV64 as well. 
> In that case, a system with 4 CPUs would cover the maximum number of 
> vCPUs supported by Xen (IIURC, it is 128).

That's a single domain. There can be many domains, totaling to far more
than 128 vCPU-s.

> Therefore, a software 
> interrupt file would not be needed at all, assuming the scheduler 
> distributes vCPUs reasonably well.
> 
> Even if GEILEN is smaller, I expect that the scheduler will migrate 
> vCPUs between pCPUs from time to time. This will free a hardware IMSIC 
> interrupt file slot on the previous pCPU, allowing another vCPU to 
> obtain a hardware IMSIC interrupt file slot.
> 
> For now, I would prefer to keep the current VGEIN allocation strategy as 
> it is definitely easier for implementation at least and consider your 
> suggestion of releasing another vCPU's VGEIN as a potential 
> optimization. I think this optimization should first be evaluated 
> through measurements and experiments.

Well, I'm not going to insist, but I expect this will need re-doing rather
sooner than later then.

>> Yet then I continue to question the presence of this array in the first 
>> place.
>> Something similar isn't needed elsewhere (afaik), and its intended use (as
>> said) doesn't become obvious here.
> 
> I can drop it for now and reintroduce it later when it is actually 
> needed. In short, it is intended to be used in hgei_interrupt(), as I 
> described above, in the following way (inside the for-loop):
> 
> ...
> for_each_set_bit(vs_guest_file_id, hgei_mask)
> {
> unsigned int owners_index = vs_guest_file_id /* - 1 */;
> 
> ```
> if ( vgein_->owners[owners_index] )
> {
>      dprintk("kick ->%pv, hgei_mask(%#lx)\n",
>              vgein_->owners[owners_index], hgei_mask);
> 
>      vcpu_kick(vgein_->owners[owners_index]);
> }
> ```
> 
> }
> ...
> 
> Alternatively, I could introduce this in this series, since it will be 
> necessary to set the HGEIE bit in imsic_state_save() anyway to allow the 
> vCPU to be woken up. It also seems like the best option, as it addresses 
> at least some of the comments you raised.

Right, and then preferably in an order where one won't need to peek ahead
in the series to actually understand what's going on.

>>>>> +unsigned int vgein_assign(struct vcpu *v)
>>>>> +{
>>>>> +    unsigned int vgein_id;
>>>>> +    struct vgein_ctrl *vgein = &per_cpu(vgein, v->processor);
>>>>> +    unsigned long *bmp = &vgein->bmp;
>>>>> +    unsigned long flags;
>>>>> +
>>>>> +    if ( !vgein->geilen )
>>>>> +        return 0;
>>>>> +
>>>>> +    spin_lock_irqsave(&vgein->lock, flags);
>>>>
>>>> Because it's unclear where this is to be called from, it's also unclear 
>>>> whether
>>>> a lock is needed here (and if so whether a plain spin lock is appropriate).
>>>
>>> Based on what I wrote in [A] above a lock is defintely needed as it
>>> could be that vgein_release() is called for old pCPU during migration
>>> and at the same time old pCPU could call vgein_assign() so we want to
>>> keep vgein bitmap consistent.
>>
>> Can this really happen? It almost sounds as if you were suspecting
>> context-switch-in could race with context-switch-out. Yet again - none of
>> this can sensibly be discussed without seeing how / where the functions are
>> to be used.
> 
> Maybe I didn't explain it clearly, but during migration (which, 
> according to my understanding of vcpu_move_irqs(), is executed on 
> pCPU1), when vCPU0 is migrated from pCPU0 to pCPU1, its old VGEIN on 
> pCPU0 needs to be released. I don't see any reason why, at the same 
> time, pCPU0 could not try to assign that VGEIN to another vCPU. Without 
> proper protection, this could lead to race conditions.

Doesn't migration of vCPU-s between pCPU-s happen under suitable scheduler
locks?

> Also, setting a bit in the VGEIN bitmap and updating the owner array 
> should be an atomic operation, at least to correctly handle the 
> hgei_interrupt() case mentioned above and vCPU migration, which calls 
> vgein_release(...,old_pcpu,...).
> 
> I agree that it would probably be easier if the migration patches were 
> included in this patch series as well. I can either post those patches 
> to this thread now or include them in the v2 series when it is ready. 
> What do you think?

Including in v2 may be helpful, again to eliminate gaps in the understanding
a reader like me needs to have.

Jan