Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 22 Jan 2019 16:10:58 +0000
Message-ID: <86zhrs8ygd.wl-marc.zyngier@arm.com>
From:   Marc Zyngier <marc.zyngier@arm.com>
To:     Heyi Guo <guoheyi@huawei.com>
Cc:     <linux-kernel@vger.kernel.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        <wanghaibin.wang@huawei.com>
Subject: Re: [RFC] irq-gic-v3-its: fix occasional VLPI drop
In-Reply-To: <b9392bee-7e54-749c-04d9-5e7fee3a6203@huawei.com>
References: <1548071508-11846-1-git-send-email-guoheyi@huawei.com>
        <86fttl9fxh.wl-marc.zyngier@arm.com>
        <b9392bee-7e54-749c-04d9-5e7fee3a6203@huawei.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/25.1
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
Organization: ARM Ltd
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, 22 Jan 2019 12:44:18 +0000,
Heyi Guo <guoheyi@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 <guoheyi@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 <guoheyi@huawei.com>
> >> Signed-off-by: Heyi Guo <heyi.guo@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.