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

Re: [Xen-devel] [PATCH v4 09/27] ARM: GICv3: introduce separate pending_irq structs for LPIs



Hi Andre,

On 03/04/17 21:28, Andre Przywara wrote:
For the same reason that allocating a struct irq_desc for each
possible LPI is not an option, having a struct pending_irq for each LPI
is also not feasible. We only care about mapped LPIs, so we can get away
with having struct pending_irq's only for them.
Maintain a radix tree per domain where we drop the pointer to the
respective pending_irq. The index used is the virtual LPI number.
The memory for the actual structures has been allocated already per
device at device mapping time.
Teach the existing VGIC functions to find the right pointer when being
given a virtual LPI number.
We also take care of checking for a NULL pointer in the VCPU exit path,
should an LPI have been removed from the tree for any reason.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
 xen/arch/arm/gic.c           | 12 ++++++++++++
 xen/arch/arm/vgic-v3.c       | 21 +++++++++++++++++++++
 xen/arch/arm/vgic.c          |  5 +++++
 xen/include/asm-arm/domain.h |  2 ++
 xen/include/asm-arm/vgic.h   |  1 +
 5 files changed, 41 insertions(+)

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index 9522c6c..3ed6f81 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -461,7 +461,19 @@ static void gic_update_one_lr(struct vcpu *v, int i)

     gic_hw_ops->read_lr(i, &lr_val);
     irq = lr_val.virq;
+

Spurious change.

     p = irq_to_pending(v, irq);
+    /* An LPI might have been unmapped, in which case we just clean up here. */
+    if ( !p )

I cannot find any code removing a pending_irq from the radix tree, so it is hard to know how p will not be freed whilst in use here. Is there any lock?

+    {
+        ASSERT(is_lpi(irq));
+
+        gic_hw_ops->clear_lr(i);
+        clear_bit(i, &this_cpu(lr_mask));
+
+        return;
+    }
+
     if ( lr_val.state & GICH_LR_ACTIVE )
     {
         set_bit(GIC_IRQ_GUEST_ACTIVE, &p->status);
diff --git a/xen/arch/arm/vgic-v3.c b/xen/arch/arm/vgic-v3.c
index 3c7161b..95fa0ba 100644
--- a/xen/arch/arm/vgic-v3.c
+++ b/xen/arch/arm/vgic-v3.c
@@ -330,6 +330,23 @@ read_unknown:
     return 1;
 }

+/*
+ * Looks up a virtual LPI number in our tree of mapped LPIs. This will return
+ * the corresponding struct pending_irq, which we also use to store the
+ * enabled and pending bit plus the priority.
+ * Returns NULL if an LPI cannot be found.

See my question above regarding the behavior of this function. It is not clear to me what is protecting the pending_irq to be freed whilst in use.

+ */
+struct pending_irq *lpi_to_pending(struct domain *d, unsigned int lpi)

This function ought to be a vgic_ops callback. I will not say it again.

+{
+    struct pending_irq *pirq;
+
+    read_lock(&d->arch.vgic.pend_lpi_tree_lock);
+    pirq = radix_tree_lookup(&d->arch.vgic.pend_lpi_tree, lpi);
+    read_unlock(&d->arch.vgic.pend_lpi_tree_lock);
+
+    return pirq;
+}
+
 static int __vgic_v3_rdistr_rd_mmio_write(struct vcpu *v, mmio_info_t *info,
                                           uint32_t gicr_reg,
                                           register_t r)
@@ -1452,6 +1469,9 @@ static int vgic_v3_domain_init(struct domain *d)
     spin_lock_init(&d->arch.vgic.its_devices_lock);
     d->arch.vgic.its_devices = RB_ROOT;

+    rwlock_init(&d->arch.vgic.pend_lpi_tree_lock);
+    radix_tree_init(&d->arch.vgic.pend_lpi_tree);
+
     /*
      * Domain 0 gets the hardware address.
      * Guests get the virtual platform layout.
@@ -1525,6 +1545,7 @@ static int vgic_v3_domain_init(struct domain *d)
 static void vgic_v3_domain_free(struct domain *d)
 {
     gicv3_its_unmap_all_devices(d);
+    radix_tree_destroy(&d->arch.vgic.pend_lpi_tree, NULL);

You may have thousands LPIs mapped, how long will you expect this to take?

     xfree(d->arch.vgic.rdist_regions);
 }

diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
index 67d75a6..28f6f66 100644
--- a/xen/arch/arm/vgic.c
+++ b/xen/arch/arm/vgic.c
@@ -431,10 +431,15 @@ bool vgic_to_sgi(struct vcpu *v, register_t sgir, enum 
gic_sgi_mode irqmode,
 struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq)
 {
     struct pending_irq *n;
+

Spurious change.

     /* Pending irqs allocation strategy: the first vgic.nr_spis irqs
      * are used for SPIs; the rests are used for per cpu irqs */
     if ( irq < 32 )
         n = &v->arch.vgic.pending_irqs[irq];
+#ifdef CONFIG_HAS_ITS

I really don't want to see #ifdef CONFIG_HAS_ITS nor vGIC specific function called directly from the common code. Please introduce proper vgic_ops.

+    else if ( is_lpi(irq) )
+        n = lpi_to_pending(v->domain, irq);
+#endif
     else
         n = &v->domain->arch.vgic.pending_irqs[irq - 32];
     return n;
diff --git a/xen/include/asm-arm/domain.h b/xen/include/asm-arm/domain.h
index 503a3cf..6ee7538 100644
--- a/xen/include/asm-arm/domain.h
+++ b/xen/include/asm-arm/domain.h
@@ -111,6 +111,8 @@ struct arch_domain
         uint32_t rdist_stride;              /* Re-Distributor stride */
         struct rb_root its_devices;         /* Devices mapped to an ITS */
         spinlock_t its_devices_lock;        /* Protects the its_devices tree */
+        struct radix_tree_root pend_lpi_tree; /* Stores struct pending_irq's */
+        rwlock_t pend_lpi_tree_lock;        /* Protects the pend_lpi_tree */
 #endif
     } vgic;

diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
index 894c3f1..69ef160 100644
--- a/xen/include/asm-arm/vgic.h
+++ b/xen/include/asm-arm/vgic.h
@@ -300,6 +300,7 @@ extern void vgic_vcpu_inject_spi(struct domain *d, unsigned 
int virq);
 extern void vgic_clear_pending_irqs(struct vcpu *v);
 extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
 extern struct pending_irq *spi_to_pending(struct domain *d, unsigned int irq);
+extern struct pending_irq *lpi_to_pending(struct domain *d, unsigned int irq);
 extern struct vgic_irq_rank *vgic_rank_offset(struct vcpu *v, int b, int n, 
int s);
 extern struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq);
 extern bool vgic_emulate(struct cpu_user_regs *regs, union hsr hsr);


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