Hello Volodymyr,
Thank you for your review comments.
On 26.08.25 23:02, Volodymyr Babchuk wrote:
>
> Hi Leonid,
>
>
> Leonid Komarianskyi <leonid_komarians...@epam.com> writes:
>
>> Introduced two new helper functions for vGIC: vgic_is_valid_line and
>> vgic_is_spi. The functions are similar to the newly introduced
>> gic_is_valid_line and gic_is_spi, 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
>>
>> Changes in V3:
>> - renamed vgic_is_valid_irq to vgic_is_valid_line and vgic_is_shared_irq
>> to vgic_is_spi
>> - added vgic_is_valid_line implementation for new-vgic, because
>> vgic_is_valid_line is called from generic code. It is necessary to fix
>> the build for new-vgic.
>> - updated commit message
>> ---
>> 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 ++++++++--
>> xen/arch/arm/vgic/vgic.c | 5 +++++
>> 5 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 9220eef6ea..b88237ccda 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_spi(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..9f437e9838 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_line(struct domain *d, unsigned int virq);
>
> Why you can't have inline implementation for vgic_is_valid_line() right here?
>
Initially, I tried to implement an inline function in the header, but it
led to a build error:
> In file included from ./arch/arm/include/asm/domain.h:11,
> from ./include/xen/domain.h:16,
> from ./include/xen/sched.h:11,
> from arch/arm/arm64/asm-offsets.c:9:
> ./arch/arm/include/asm/vgic.h: In function ‘vgic_is_valid_line’:
> ./arch/arm/include/asm/vgic.h:330:37: error: invalid use of undefined
> type ‘struct domain’
> 330 | #define vgic_num_irqs(d) ((d)->arch.vgic.nr_spis + 32)
> | ^~
> ./arch/arm/include/asm/vgic.h:340:19: note: in expansion of macro
‘vgic_num_irqs’
> 340 | return virq < vgic_num_irqs(d);
| ^~~~~~~~~~~~~
> make[2]: *** [build.mk:45: asm-offsets.s] Error 1
This occurs because the arch_domain structure, which is defined in
asm/domain.h, requires the vgic header (as it contains the vgic_dist
structure field), and the struct domain (defined in xen/sched.h)
requires xen/domain.h to use the arch_domain structure. As a result,
when attempting to inline the vgic_is_valid_line in the vgic header, the
struct domain is not yet defined at the compilation time of the
function, which led to the following build error. Also, there is no
appropriate common .c file (with the vgic prefix) to place the actual
function implementation for both configurations in a single location.
By the way, some function implementations use the same approach (e.g.,
vgic_reserve_virq, which currently has two identical copies - one for
vgic and one for new-vgic).
Perhaps I missed something, but it seems like this is the most suitable
solution for this..
>> +
>> +static inline bool vgic_is_spi(struct domain *d, unsigned int virq)
>> +{
>> + return (virq >= NR_LOCAL_IRQS && vgic_is_valid_line(d, virq));
>
> You don't need parentheses here.
>
I will remove them in V4.
>> +}
>> +
>> /*
>> * 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 7dd5a2a453..b8eccfc924 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_line(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_spi(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..2bbf4d99aa 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_line(struct domain *d, unsigned int virq)
>
> Implementation of this function here in for new vgic is basically the
> same and depends only on vgic_num_irqs() which is macro defined in
> vgic.h and used by both implementations.
>
>> +{
>> + return virq < vgic_num_irqs(d);
>> +}
>> +
>> static inline struct vgic_irq_rank *vgic_get_rank(struct vcpu *v,
>> unsigned int rank)
>> {
>> @@ -582,7 +588,7 @@ void vgic_inject_irq(struct domain *d, struct vcpu *v,
>> unsigned int virq,
>> if ( !v )
>> {
>> /* The IRQ needs to be an SPI if no vCPU is specified. */
>> - ASSERT(virq >= 32 && virq <= vgic_num_irqs(d));
>> + ASSERT(vgic_is_spi(d, virq));
>>
>> v = vgic_get_target_vcpu(d->vcpu[0], virq);
>> };
>> @@ -659,7 +665,7 @@ bool vgic_emulate(struct cpu_user_regs *regs, union hsr
>> hsr)
>>
>> bool vgic_reserve_virq(struct domain *d, unsigned int virq)
>> {
>> - if ( virq >= vgic_num_irqs(d) )
>> + if ( !vgic_is_valid_line(d, virq) )
>> return false;
>>
>> return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
>> index 6cabd0496d..b2c0e1873a 100644
>> --- a/xen/arch/arm/vgic/vgic.c
>> +++ b/xen/arch/arm/vgic/vgic.c
>> @@ -718,6 +718,11 @@ bool vgic_reserve_virq(struct domain *d, unsigned int
>> virq)
>> return !test_and_set_bit(virq, d->arch.vgic.allocated_irqs);
>> }
>>
>> +bool vgic_is_valid_line(struct domain *d, unsigned int virq)
>> +{
>> + return virq < vgic_num_irqs(d);
>> +}
>
>> +
>> int vgic_allocate_virq(struct domain *d, bool spi)
>> {
>> int first, end;
>
Best regards,
Leonid
