Re: [Xen-devel] [PATCH 10/12] ARM: VGIC: factor out vgic_connect_hw_irq()

[Stefano Stabellini]

On Thu, 19 Oct 2017, Andre Przywara wrote:
> At the moment we happily access VGIC internal data structures like
> the rank and struct pending_irq in gic.c, which should be VGIC agnostic.
> 
> Factor out a new function vgic_connect_hw_irq(), which allows a virtual
> IRQ to be connected to a hardware IRQ (using the hw bit in the LR).
> 
> This removes said accesses to VGIC data structures and improves abstraction.
> 
> Signed-off-by: Andre Przywara 

Acked-by: Stefano Stabellini 


> ---
>  xen/arch/arm/gic-vgic.c| 31 +++
>  xen/arch/arm/gic.c | 42 ++
>  xen/include/asm-arm/vgic.h |  2 ++
>  3 files changed, 39 insertions(+), 36 deletions(-)
> 
> diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
> index 66cae21e82..bf9455a34e 100644
> --- a/xen/arch/arm/gic-vgic.c
> +++ b/xen/arch/arm/gic-vgic.c
> @@ -385,6 +385,37 @@ void gic_inject(struct vcpu *v)
>  gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
>  }
>  
> +int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
> +struct irq_desc *desc)
> +{
> +unsigned long flags;
> +/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> + * route SPIs to guests, it doesn't make any difference. */
> +struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> +struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> +struct pending_irq *p = irq_to_pending(v_target, virq);
> +int ret = 0;
> +
> +/* We are taking to rank lock to prevent parallel connections. */
> +vgic_lock_rank(v_target, rank, flags);
> +
> +if ( desc )
> +{
> +/* The VIRQ should not be already enabled by the guest */
> +if ( !p->desc &&
> + !test_bit(GIC_IRQ_GUEST_ENABLED, >status) )
> +p->desc = desc;
> +else
> +ret = -EBUSY;
> +}
> +else
> +p->desc = NULL;
> +
> +vgic_unlock_rank(v_target, rank, flags);
> +
> +return ret;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 4cb74d449e..d46a6d54b3 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -128,27 +128,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc, 
> unsigned int priority)
>  int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
> struct irq_desc *desc, unsigned int priority)
>  {
> -unsigned long flags;
> -/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
> - * route SPIs to guests, it doesn't make any difference. */
> -struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -struct pending_irq *p = irq_to_pending(v_target, virq);
> -int res = -EBUSY;
> -
>  ASSERT(spin_is_locked(>lock));
>  /* Caller has already checked that the IRQ is an SPI */
>  ASSERT(virq >= 32);
>  ASSERT(virq < vgic_num_irqs(d));
>  ASSERT(!is_lpi(virq));
>  
> -vgic_lock_rank(v_target, rank, flags);
> -
> -if ( p->desc ||
> - /* The VIRQ should not be already enabled by the guest */
> - test_bit(GIC_IRQ_GUEST_ENABLED, >status) )
> -goto out;
> -
>  desc->handler = gic_hw_ops->gic_guest_irq_type;
>  set_bit(_IRQ_GUEST, >status);
>  
> @@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned 
> int virq,
>  gic_set_irq_type(desc, desc->arch.type);
>  gic_set_irq_priority(desc, priority);
>  
> -p->desc = desc;
> -res = 0;
> -
> -out:
> -vgic_unlock_rank(v_target, rank, flags);
> -
> -return res;
> +return vgic_connect_hw_irq(d, NULL, virq, desc);
>  }
>  
>  /* This function only works with SPIs for now */
>  int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
>struct irq_desc *desc)
>  {
> -struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
> -struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
> -struct pending_irq *p = irq_to_pending(v_target, virq);
> -unsigned long flags;
> +int ret;
>  
>  ASSERT(spin_is_locked(>lock));
>  ASSERT(test_bit(_IRQ_GUEST, >status));
> -ASSERT(p->desc == desc);
>  ASSERT(!is_lpi(virq));
>  
> -vgic_lock_rank(v_target, rank, flags);
> -
>  if ( d->is_dying )
>  {
>  desc->handler->shutdown(desc);
> @@ -198,19 +171,16 @@ int gic_remove_irq_from_guest(struct domain *d, 
> unsigned int virq,
>   */
>  if ( test_bit(_IRQ_INPROGRESS, >status) ||
>   !test_bit(_IRQ_DISABLED, >status) )
> -{
> -vgic_unlock_rank(v_target, rank, flags);
>  return -EBUSY;
> -}
>  }
>  
> +ret = vgic_connect_hw_irq(d, NULL, virq, NULL);
> +if ( ret )
> +return ret;
> 

[Xen-devel] [PATCH 10/12] ARM: VGIC: factor out vgic_connect_hw_irq()

[Andre Przywara]

At the moment we happily access VGIC internal data structures like
the rank and struct pending_irq in gic.c, which should be VGIC agnostic.

Factor out a new function vgic_connect_hw_irq(), which allows a virtual
IRQ to be connected to a hardware IRQ (using the hw bit in the LR).

This removes said accesses to VGIC data structures and improves abstraction.

Signed-off-by: Andre Przywara 
---
 xen/arch/arm/gic-vgic.c| 31 +++
 xen/arch/arm/gic.c | 42 ++
 xen/include/asm-arm/vgic.h |  2 ++
 3 files changed, 39 insertions(+), 36 deletions(-)

diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c
index 66cae21e82..bf9455a34e 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -385,6 +385,37 @@ void gic_inject(struct vcpu *v)
 gic_hw_ops->update_hcr_status(GICH_HCR_UIE, 1);
 }
 
+int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+struct irq_desc *desc)
+{
+unsigned long flags;
+/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
+ * route SPIs to guests, it doesn't make any difference. */
+struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
+struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
+struct pending_irq *p = irq_to_pending(v_target, virq);
+int ret = 0;
+
+/* We are taking to rank lock to prevent parallel connections. */
+vgic_lock_rank(v_target, rank, flags);
+
+if ( desc )
+{
+/* The VIRQ should not be already enabled by the guest */
+if ( !p->desc &&
+ !test_bit(GIC_IRQ_GUEST_ENABLED, >status) )
+p->desc = desc;
+else
+ret = -EBUSY;
+}
+else
+p->desc = NULL;
+
+vgic_unlock_rank(v_target, rank, flags);
+
+return ret;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 4cb74d449e..d46a6d54b3 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -128,27 +128,12 @@ void gic_route_irq_to_xen(struct irq_desc *desc, unsigned 
int priority)
 int gic_route_irq_to_guest(struct domain *d, unsigned int virq,
struct irq_desc *desc, unsigned int priority)
 {
-unsigned long flags;
-/* Use vcpu0 to retrieve the pending_irq struct. Given that we only
- * route SPIs to guests, it doesn't make any difference. */
-struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
-struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
-struct pending_irq *p = irq_to_pending(v_target, virq);
-int res = -EBUSY;
-
 ASSERT(spin_is_locked(>lock));
 /* Caller has already checked that the IRQ is an SPI */
 ASSERT(virq >= 32);
 ASSERT(virq < vgic_num_irqs(d));
 ASSERT(!is_lpi(virq));
 
-vgic_lock_rank(v_target, rank, flags);
-
-if ( p->desc ||
- /* The VIRQ should not be already enabled by the guest */
- test_bit(GIC_IRQ_GUEST_ENABLED, >status) )
-goto out;
-
 desc->handler = gic_hw_ops->gic_guest_irq_type;
 set_bit(_IRQ_GUEST, >status);
 
@@ -156,31 +141,19 @@ int gic_route_irq_to_guest(struct domain *d, unsigned int 
virq,
 gic_set_irq_type(desc, desc->arch.type);
 gic_set_irq_priority(desc, priority);
 
-p->desc = desc;
-res = 0;
-
-out:
-vgic_unlock_rank(v_target, rank, flags);
-
-return res;
+return vgic_connect_hw_irq(d, NULL, virq, desc);
 }
 
 /* This function only works with SPIs for now */
 int gic_remove_irq_from_guest(struct domain *d, unsigned int virq,
   struct irq_desc *desc)
 {
-struct vcpu *v_target = vgic_get_target_vcpu(d->vcpu[0], virq);
-struct vgic_irq_rank *rank = vgic_rank_irq(v_target, virq);
-struct pending_irq *p = irq_to_pending(v_target, virq);
-unsigned long flags;
+int ret;
 
 ASSERT(spin_is_locked(>lock));
 ASSERT(test_bit(_IRQ_GUEST, >status));
-ASSERT(p->desc == desc);
 ASSERT(!is_lpi(virq));
 
-vgic_lock_rank(v_target, rank, flags);
-
 if ( d->is_dying )
 {
 desc->handler->shutdown(desc);
@@ -198,19 +171,16 @@ int gic_remove_irq_from_guest(struct domain *d, unsigned 
int virq,
  */
 if ( test_bit(_IRQ_INPROGRESS, >status) ||
  !test_bit(_IRQ_DISABLED, >status) )
-{
-vgic_unlock_rank(v_target, rank, flags);
 return -EBUSY;
-}
 }
 
+ret = vgic_connect_hw_irq(d, NULL, virq, NULL);
+if ( ret )
+return ret;
+
 clear_bit(_IRQ_GUEST, >status);
 desc->handler = _irq_type;
 
-p->desc = NULL;
-
-vgic_unlock_rank(v_target, rank, flags);
-
 return 0;
 }
 
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index dcdb1acaf3..cf02dc6394 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -220,6 +220,8 @@ int vgic_v2_init(struct