Hi Marc,
Thanks for your feedback. Please see my comments below.
On 2019/1/22 17:53, Marc Zyngier wrote:
Hi Heyi,
On Mon, 21 Jan 2019 11:51:48 +0000,
Heyi Guo <guoh...@huawei.com> wrote:
Every VLPI will temporarily be mapped to the first CPU in system
(normally CPU0) and then moved to the real scheduled CPU later. There
is a time window so a VLPI may be sent to CPU0 instead of the real
scheduled vCPU, in a multi-CPU virtual machine. However, CPU0 may have
not been scheduled as a virtual CPU after system boots up, so the
value of GICR_VPROPBASER still be the reset value. According to GIC
spec, the reset value of IDbits in GICR_VPROPBASER is architecturally
UNKNOWN, and the GIC will behave as if all virtual LPIs are out of
range if it is less than 0b1101. On our platform the GICR will simply
drop the incoming VLPI, which results in interrupt missing in Guest.
OK, it took me some time to page this horror back in. So let's see if
I can sum-up the issue correctly:
Sorry for not explaining the whole thing clearly...
- When a VM gets created, all the vPEs are mapped to CPU0's
redistributor.
Not exactly on VM geting created, but when the passthru PCI device driver in
Guest tries to enable MSI interrupts. The specific code is in its_map_vm().
- If a device starts emitting VLPIs targeting a vPE that has not run
yet, these VLPIs are forwarded to CPU0's redistributor.
- If CPU0 has itself never run any vPE, its GICR_PROPBASER is not
initialised, meaning that the IDbits field may contain a value that
makes the redistributor drop the interrupt on the floor.
Yes.
Is that a correct assessment of the issue you're seeing? If so, I
think you have a very good point here, and this looks like a hole in
the driver.
Comments below:
As no code will clear GICR_VPROPBASER at runtime, we can safely
initialize the IDbits field at boot time for each CPU to get rid of
this issue.
Signed-off-by: Heyi Guo <guoh...@huawei.com>
Signed-off-by: Heyi Guo <heyi....@linaro.org>
---
drivers/irqchip/irq-gic-v3-its.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index db20e99..6116215 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2144,6 +2144,20 @@ static void its_cpu_init_lpis(void)
val |= GICR_CTLR_ENABLE_LPIS;
writel_relaxed(val, rbase + GICR_CTLR);
+ /*
+ * Temporary workaround for vlpi drop on Hi1620.
Why is this specific to this implementation? Isn't this an issue that
affects every GICv4 implementations?
This was an internal patch and I forgot to modify the comment before sending
out, either not 100% sure that it is the common behavior of GICv4 to drop VLPI
if IDbits is not correctly configured.
I can change it in V2.
+ * IDbits must be set before any VLPI is sent to this CPU, or else the
+ * VLPI will be considered as out of range and dropped.
+ */
+ if (gic_rdists->has_vlpis) {
+ void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
+
+ val = (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
+ pr_info("GICv4: CPU%d: Init IDbits to 0x%llx for
GICR_VPROPBASER\n",
+ smp_processor_id(), val);
I don't think this pr_info is useful to a normal user, as it is only
debug information. I'm actually minded to demote a bunch of the GICv3
prints to pr_debug.
OK.
+ gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
+ }
+
I think we need to clear GICR_VPENDBASER.Valid too (you can probably
reuse part of its_vpe_deschedule for that), so that we don't get into
a bizarre situation where CPU0's redistributor has some ancient
programming left in, and could start corrupting memory.
I can do that for safety. But is it possible of corrupting memory? Even if
GICR_VPENDBASER.Valid==1, I don't think it is possible that
GICR_VPENDBASER.Physical_Address equals to VPT_addr, so kernel should consider
vPE is not sheculed on CPU0 and only memory pointed by VPT_addr will be
modified. Please let me know if I'm wrong :)
/* Make sure the GIC has seen the above */
dsb(sy);
out:
--
1.8.3.1
Can you please respin this quickly with the above changes?
Sure.
Thanks,
Heyi
Thanks,
M.
