linux-kernel - Re: [RFC] irq-gic-v3-its: fix occasional VLPI drop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86fttl9fxh.wl-marc.zyngier@arm.com>
Date:   Tue, 22 Jan 2019 09:53:30 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     Heyi Guo <guoheyi@...wei.com>
Cc:     <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Jason Cooper <jason@...edaemon.net>,
        <wanghaibin.wang@...wei.com>
Subject: Re: [RFC] irq-gic-v3-its: fix occasional VLPI drop

Hi Heyi,

On Mon, 21 Jan 2019 11:51:48 +0000,
Heyi Guo <guoheyi@...wei.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:

- When a VM gets created, all the vPEs are mapped to CPU0's
  redistributor.

- 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.

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@...wei.com>
> Signed-off-by: Heyi Guo <heyi.guo@...aro.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?

> +	 * 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.

> +		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.

>  	/* 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?

Thanks,

	M.

-- 
Jazz is not dead, it just smell funny.