On Tue, 22 Jan 2019 12:44:18 +0000, Heyi Guo <guoh...@huawei.com> wrote: > > 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... >
No worries, that's mostly for me to make sure I understood the issue correctly. I've paged out the GICv4 driver a while ago, and it takes some effort to reload it. ;-) > > > > - 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(). What I meant is that as far as the GIC is concerned, that's equivalent to a VM creation. its_map_vm() is what makes the GIC aware of it. > > > > - 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. Dropping VLPIs that are out of range is the expected behaviour. > > > > >> + * 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 :) Well, we must still make sure that no VLPI will be propagated to memory before KVM actually run a vcpu on this CPU. And yes, this is for safety, we'd be very unlucky if that happened. But still, it is theoretically possible > > > >> /* 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. Cool, thanks. M. -- Jazz is not dead, it just smell funny.