[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [RFC PATCH v2 09/22] ARM: vITS: protect LPI priority update with pending_irq lock



Hi Andre,

On 21/07/17 20:59, Andre Przywara wrote:
As the priority value is now officially a member of struct pending_irq,
we need to take its lock when manipulating it via ITS commands.
Make sure we take the IRQ lock after the VCPU lock when we need both.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/vgic-v3-its.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
index 66095d4..705708a 100644
--- a/xen/arch/arm/vgic-v3-its.c
+++ b/xen/arch/arm/vgic-v3-its.c
@@ -402,6 +402,7 @@ static int update_lpi_property(struct domain *d, struct 
pending_irq *p)
     uint8_t property;
     int ret;

+    ASSERT(spin_is_locked(&p->lock));
     /*
      * If no redistributor has its LPIs enabled yet, we can't access the
      * property table. In this case we just can't update the properties,
@@ -419,7 +420,7 @@ static int update_lpi_property(struct domain *d, struct 
pending_irq *p)
     if ( ret )
         return ret;

-    write_atomic(&p->priority, property & LPI_PROP_PRIO_MASK);
+    p->priority = property & LPI_PROP_PRIO_MASK;

     if ( property & LPI_PROP_ENABLED )
         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
@@ -457,7 +458,7 @@ static int its_handle_inv(struct virt_its *its, uint64_t 
*cmdptr)
     uint32_t devid = its_cmd_get_deviceid(cmdptr);
     uint32_t eventid = its_cmd_get_id(cmdptr);
     struct pending_irq *p;
-    unsigned long flags;
+    unsigned long flags, vcpu_flags;

Same remark as patch #8 for the vcpu_flags and the locking.

     struct vcpu *vcpu;
     uint32_t vlpi;
     int ret = -1;
@@ -485,7 +486,8 @@ static int its_handle_inv(struct virt_its *its, uint64_t 
*cmdptr)
     if ( unlikely(!p) )
         goto out_unlock_its;

-    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
+    spin_lock_irqsave(&vcpu->arch.vgic.lock, vcpu_flags);
+    vgic_irq_lock(p, flags);

     /* Read the property table and update our cached status. */
     if ( update_lpi_property(d, p) )
@@ -497,7 +499,8 @@ static int its_handle_inv(struct virt_its *its, uint64_t 
*cmdptr)
     ret = 0;

 out_unlock:
-    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
+    vgic_irq_unlock(p, flags);
+    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, vcpu_flags);

 out_unlock_its:
     spin_unlock(&its->its_lock);
@@ -517,7 +520,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
*cmdptr)
     struct pending_irq *pirqs[16];
     uint64_t vlpi = 0;          /* 64-bit to catch overflows */
     unsigned int nr_lpis, i;
-    unsigned long flags;
+    unsigned long flags, vcpu_flags;
     int ret = 0;

     /*
@@ -542,7 +545,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
*cmdptr)
     vcpu = get_vcpu_from_collection(its, collid);
     spin_unlock(&its->its_lock);

-    spin_lock_irqsave(&vcpu->arch.vgic.lock, flags);
+    spin_lock_irqsave(&vcpu->arch.vgic.lock, vcpu_flags);
     read_lock(&its->d->arch.vgic.pend_lpi_tree_lock);

     do
@@ -555,9 +558,13 @@ static int its_handle_invall(struct virt_its *its, 
uint64_t *cmdptr)

         for ( i = 0; i < nr_lpis; i++ )
         {
+            vgic_irq_lock(pirqs[i], flags);
             /* We only care about LPIs on our VCPU. */
             if ( pirqs[i]->lpi_vcpu_id != vcpu->vcpu_id )
+            {
+                vgic_irq_unlock(pirqs[i], flags);

This locking does not seem to be related to the priority.

                 continue;
+            }

             vlpi = pirqs[i]->irq;
             /* If that fails for a single LPI, carry on to handle the rest. */
@@ -566,6 +573,8 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
*cmdptr)
                 update_lpi_vgic_status(vcpu, pirqs[i]);
             else
                 ret = err;
+
+            vgic_irq_unlock(pirqs[i], flags);
         }
     /*
      * Loop over the next gang of pending_irqs until we reached the end of
@@ -576,7 +585,7 @@ static int its_handle_invall(struct virt_its *its, uint64_t 
*cmdptr)
               (nr_lpis == ARRAY_SIZE(pirqs)) );

     read_unlock(&its->d->arch.vgic.pend_lpi_tree_lock);
-    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
+    spin_unlock_irqrestore(&vcpu->arch.vgic.lock, vcpu_flags);

     return ret;
 }
@@ -712,6 +721,7 @@ static int its_handle_mapti(struct virt_its *its, uint64_t 
*cmdptr)
     uint32_t intid = its_cmd_get_physical_id(cmdptr), _intid;
     uint16_t collid = its_cmd_get_collection(cmdptr);
     struct pending_irq *pirq;
+    unsigned long flags;
     struct vcpu *vcpu = NULL;
     int ret = -1;

@@ -765,7 +775,9 @@ static int its_handle_mapti(struct virt_its *its, uint64_t 
*cmdptr)
      * We don't need the VGIC VCPU lock here, because the pending_irq isn't
      * in the radix tree yet.
      */
+    vgic_irq_lock(pirq, flags);
     ret = update_lpi_property(its->d, pirq);
+    vgic_irq_unlock(pirq, flags);
     if ( ret )
         goto out_remove_host_entry;



Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.