[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 5/8] ARM: VGIC: factor out vgic_connect_hw_irq()
Hi Andre,
On 24/01/18 18:10, 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.
You are modifying the locking of the 2 functions. But I don't see how
this is safe. Can you explain it?
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
Acked-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
---
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 0a2958c5db..d44e4dacd3 100644
--- a/xen/arch/arm/gic-vgic.c
+++ b/xen/arch/arm/gic-vgic.c
@@ -411,6 +411,37 @@ void gic_dump_vgic_info(struct vcpu *v)
printk("Pending irq=%d\n", p->irq);
}
+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. */
Can you fix the coding style of the comment?
+ 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, &p->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(&desc->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, &p->status) )
- goto out;
-
desc->handler = gic_hw_ops->gic_guest_irq_type;
set_bit(_IRQ_GUEST, &desc->status);
This looks wrong to me. You don't want to execute any of the code below
if you are not able to route the pIRQ. For instance because the vIRQ has
already a desc assigned.
@@ -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(&desc->lock));
ASSERT(test_bit(_IRQ_GUEST, &desc->status));
- ASSERT(p->desc == desc);
You dropped this assert but I don't see any replacement in the new code?
We really want to make sure the caller will not do something dumb here
(like passing a different 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, &desc->status) ||
!test_bit(_IRQ_DISABLED, &desc->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, &desc->status);
desc->handler = &no_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 22c8502c95..f4240df371 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -219,6 +219,8 @@ int vgic_v2_init(struct domain *d, int *mmio_count);
int vgic_v3_init(struct domain *d, int *mmio_count);
bool vgic_evtchn_irq_pending(struct vcpu *v);
+int vgic_connect_hw_irq(struct domain *d, struct vcpu *v, unsigned int virq,
+ struct irq_desc *desc);
extern int domain_vgic_register(struct domain *d, int *mmio_count);
extern int vcpu_vgic_free(struct vcpu *v);
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
