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

Re: [Xen-devel] [PATCH 37/57] ARM: new VGIC: Add ENABLE registers handlers



Hi Andre,

On 03/05/2018 04:03 PM, Andre Przywara wrote:
As the enable register handlers are shared between the v2 and v3
emulation, their implementation goes into vgic-mmio.c, to be easily
referenced from the v3 emulation as well later.
This introduces a vgic_sync_hardware_irq() function, which updates the
physical side of a hardware mapped virtual IRQ.
Because the existing locking order between vgic_irq->irq_lock and
irq_desc->lock dictates so, we dropu the irq_lock and retake them in the
proper order.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
---
Changelog RFC ... v1:
- extend and move vgic_sync_hardware_irq()
- do proper locking sequence
- skip already disabled/enabled IRQs

  xen/arch/arm/vgic/vgic-mmio-v2.c |   4 +-
  xen/arch/arm/vgic/vgic-mmio.c    | 117 +++++++++++++++++++++++++++++++++++++++
  xen/arch/arm/vgic/vgic-mmio.h    |  11 ++++
  xen/arch/arm/vgic/vgic.c         |  38 +++++++++++++
  xen/arch/arm/vgic/vgic.h         |   3 +
  5 files changed, 171 insertions(+), 2 deletions(-)

diff --git a/xen/arch/arm/vgic/vgic-mmio-v2.c b/xen/arch/arm/vgic/vgic-mmio-v2.c
index 2e015ed0b1..3dd983f885 100644
--- a/xen/arch/arm/vgic/vgic-mmio-v2.c
+++ b/xen/arch/arm/vgic/vgic-mmio-v2.c
@@ -80,10 +80,10 @@ static const struct vgic_register_region 
vgic_v2_dist_registers[] = {
          vgic_mmio_read_rao, vgic_mmio_write_wi, 1,
          VGIC_ACCESS_32bit),
      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_senable, 1,
          VGIC_ACCESS_32bit),
      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ICENABLER,
-        vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
+        vgic_mmio_read_enable, vgic_mmio_write_cenable, 1,
          VGIC_ACCESS_32bit),
      REGISTER_DESC_WITH_BITS_PER_IRQ(GICD_ISPENDR,
          vgic_mmio_read_raz, vgic_mmio_write_wi, 1,
diff --git a/xen/arch/arm/vgic/vgic-mmio.c b/xen/arch/arm/vgic/vgic-mmio.c
index 284a92d288..f8f0252eff 100644
--- a/xen/arch/arm/vgic/vgic-mmio.c
+++ b/xen/arch/arm/vgic/vgic-mmio.c
@@ -39,6 +39,123 @@ void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
      /* Ignore */
  }
+/*
+ * Read accesses to both GICD_ICENABLER and GICD_ISENABLER return the value
+ * of the enabled bit, so there is only one function for both here.
+ */
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+                                    paddr_t addr, unsigned int len)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    uint32_t value = 0;
+    unsigned int i;
+
+    /* Loop over all IRQs affected by this read */
+    for ( i = 0; i < len * 8; i++ )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+
+        if ( irq->enabled )
+            value |= (1U << i);
+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+
+    return value;
+}
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+                             paddr_t addr, unsigned int len,
+                             unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    unsigned int i;
+
+    for_each_set_bit( i, &val, len * 8 )
+    {
+        struct vgic_irq *irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+        unsigned long flags;
+        irq_desc_t *desc;
+
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        if ( irq->enabled )            /* skip already enabled IRQs */
+        {
+            spin_unlock_irqrestore(&irq->irq_lock, flags);
+            vgic_put_irq(vcpu->domain, irq);
+            continue;
+        }
+
+        irq->enabled = true;
+        if ( irq->hw )
+        {
+            /*
+             * The irq cannot be a PPI, we only support delivery
+             * of SPIs to guests.
+             */
+            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+            desc = irq_to_desc(irq->hwintid);
+        }
+        else
+            desc = NULL;

You could just initialize desc to NULL at the declaration time and drop the else part.

+
+        vgic_queue_irq_unlock(vcpu->domain, irq, flags);
+
+        if ( desc )
+            vgic_sync_hardware_irq(vcpu->domain, desc, irq);

A comment explaining why desc is done outside the locking would be useful. This would avoid to loose time using git blame.

+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+                 paddr_t addr, unsigned int len,
+                 unsigned long val)
+{
+    uint32_t intid = VGIC_ADDR_TO_INTID(addr, 1);
+    unsigned int i;
+
+    for_each_set_bit( i, &val, len * 8 )
+    {
+        struct vgic_irq *irq;
+        unsigned long flags;
+        irq_desc_t *desc;
+
+        irq = vgic_get_irq(vcpu->domain, vcpu, intid + i);
+        spin_lock_irqsave(&irq->irq_lock, flags);
+
+        if ( !irq->enabled )            /* skip already disabled IRQs */
+        {
+            spin_unlock_irqrestore(&irq->irq_lock, flags);
+            vgic_put_irq(vcpu->domain, irq);
+            continue;
+        }
+
+        irq->enabled = false;
+
+        if ( irq->hw )
+        {
+            /*
+             * The irq cannot be a PPI, we only support delivery
+             * of SPIs to guests.
+             */
+            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+            desc = irq_to_desc(irq->hwintid);
+        }
+        else
+            desc = NULL;
+
+        spin_unlock_irqrestore(&irq->irq_lock, flags);
+
+        if ( desc )
+            vgic_sync_hardware_irq(vcpu->domain, desc, irq);

Ditto.

+
+        vgic_put_irq(vcpu->domain, irq);
+    }
+}
+
  static int match_region(const void *key, const void *elt)
  {
      const unsigned int offset = (unsigned long)key;
diff --git a/xen/arch/arm/vgic/vgic-mmio.h b/xen/arch/arm/vgic/vgic-mmio.h
index 621b9a281c..2ddcbbf58d 100644
--- a/xen/arch/arm/vgic/vgic-mmio.h
+++ b/xen/arch/arm/vgic/vgic-mmio.h
@@ -96,6 +96,17 @@ unsigned long vgic_mmio_read_rao(struct vcpu *vcpu,
  void vgic_mmio_write_wi(struct vcpu *vcpu, paddr_t addr,
                          unsigned int len, unsigned long val);
+unsigned long vgic_mmio_read_enable(struct vcpu *vcpu,
+                                    paddr_t addr, unsigned int len);
+
+void vgic_mmio_write_senable(struct vcpu *vcpu,
+                             paddr_t addr, unsigned int len,
+                             unsigned long val);
+
+void vgic_mmio_write_cenable(struct vcpu *vcpu,
+                             paddr_t addr, unsigned int len,
+                             unsigned long val);
+
  unsigned int vgic_v2_init_dist_iodev(struct vgic_io_device *dev);
#endif
diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index 465a95f415..5246d7c2e7 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -698,6 +698,44 @@ void vgic_kick_vcpus(struct domain *d)
      }
  }
+static unsigned int translate_irq_type(bool is_level)
+{
+    return is_level ? IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
+}
+
+void vgic_sync_hardware_irq(struct domain *d,
+                            irq_desc_t *desc, struct vgic_irq *irq)
+{
+    unsigned long flags;
+
+    spin_lock_irqsave(&desc->lock, flags);
+    spin_lock(&irq->irq_lock);
+
+    /* Is that association actually still valid? (we entered with no locks) */

If the association is not valid, then you need to fetch the new desc. Right?

+    if ( desc->irq == irq->hwintid )
+    {
+        if ( irq->enabled )
+        {
+            /*
+             * We might end up from various callers, so check that the
+             * interrrupt is disabled before trying to change the config.
+             */
+            if ( irq_type_set_by_domain(d) &&
+                 test_bit(_IRQ_DISABLED, &desc->status) )
+                gic_set_irq_type(desc, translate_irq_type(irq->config));
+
+            if ( irq->target_vcpu )
+                irq_set_affinity(desc, 
cpumask_of(irq->target_vcpu->processor));
+            desc->handler->enable(desc);
+        }
+        else
+            desc->handler->disable(desc);
+    }
+
+    spin_unlock(&irq->irq_lock);
+    spin_unlock_irqrestore(&desc->lock, flags);
+}
+
  /*
   * Local variables:
   * mode: C
diff --git a/xen/arch/arm/vgic/vgic.h b/xen/arch/arm/vgic/vgic.h
index 588bd067b7..68e205d10a 100644
--- a/xen/arch/arm/vgic/vgic.h
+++ b/xen/arch/arm/vgic/vgic.h
@@ -50,6 +50,9 @@ static inline void vgic_get_irq_kref(struct vgic_irq *irq)
      atomic_inc(&irq->refcount);
  }
+void vgic_sync_hardware_irq(struct domain *d,
+                            irq_desc_t *desc, struct vgic_irq *irq);
+
  void vgic_v2_fold_lr_state(struct vcpu *vcpu);
  void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq, int lr);
  void vgic_v2_set_underflow(struct vcpu *vcpu);


Cheers,

--
Julien Grall

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

 


Rackspace

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