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

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Leonid,


Leonid Komarianskyi <Leonid_Komarianskyi@xxxxxxxx> 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_komarianskyi@xxxxxxxx>
> ---
> 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?

> +
> +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.

> +}
> +
>  /*
>   * 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;

-- 
WBR, Volodymyr