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

Re: [Xen-devel] [RFC PATCH 38/49] ARM: new VGIC: handle hardware mapped IRQs





On 23/02/18 18:02, Andre Przywara wrote:
Hi,

Hi Andre,

On 19/02/18 12:19, Julien Grall wrote:
Hi,

On 09/02/18 14:39, Andre Przywara wrote:
The VGIC supports virtual IRQs to be connected to a hardware IRQ, so
when a guest EOIs the virtual interrupt, it affects the state of that
corresponding interrupt on the hardware side at the same time.
Implement the interface that the Xen arch/core code expects to connect
the virtual and the physical world.

Signed-off-by: Andre Przywara <andre.przywara@xxxxxxxxxx>
---
   xen/arch/arm/vgic/vgic.c | 63
++++++++++++++++++++++++++++++++++++++++++++++++
   1 file changed, 63 insertions(+)

diff --git a/xen/arch/arm/vgic/vgic.c b/xen/arch/arm/vgic/vgic.c
index dc5e011fa3..8d5260a7db 100644
--- a/xen/arch/arm/vgic/vgic.c
+++ b/xen/arch/arm/vgic/vgic.c
@@ -693,6 +693,69 @@ void vgic_kick_vcpus(struct domain *d)
       }
   }
   +struct irq_desc *vgic_get_hw_irq_desc(struct domain *d, struct vcpu
*v,
+                                      unsigned int virq)
+{
+    struct irq_desc *desc = NULL;
+    struct vgic_irq *irq = vgic_get_irq(d, v, virq);
+    unsigned long flags;
+
+    if ( !irq )
+        return NULL;
+
+    spin_lock_irqsave(&irq->irq_lock, flags);
+    if ( irq->hw )
+        desc = irq_to_desc(irq->hwintid);

This is not going to work well for PPIs. We should consider to add at
least an ASSERT(...) in the code to prevent bad use of it.

Yeah, done. But I wonder if we eventually should extend the
irq_to_desc() function to take the vCPU, since we will need it anyway
once we use hardware mapped timer IRQs (PPIs) in the future. But this
should not be in this series, I guess.

irq_to_desc only deal with hardware interrupt, so you mean pCPU instead of vCPU?


+    spin_unlock_irqrestore(&irq->irq_lock, flags);
+
+    vgic_put_irq(d, irq);
+
+    return desc;
+}
+
+/*
+ * was:
+ *      int kvm_vgic_map_phys_irq(struct vcpu *vcpu, u32 virt_irq,
u32 phys_irq)
+ *      int kvm_vgic_unmap_phys_irq(struct vcpu *vcpu, unsigned int
virt_irq)
+ */
+int vgic_connect_hw_irq(struct domain *d, struct vcpu *vcpu,
+            unsigned int virt_irq, struct irq_desc *desc,
+            bool connect)

Indentation.

+{
+    struct vgic_irq *irq = vgic_get_irq(d, vcpu, virt_irq);
+    unsigned long flags;
+    int ret = 0;
+
+    if ( !irq )
+        return -EINVAL;
+
+    spin_lock_irqsave(&irq->irq_lock, flags);
+
+    if ( connect )                      /* assign a mapped IRQ */
+    {
+        /* The VIRQ should not be already enabled by the guest */
+        if ( !irq->hw && !irq->enabled )
+        {
+            irq->hw = true;
+            irq->hwintid = desc->irq;
+        }
+        else
+        {
+            ret = -EBUSY;
+        }

I know that it should not matter for SPIs today. But aren't you meant to
get a reference on that interrupt if you connect it?

No, the refcount feature is strictly for the pointer to the structure,
not for everything related to this virtual IRQ.
We store only the virtual IRQ number in the dev_id/info members, we will
get the struct vgic_irq pointer via the vIRQ number on do_IRQ().
Does that make sense?

But technically you "allocate" the virtual SPI at that time, right? So this would mean you need to get a reference, otherwise it might disappear.

So I am not entirely sure why the reference is not necessary here.

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