[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 02/26/2018 04:48 PM, Andre Przywara wrote:
Hi,
On 23/02/18 18:14, Julien Grall wrote:
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?
Yes, indeed. But I think this points to the problem of this approach:
the virtual IRQ is tied to a VCPU, and we have to make sure that not
only the affinity is updated on a CPU migration (as we do for SPIs), but
actually the interrupt itself is changed: since CPU0/PPI9 has a
different irq_desc* from, say, CPU1/PPI9.
So there is more than just adding a parameter to irq_to_desc().
Change in the irq_to_desc() interface needs to be justify. The use case
I have in mind for PPI is the virtual timer. In that case, you will
always receive the PPI on the right pCPU.
Do you really see a use case where a vCPU is running on pCPU A but the
PPI is routed to pCPU B?
+ 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.
We will realise that is has disappeared when vgic_get_irq() called with
that virtual number returns NULL. The refcount is really just to know
when you can free dynamically allocated struct vgic_irqs, so it's
strictly about the *pointer* to the *memory*, not about the logical
entity of that particular virtual IRQ.
Actually it should not really happen that you end up with a hardware IRQ
still assigned to an abandoned virtual IRQ, as you would expect to free
that connection *before* disbanding the virtual IRQ.
So I am not entirely sure why the reference is not necessary here.
Typically to remove a virtual IRQ, you arrange for vgic_get_irq() to
return NULL on that number. Then you "wait" for the refcount to drop to
zero, at which point it's safe to free the memory allocated for that
vgic_irq. As mentioned, only really useful for LPIs, but it's a central
property of the new VGIC architecture, because we need to have those
gets/puts in virtually every function.
Thank you for the explanation.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel
|
