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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

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.

-- 
WBR, Volodymyr