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

Re: [Xen-devel] [RFC PATCH v2 14/22] ARM: vGIC: move virtual IRQ configuration from rank to pending_irq



Hi Andre,

On 21/07/17 21:00, Andre Przywara wrote:
The IRQ configuration (level or edge triggered) for a group of IRQs
are still stored in the irq_rank structure.
Introduce a new bit called GIC_IRQ_GUEST_LEVEL in the "status" field,
which holds that information.
Remove the storage from the irq_rank and use the existing wrappers to
store and retrieve the configuration bit for multiple IRQs.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/vgic-v2.c     | 21 +++---------
 xen/arch/arm/vgic-v3.c     | 25 ++++----------
 xen/arch/arm/vgic.c        | 81 +++++++++++++++++++++++++++++++++-------------
 xen/include/asm-arm/vgic.h |  5 ++-
 4 files changed, 73 insertions(+), 59 deletions(-)

diff --git a/xen/arch/arm/vgic-v2.c b/xen/arch/arm/vgic-v2.c
index a3fd500..0c8a598 100644
--- a/xen/arch/arm/vgic-v2.c
+++ b/xen/arch/arm/vgic-v2.c
@@ -278,20 +278,12 @@ static int vgic_v2_distr_mmio_read(struct vcpu *v, 
mmio_info_t *info,
         goto read_reserved;

     case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
-    {
-        uint32_t icfgr;
-
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
-        if ( rank == NULL) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        icfgr = rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR, 
DABT_WORD)];
-        vgic_unlock_rank(v, rank, flags);

-        *r = vreg_reg32_extract(icfgr, info);
+        irq = (gicd_reg - GICD_ICFGR) * 4;
+        *r = vgic_fetch_irq_config(v, irq);

         return 1;
-    }

     case VRANGE32(0xD00, 0xDFC):
         goto read_impl_defined;
@@ -529,13 +521,8 @@ static int vgic_v2_distr_mmio_write(struct vcpu *v, 
mmio_info_t *info,

     case VRANGE32(GICD_ICFGR2, GICD_ICFGRN): /* SPIs */
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 2, gicd_reg - GICD_ICFGR, DABT_WORD);
-        if ( rank == NULL) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, gicd_reg - GICD_ICFGR,
-                                                     DABT_WORD)],
-                          r, info);
-        vgic_unlock_rank(v, rank, flags);
+        irq = (gicd_reg - GICD_ICFGR) * 4; /* 2 bit per IRQ */

s/bit/bits/

+        vgic_store_irq_config(v, irq, r);
         return 1;

     case VRANGE32(0xD00, 0xDFC):
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index d3356ae..e9e36eb 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -722,20 +722,11 @@ static int __vgic_v3_distr_common_mmio_read(const char 
*name, struct vcpu *v,
         return 1;

     case VRANGE32(GICD_ICFGR, GICD_ICFGRN):
-    {
-        uint32_t icfgr;
-
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
-        if ( rank == NULL ) goto read_as_zero;
-        vgic_lock_rank(v, rank, flags);
-        icfgr = rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR, DABT_WORD)];
-        vgic_unlock_rank(v, rank, flags);
-
-        *r = vreg_reg32_extract(icfgr, info);
-
+        irq = (reg - GICD_ICFGR) * 4;
+        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto read_as_zero;
+        *r = vgic_fetch_irq_config(v, irq);
         return 1;
-    }

     default:
         printk(XENLOG_G_ERR
@@ -834,13 +825,9 @@ static int __vgic_v3_distr_common_mmio_write(const char 
*name, struct vcpu *v,
         /* ICFGR1 for PPI's, which is implementation defined
            if ICFGR1 is programmable or not. We chose to program */
         if ( dabt.size != DABT_WORD ) goto bad_width;
-        rank = vgic_rank_offset(v, 2, reg - GICD_ICFGR, DABT_WORD);
-        if ( rank == NULL ) goto write_ignore;
-        vgic_lock_rank(v, rank, flags);
-        vreg_reg32_update(&rank->icfg[REG_RANK_INDEX(2, reg - GICD_ICFGR,
-                                                     DABT_WORD)],
-                          r, info);
-        vgic_unlock_rank(v, rank, flags);
+        irq = (reg - GICD_ICFGR) * 4;
+        if ( irq >= v->domain->arch.vgic.nr_spis + 32 ) goto write_ignore;
+        vgic_store_irq_config(v, irq, r);
         return 1;

     default:
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index ddcd99b..e5a4765 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -268,6 +268,55 @@ void vgic_store_irq_priority(struct vcpu *v, unsigned int 
nrirqs,
     local_irq_restore(flags);
 }

+#define IRQS_PER_CFGR   16
+/**
+ * vgic_fetch_irq_config: assemble the configuration bits for a group of 16 
IRQs
+ * @v: the VCPU for private IRQs, any VCPU of a domain for SPIs
+ * @first_irq: the first IRQ to be queried, must be aligned to 16
+ */
+uint32_t vgic_fetch_irq_config(struct vcpu *v, unsigned int first_irq)
+{
+    struct pending_irq *pirqs[IRQS_PER_CFGR];
+    unsigned long flags;
+    uint32_t ret = 0, i;
+
+    local_irq_save(flags);
+    vgic_lock_irqs(v, IRQS_PER_CFGR, first_irq, pirqs);
+
+    for ( i = 0; i < IRQS_PER_CFGR; i++ )
+        if ( test_bit(GIC_IRQ_GUEST_LEVEL, &pirqs[i]->status) )
+            ret |= 1 << (i * 2);
+        else
+            ret |= 3 << (i * 2);
+
+    vgic_unlock_irqs(pirqs, IRQS_PER_CFGR);
+    local_irq_restore(flags);
+
+    return ret;
+}
+
+void vgic_store_irq_config(struct vcpu *v, unsigned int first_irq,
+                           uint32_t value)
+{
+    struct pending_irq *pirqs[IRQS_PER_CFGR];
+    unsigned long flags;
+    unsigned int i;
+
+    local_irq_save(flags);
+    vgic_lock_irqs(v, IRQS_PER_CFGR, first_irq, pirqs);
+
+    for ( i = 0; i < IRQS_PER_CFGR; i++, value >>= 2 )
+    {
+        if ( (value & 0x3) > 1 )
+            clear_bit(GIC_IRQ_GUEST_LEVEL, &pirqs[i]->status);
+        else
+            set_bit(GIC_IRQ_GUEST_LEVEL, &pirqs[i]->status);
+    }
+
+    vgic_unlock_irqs(pirqs, IRQS_PER_CFGR);
+    local_irq_restore(flags);
+}
+
 bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, unsigned int irq)
 {
     unsigned long flags;
@@ -384,22 +433,6 @@ void vgic_disable_irqs(struct vcpu *v, uint32_t r, int n)
     }
 }

-#define VGIC_ICFG_MASK(intr) (1 << ((2 * ((intr) % 16)) + 1))
-
-/* The function should be called with the rank lock taken */
-static inline unsigned int vgic_get_virq_type(struct vcpu *v, int n, int index)
-{
-    struct vgic_irq_rank *r = vgic_get_rank(v, n);
-    uint32_t tr = r->icfg[index >> 4];
-
-    ASSERT(spin_is_locked(&r->lock));
-
-    if ( tr & VGIC_ICFG_MASK(index) )
-        return IRQ_TYPE_EDGE_RISING;
-    else
-        return IRQ_TYPE_LEVEL_HIGH;
-}
-
 void vgic_lock_irqs(struct vcpu *v, unsigned int nrirqs,
                     unsigned int first_irq, struct pending_irq **pirqs)
 {
@@ -424,8 +457,8 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
 {
     const unsigned long mask = r;
     struct pending_irq *p;
-    unsigned int irq;
-    unsigned long flags;
+    unsigned int irq, int_type;
+    unsigned long flags, vcpu_flags;
     int i = 0;
     struct vcpu *v_target;
     struct domain *d = v->domain;
@@ -436,23 +469,27 @@ void vgic_enable_irqs(struct vcpu *v, uint32_t r, int n)
     while ( (i = find_next_bit(&mask, 32, i)) < 32 ) {
         irq = i + (32 * n);
         v_target = vgic_get_target_vcpu(v, irq);
-        spin_lock_irqsave(&v_target->arch.vgic.lock, flags);
+        spin_lock_irqsave(&v_target->arch.vgic.lock, vcpu_flags);

Same my comments about the flags in previous patches.

         p = irq_to_pending(v_target, irq);
+        vgic_irq_lock(p, flags);

Same as before about the pending_lock. You need to explain why it protects much more than the configuration.

         set_bit(GIC_IRQ_GUEST_ENABLED, &p->status);
+        int_type = test_bit(GIC_IRQ_GUEST_LEVEL, &p->status) ?
+                            IRQ_TYPE_LEVEL_HIGH : IRQ_TYPE_EDGE_RISING;
         if ( !list_empty(&p->inflight) && !test_bit(GIC_IRQ_GUEST_VISIBLE, 
&p->status) )
             gic_raise_guest_irq(v_target, irq, p->cur_priority);
-        spin_unlock_irqrestore(&v_target->arch.vgic.lock, flags);
+        vgic_irq_unlock(p, flags);
+        spin_unlock_irqrestore(&v_target->arch.vgic.lock, vcpu_flags);
         if ( p->desc != NULL )
         {
-            irq_set_affinity(p->desc, cpumask_of(v_target->processor));
             spin_lock_irqsave(&p->desc->lock, flags);
+            irq_set_affinity(p->desc, cpumask_of(v_target->processor));

Why this is moved?

             /*
              * The irq cannot be a PPI, we only support delivery of SPIs
              * to guests.
              */
             ASSERT(irq >= 32);
             if ( irq_type_set_by_domain(d) )
-                gic_set_irq_type(p->desc, vgic_get_virq_type(v, n, i));
+                gic_set_irq_type(p->desc, int_type);
             p->desc->handler->enable(p->desc);
             spin_unlock_irqrestore(&p->desc->lock, flags);
         }
diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 6343c95..14c22b2 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -73,6 +73,7 @@ struct pending_irq
 #define GIC_IRQ_GUEST_ENABLED  3
 #define GIC_IRQ_GUEST_MIGRATING   4
 #define GIC_IRQ_GUEST_PRISTINE_LPI  5
+#define GIC_IRQ_GUEST_LEVEL    6
     unsigned long status;
     struct irq_desc *desc; /* only set it the irq corresponds to a physical 
irq */
     unsigned int irq;
@@ -110,7 +111,6 @@ struct vgic_irq_rank {
     uint8_t index;

     uint32_t ienable;
-    uint32_t icfg[2];

     /*
      * It's more convenient to store a target VCPU per vIRQ
@@ -191,6 +191,9 @@ uint32_t vgic_fetch_irq_priority(struct vcpu *v, unsigned 
int nrirqs,
                                  unsigned int first_irq);
 void vgic_store_irq_priority(struct vcpu *v, unsigned int nrirqs,
                              unsigned int first_irq, uint32_t reg);
+uint32_t vgic_fetch_irq_config(struct vcpu *v, unsigned int first_irq);
+void vgic_store_irq_config(struct vcpu *v, unsigned int first_irq,
+                           uint32_t reg);

 enum gic_sgi_mode;



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®.