Re: [Xen-devel] [PATCH v3 14/39] ARM: new VGIC: Add GICv2 world switch backend

[Julien Grall]

Hi Andre,

On 03/22/2018 11:04 AM, Andre Przywara wrote:

This is a "patch to the patch" mentioned above, to make it clear what
changed:
We now take the desc lock in vgic_v2_fold_lr_state() when we are dealing
with a hardware IRQ. This is a bit complicated, because we have to obey
the existing locking order, so do our infamous "drop-take-retake" dance.
Also I print a message about using the new VGIC and fix that last
remaining "u32" usage.

Please note that I had to initialise "desc" to NULL because my compiler
(GCC 5.3) is not smart enough to see that we only use it with irq->hw
set and it's safe. Please let me know if it's me not being smart enough
here instead ;-)


I would not be surprised that even recent compiler can't deal with that. 
It would require quite some work from the compiler to know that desc is 
only used when irq->hw.


I will comment the code on 3a.

Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 14/39] ARM: new VGIC: Add GICv2 world switch backend

[Andre Przywara]

This is a "patch to the patch" mentioned above, to make it clear what
changed:
We now take the desc lock in vgic_v2_fold_lr_state() when we are dealing
with a hardware IRQ. This is a bit complicated, because we have to obey
the existing locking order, so do our infamous "drop-take-retake" dance.
Also I print a message about using the new VGIC and fix that last
remaining "u32" usage.

Please note that I had to initialise "desc" to NULL because my compiler
(GCC 5.3) is not smart enough to see that we only use it with irq->hw
set and it's safe. Please let me know if it's me not being smart enough
here instead ;-)

Signed-off-by: Andre Przywara 
---
Hi,

will send a proper, merged v3a version of the patch separately.

Cheers,
Andre

 xen/arch/arm/vgic/vgic-v2.c | 43 ++-
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/xen/arch/arm/vgic/vgic-v2.c b/xen/arch/arm/vgic/vgic-v2.c
index 5516a8534f..3424a4a66f 100644
--- a/xen/arch/arm/vgic/vgic-v2.c
+++ b/xen/arch/arm/vgic/vgic-v2.c
@@ -43,6 +43,8 @@ void vgic_v2_setup_hw(paddr_t dbase, paddr_t cbase, paddr_t 
csize,
 gic_v2_hw_data.csize = csize;
 gic_v2_hw_data.vbase = vbase;
 gic_v2_hw_data.aliased_offset = aliased_offset;
+
+printk("Using the new VGIC implementation.\n");
 }
 
 /*
@@ -69,6 +71,8 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
 struct gic_lr lr_val;
 uint32_t intid;
 struct vgic_irq *irq;
+struct irq_desc *desc = NULL;
+bool have_desc_lock = false;
 
 gic_hw_ops->read_lr(lr, _val);
 
@@ -88,18 +92,30 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
 intid = lr_val.virq;
 irq = vgic_get_irq(vcpu->domain, vcpu, intid);
 
-spin_lock_irqsave(>irq_lock, flags);
+local_irq_save(flags);
+spin_lock(>irq_lock);
+
+/* The locking order forces us to drop and re-take the locks here. */
+if ( irq->hw )
+{
+spin_unlock(>irq_lock);
+
+desc = irq_to_desc(irq->hwintid);
+spin_lock(>lock);
+spin_lock(>irq_lock);
+
+/* This h/w IRQ should still be assigned to the virtual IRQ. */
+ASSERT(irq->hw && desc->irq == irq->hwintid);
+
+have_desc_lock = true;
+}
 
 /*
  * If a hardware mapped IRQ has been handled for good, we need to
  * clear the _IRQ_INPROGRESS bit to allow handling of new IRQs.
  */
 if ( irq->hw && !lr_val.active && !lr_val.pending )
-{
-struct irq_desc *irqd = irq_to_desc(irq->hwintid);
-
-clear_bit(_IRQ_INPROGRESS, >status);
-}
+clear_bit(_IRQ_INPROGRESS, >status);
 
 /* Always preserve the active bit */
 irq->active = lr_val.active;
@@ -132,18 +148,19 @@ void vgic_v2_fold_lr_state(struct vcpu *vcpu)
  */
 if ( vgic_irq_is_mapped_level(irq) && lr_val.pending )
 {
-struct irq_desc *irqd;
-
 ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
 
-irqd = irq_to_desc(irq->hwintid);
-irq->line_level = gic_read_pending_state(irqd);
+irq->line_level = gic_read_pending_state(desc);
 
 if ( !irq->line_level )
-gic_set_active_state(irqd, false);
+gic_set_active_state(desc, false);
 }
 
-spin_unlock_irqrestore(>irq_lock, flags);
+spin_unlock(>irq_lock);
+if ( have_desc_lock )
+spin_unlock(>lock);
+local_irq_restore(flags);
+
 vgic_put_irq(vcpu->domain, irq);
 }
 
@@ -184,7 +201,7 @@ void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq 
*irq, int lr)
 
 if ( vgic_irq_is_sgi(irq->intid) )
 {
-u32 src = ffs(irq->source);
+uint32_t src = ffs(irq->source);
 
 BUG_ON(!src);
 lr_val.virt.source = (src - 1);
-- 
2.14.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v3 14/39] ARM: new VGIC: Add GICv2 world switch backend

[Julien Grall]

Hi Andre,

On 03/21/2018 04:32 PM, Andre Przywara wrote:

+/*
+ * If a hardware mapped IRQ has been handled for good, we need to
+ * clear the _IRQ_INPROGRESS bit to allow handling of new IRQs.
+ */
+if ( irq->hw && !lr_val.active && !lr_val.pending )
+{
+struct irq_desc *irqd = irq_to_desc(irq->hwintid);
+
+clear_bit(_IRQ_INPROGRESS, >status);


I realize the current vGIC is doing exactly the same thing. But this is 
racy.


Imagine the interrupt is firing on another pCPU (I wasn't able to rule 
out this even when the interrupt is following the vCPU), that pCPU may 
set _IRQ_INPROGRESS before this is cleared here.


So you would end up clearing _IRQ_INPROGRESS here, making the desc state 
inconsistent and would affect the removal of the IRQ from a guest (see 
gic_remove_irq_from_guest).


I am not entirely sure how to fix this because taking the desc->lock 
would not be enough. Indeed you may end up to clear right after the flag 
was set in do_IRQ.


Because the race is already there in the current vGIC and only affecting 
release IRQ, I guess I would be ok to keep like that for now. Can you 
add a TODO?



+}
+
+/* Always preserve the active bit */
+irq->active = lr_val.active;
+
+/* Edge is the only case where we preserve the pending bit */
+if ( irq->config == VGIC_CONFIG_EDGE && lr_val.pending )
+{
+irq->pending_latch = true;
+
+if ( vgic_irq_is_sgi(intid) )
+irq->source |= (1U << lr_val.virt.source);
+}
+
+/* Clear soft pending state when level irqs have been acked. */
+if ( irq->config == VGIC_CONFIG_LEVEL && !lr_val.pending )
+irq->pending_latch = false;
+
+/*
+ * Level-triggered mapped IRQs are special because we only
+ * observe rising edges as input to the VGIC.
+ *
+ * If the guest never acked the interrupt we have to sample
+ * the physical line and set the line level, because the
+ * device state could have changed or we simply need to
+ * process the still pending interrupt later.
+ *
+ * If this causes us to lower the level, we have to also clear
+ * the physical active state, since we will otherwise never be
+ * told when the interrupt becomes asserted again.
+ */
+if ( vgic_irq_is_mapped_level(irq) && lr_val.pending )
+{
+struct irq_desc *irqd;
+
+ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+irqd = irq_to_desc(irq->hwintid);
+irq->line_level = gic_read_pending_state(irqd);
+
+if ( !irq->line_level )
+gic_set_active_state(irqd, false);


Sorry, I didn't notice it before. gic_set_active_state expect the 
desc->lock to be taken. But I can't see the code here to do that.


Cheers,

--
Julien Grall

___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel