Hi Volodymyr,
Thank you for you comment.
On 21.08.25 18:46, Volodymyr Babchuk wrote:
>
> Leonid Komarianskyi <leonid_komarians...@epam.com> 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_komarians...@epam.com>
>>
>> ---
>> 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?
Best regards,
Leonid
