Re: [PATCH v2 03/10] xen/arm: vgic: implement helper functions for virq checks

[Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx>]

Hi Volodymyr,

On 22.08.25 15:28, Volodymyr Babchuk wrote:
> 
> Leonid,
> 
> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
> 
>> Hi Volodymyr,
>>
>> Thank you for you comment.
>>
>> On 21.08.25 18:46, Volodymyr Babchuk wrote:
>>>
>>> Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> writes:
>>>
>>>> Introduced two new helper functions for vGIC: vgic_is_valid_irq and
>>>> vgic_is_shared_irq. The functions are similar to the newly introduced
>>>> gic_is_valid_irq and gic_is_shared_irq, but they verify whether a vIRQ
>>>> is available for a specific domain, while GIC-specific functions
>>>> validate INTIDs for the real GIC hardware. For example, the GIC may
>>>> support all 992 SPI lines, but the domain may use only some part of them
>>>> (e.g., 640), depending on the highest IRQ number defined in the domain
>>>> configuration. Therefore, for vGIC-related code and checks, the
>>>> appropriate functions should be used. Also, updated the appropriate
>>>> checks to use these new helper functions.
>>>>
>>>> The purpose of introducing new helper functions for vGIC is essentially
>>>> the same as for GIC: to avoid potential confusion with GIC-related
>>>> checks and to consolidate similar code into separate functions, which
>>>> can be more easily extended by additional conditions, e.g., when
>>>> implementing extended SPI interrupts.
>>>>
>>>> Only the validation change in vgic_inject_irq may affect existing
>>>> functionality, as it currently checks whether the vIRQ is less than or
>>>> equal to vgic_num_irqs. Since IRQ indexes start from 0 (where 32 is the
>>>> first SPI), the check should behave consistently with similar logic in
>>>> other places and should check if the vIRQ number is less than
>>>> vgic_num_irqs. The remaining changes, which replace open-coded checks
>>>> with the use of these new helper functions, do not introduce any
>>>> functional changes, as the helper functions follow the current vIRQ
>>>> index verification logic.
>>>>
>>>> Signed-off-by: Leonid Komarianskyi <leonid_komarianskyi@xxxxxxxx>
>>>>
>>>> ---
>>>> Changes in V2:
>>>> - introduced this patch
>>>> ---
>>>>    xen/arch/arm/gic.c              |  3 +--
>>>>    xen/arch/arm/include/asm/vgic.h |  7 +++++++
>>>>    xen/arch/arm/irq.c              |  4 ++--
>>>>    xen/arch/arm/vgic.c             | 10 ++++++++--
>>>>    4 files changed, 18 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>>>> index eb0346a898..47fccf21d8 100644
>>>> --- a/xen/arch/arm/gic.c
>>>> +++ b/xen/arch/arm/gic.c
>>>> @@ -133,8 +133,7 @@ int gic_route_irq_to_guest(struct domain *d, unsigned 
>>>> int virq,
>>>>    
>>>>        ASSERT(spin_is_locked(&desc->lock));
>>>>        /* Caller has already checked that the IRQ is an SPI */
>>>> -    ASSERT(virq >= 32);
>>>> -    ASSERT(virq < vgic_num_irqs(d));
>>>> +    ASSERT(vgic_is_shared_irq(d, virq));
>>>>        ASSERT(!is_lpi(virq));
>>>>    
>>>>        ret = vgic_connect_hw_irq(d, NULL, virq, desc, true);
>>>> diff --git a/xen/arch/arm/include/asm/vgic.h 
>>>> b/xen/arch/arm/include/asm/vgic.h
>>>> index 35c0c6a8b0..45201f4ca5 100644
>>>> --- a/xen/arch/arm/include/asm/vgic.h
>>>> +++ b/xen/arch/arm/include/asm/vgic.h
>>>> @@ -335,6 +335,13 @@ extern void vgic_check_inflight_irqs_pending(struct 
>>>> vcpu *v,
>>>>    /* Default number of vGIC SPIs. 32 are substracted to cover local IRQs. 
>>>> */
>>>>    #define VGIC_DEF_NR_SPIS (min(gic_number_lines(), VGIC_MAX_IRQS) - 32)
>>>>    
>>>> +extern bool vgic_is_valid_irq(struct domain *d, unsigned int virq);
>>>> +
>>>> +static inline bool vgic_is_shared_irq(struct domain *d, unsigned int virq)
>>>> +{
>>>> +    return (virq >= NR_LOCAL_IRQS && vgic_is_valid_irq(d, virq));
>>>> +}
>>>> +
>>>>    /*
>>>>     * Allocate a guest VIRQ
>>>>     *  - spi == 0 => allocate a PPI. It will be the same on every vCPU
>>>> diff --git a/xen/arch/arm/irq.c b/xen/arch/arm/irq.c
>>>> index 12c70d02cc..50e57aaea7 100644
>>>> --- a/xen/arch/arm/irq.c
>>>> +++ b/xen/arch/arm/irq.c
>>>> @@ -442,7 +442,7 @@ int route_irq_to_guest(struct domain *d, unsigned int 
>>>> virq,
>>>>        unsigned long flags;
>>>>        int retval = 0;
>>>>    
>>>> -    if ( virq >= vgic_num_irqs(d) )
>>>> +    if ( !vgic_is_valid_irq(d, virq) )
>>>>        {
>>>>            printk(XENLOG_G_ERR
>>>>                   "the vIRQ number %u is too high for domain %u (max = 
>>>> %u)\n",
>>>> @@ -560,7 +560,7 @@ int release_guest_irq(struct domain *d, unsigned int 
>>>> virq)
>>>>        int ret;
>>>>    
>>>>        /* Only SPIs are supported */
>>>> -    if ( virq < NR_LOCAL_IRQS || virq >= vgic_num_irqs(d) )
>>>> +    if ( !vgic_is_shared_irq(d, virq) )
>>>>            return -EINVAL;
>>>>    
>>>>        desc = vgic_get_hw_irq_desc(d, NULL, virq);
>>>> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
>>>> index c563ba93af..48fbaf56fb 100644
>>>> --- a/xen/arch/arm/vgic.c
>>>> +++ b/xen/arch/arm/vgic.c
>>>> @@ -24,6 +24,12 @@
>>>>    #include <asm/gic.h>
>>>>    #include <asm/vgic.h>
>>>>    
>>>> +
>>>> +bool vgic_is_valid_irq(struct domain *d, unsigned int virq)
>>>
>>> I have the same comment as for the previous patch. This function
>>> completely ignores LPIs presence, while you can't argue that LPIs as
>>> valid. Again, function callers are expecting this behavior, so this is
>>> fine, but function name should better reflect its behavior.
>>>
>>> [...]
>>>
>>
>> Would it be okay to rename these functions as proposed in the previous
>> patch discussion:
>> vgic_is_valid_irq -> vgic_is_valid_line
>> vgic_is_shared_irq -> vgic_is_spi?
>>
>> Or, in the case of vgic, is it not a good idea to use the "line" suffix
>> because vgic does not have physical interrupt lines? Would it be better
>> to rename it to vgic_is_valid_non_lpi instead?
> 
> I think it is better to follow the pGIC naming convention. While there
> is no physical IRQ lines in vGIC, it emulates real GIC anyways.
> 

Okay, good point. I will rename the vGIC-related functions in the pGIC 
manner in V3.

Best regards,
Leonid