[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v9 20/28] ARM: GICv3: handle unmapped LPIs
Hi Stefano,
On 05/23/2017 07:23 PM, Stefano Stabellini wrote:
On Tue, 23 May 2017, Julien Grall wrote:
Hi Stefano,
On 23/05/17 00:48, Stefano Stabellini wrote:
On Fri, 19 May 2017, Stefano Stabellini wrote:
On Thu, 11 May 2017, Andre Przywara wrote:
When LPIs get unmapped by a guest, they might still be in some LR of
some VCPU. Nevertheless we remove the corresponding pending_irq
(possibly freeing it), and detect this case (irq_to_pending() returns
NULL) when the LR gets cleaned up later.
However a *new* LPI may get mapped with the same number while the old
LPI is *still* in some LR. To avoid getting the wrong state, we mark
every newly mapped LPI as PRISTINE, which means: has never been in an
LR before. If we detect the LPI in an LR anyway, it must have been an
older one, which we can simply retire.
Before inserting such a PRISTINE LPI into an LR, we must make sure that
it's not already in another LR, as the architecture forbids two
interrupts with the same virtual IRQ number on one CPU.
Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
---
xen/arch/arm/gic.c | 55
+++++++++++++++++++++++++++++++++++++++++-----
xen/include/asm-arm/vgic.h | 6 +++++
2 files changed, 56 insertions(+), 5 deletions(-)
diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index fd3fa05..8bf0578 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -375,6 +375,8 @@ static inline void gic_set_lr(int lr, struct
pending_irq *p,
{
ASSERT(!local_irq_is_enabled());
+ clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status);
+
gic_hw_ops->update_lr(lr, p, state);
set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
@@ -442,12 +444,41 @@ void gic_raise_inflight_irq(struct vcpu *v,
unsigned int virtual_irq)
#endif
}
+/*
+ * Find an unused LR to insert an IRQ into. If this new interrupt is a
+ * PRISTINE LPI, scan the other LRs to avoid inserting the same IRQ
twice.
+ */
+static int gic_find_unused_lr(struct vcpu *v, struct pending_irq *p,
int lr)
+{
+ unsigned int nr_lrs = gic_hw_ops->info->nr_lrs;
+ unsigned long *lr_mask = (unsigned long *) &this_cpu(lr_mask);
+ struct gic_lr lr_val;
+
+ ASSERT(spin_is_locked(&v->arch.vgic.lock));
+
+ if ( test_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status) )
Maybe we should add an "unlikely".
I can see how this would be OKish at runtime, but at boot time there
might be a bunch of PRISTINE_LPIs, but no MAPDs have been issued yet,
right?
You cannot have any PRISTINE_LPIs without any MAPDs done. This bit will be set
when you do the first MAPTI.
I have a suggestion, I'll leave it to you and Julien if you want to do
this now, or maybe consider it as a TODO item. I am OK either way (I
don't want to delay the ITS any longer).
I am thinking we should do this scanning only after at least one MAPD
has been issued for a given cpu at least once. I would resurrect the
idea of a DISCARD flag, but not on the pending_irq, that I believe it's
difficult to handle, but a single global DISCARD flag per struct vcpu.
On MAPD, we set DISCARD for the target vcpu of the LPI we are dropping.
Next time we want to inject a PRISTINE_IRQ on that cpu interface, we
scan all LRs for interrupts with a NULL pending_irq. We remove those
from LRs, then we remove the DISCARD flag.
Do you think it would work?
I don't understand the point to do that. Ok, you will get the first
PRISTINE_LPI "fast" (though likely LRs will be empty), but all the other will
be "slow" (though likely LRs will be empty too).
The pain to implement your suggestion does not seem to be worth it so far.
Let me explain it a bit better, I think I didn't clarify it well enough.
Let me also premise, that this would be fine to do later, it doesn't
have to be part of this patch.
When I wrote MAPD above, I meant actually any commands that delete an
existing pending_irq - vLPI mapping. Specifically, DISCARD, and MAPD
when the
if ( !valid )
/* Discard all events and remove pending LPIs. */
its_unmap_device(its, devid);
code path is taken, which should not be the case at boot time, right?
Are there any other commands that remove a pending_irq - vLPI mapping
that I missed?
I don't think so.
The idea is that we could add a VGIC_V3_LPIS_DISCARD flag to arch_vcpu.
VGIC_V3_LPIS_DISCARD is set on a DISCARD command, and on a MAPD (!valid)
command. If VGIC_V3_LPIS_DISCARD is not set, there is no need to scan
anything. If VGIC_V3_LPIS_DISCARD is set *and* we want to inject a
PRISTINE_IRQ, then we do the scanning.
When we do the scanning, we check all LRs for NULL pending_irq structs.
We clear them all in one go. Then we remove VGIC_V3_LPIS_DISCARD.
The problem we are trying to solve here is not because of NULL
pending_irq structs. It is because of the previous interrupt may still
be in LRs so we would end-up to have twice the LPIs in it. This will
result to unpredictable behavior.
This could happen if you do:
vCPU A | vCPU B
|
DISCARD vLPI1 |
MAPTI vLPI1 |
interrupt injected on vCPU B
| entering in the hyp
| clear_lrs
| vgic_vcpu_inject_irq
clear_lrs will not remove the interrupt from LRs if it was already
pending because pending_irq is not NULL anymore.
This is causing issue because we are trying to be clever with the LRs by
not regenerating them at every entry/exit. This is causing trouble in
many more place in the vGIC. IHMO we should attempt to regenerate them
and see how this will affect the performance.
This way, we get all PRISTINE_LPI fast, except for the very first one
after a DISCARD or MAPD (!valid) command.
Does it make more sense now? What do you think?
To be honest, I think we are trying to think about premature
optimization without any number. We should first look at the vGIC rework
and then see if this code will stay in place (I have the feeling it will
disappear).
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
