Re: [PATCH v4 08/12] xen/arm: vgic: add resource management for extended SPIs

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr,

Thank you for your review comment.

On 28.08.25 02:01, Volodymyr Babchuk wrote:
> Hi Leonid,
> 
> Leonid Komarianskyi<Leonid_Komarianskyi@xxxxxxxx> writes:
> 
>> This change introduces resource management in the VGIC to handle
>> extended SPIs introduced in GICv3.1. The pending_irqs and
>> allocated_irqs arrays are resized to support the required
>> number of eSPIs, based on what is supported by the hardware and
>> requested by the guest. A new field, ext_shared_irqs, is added
>> to the VGIC structure to store information about eSPIs, similar
>> to how shared_irqs is used for regular SPIs.
>>
>> Since the eSPI range starts at INTID 4096 and INTIDs between 1025
>> and 4095 are reserved, helper macros are introduced to simplify the
>> transformation of indices and to enable easier access to eSPI-specific
>> resources. These changes prepare the VGIC for processing eSPIs as
>> required by future functionality.
>>
>> The initialization and deinitialization paths for vgic have been updated
>> to allocate and free these resources appropriately. Additionally,
>> updated handling of INTIDs greater than 1024, passed from the toolstack
>> during domain creation, and verification logic ensures only valid SPI or
>> eSPI INTIDs are used.
>>
>> The existing SPI behavior remains unaffected when guests do not request
>> eSPIs, GIC hardware does not support them, or the CONFIG_GICV3_ESPI
>> option is disabled.
>>
>> Signed-off-by: Leonid Komarianskyi<leonid_komarianskyi@xxxxxxxx>
>>
>> ---
>> Changes in V4:
>> - added has_espi field to simplify determining whether a domain is able
>>    to operate with eSPI
> I don't think that this is a good idea. You already have an invariant that
> tells if domain has eSPIs: d->nr_espis != 0. If you introduce a new
> field, you now have to keep these two values coherent or deal with possible 
> cases
> like d->nr_espis == 0 && d->has_espi == true
> 
> Also, this new field is not used anywhere, so why adding it in the first
> place?

I just wanted to simplify the checks in the next patch:
https://patchew.org/Xen/cover.1756317702.git.leonid._5Fkomarianskyi@xxxxxxxx/6b312e1997da5abdf592f66d16067f4330431ded.1756317702.git.leonid._5Fkomarianskyi@xxxxxxxx/
e.g.:

+        if ( !v->domain->arch.vgic.has_espi )
+            goto read_reserved;

But yes, I agree that it looks redundant. Would it be okay if I drop 
this change in V5 and modify the checks in the next patch to something 
like this?

+        if ( v->domain->arch.vgic.nr_espis == 0 )
+            goto read_reserved;

Best regards,
Leonid