Leonid,
Leonid Komarianskyi <leonid_komarians...@epam.com> writes:
> 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?
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
