[PATCH] irqchip/gic-v4: fix occasional VLPI drop

[PATCH] irqchip/gic-v4: fix occasional VLPI drop

[Heyi Guo]

1. In current implementation, every VLPI will temporarily be mapped to
the first CPU in system (normally CPU0) and then moved to the real
scheduled CPU later.

2. So there is a time window and a VLPI may be sent to CPU0 instead of
the real scheduled vCPU, in a multi-CPU virtual machine.

3. However, CPU0 may have not been scheduled as a virtual CPU after
system boots up, so the value of its GICR_VPROPBASER is unknown at
that moment.

4. If the INTID of VLPI is larger than 2^(GICR_VPROPBASER.IDbits+1),
while IDbits is also in unknown state, GIC will behave as if the VLPI
is out of range and simply drop it, which results in interrupt missing
in Guest.

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.

We also clear Valid bit of GICR_VPENDBASER in case any ancient
programming gets left in and causes memory corrupting.

Signed-off-by: Heyi Guo <guoheyi@huawei.com>
Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
---
 drivers/irqchip/irq-gic-v3-its.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 3365d44..634c8a7 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2145,6 +2145,31 @@ static void its_cpu_init_lpis(void)
 	val |= GICR_CTLR_ENABLE_LPIS;
 	writel_relaxed(val, rbase + GICR_CTLR);
 
+	if (gic_rdists->has_vlpis) {
+		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
+
+		/*
+		 * It's possible for CPU to receive VLPIs before it is
+		 * sheduled as a vPE, especially for the first CPU, and the
+		 * VLPI with INTID larger than 2^(IDbits+1) will be considered
+		 * as out of range and dropped by GIC.
+		 * So we initialize IDbits to known value to avoid VLPI drop.
+		 */
+		val = (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
+		pr_debug("GICv4: CPU%d: Init IDbits to 0x%llx for GICR_VPROPBASER\n",
+			smp_processor_id(), val);
+		gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
+
+		/*
+		 * Also clear Valid bit of GICR_VPENDBASER, in case some
+		 * ancient programming gets left in and has possibility of
+		 * corrupting memory.
+		 */
+		val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
+		val &= ~GICR_VPENDBASER_Valid;
+		gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
+	}
+
 	/* Make sure the GIC has seen the above */
 	dsb(sy);
 out:
-- 
1.8.3.1

Re: [PATCH] irqchip/gic-v4: fix occasional VLPI drop

[Marc Zyngier]

On Wed, 23 Jan 2019 08:59:08 +0000,
Heyi Guo <guoheyi@huawei.com> wrote:
> 
> 1. In current implementation, every VLPI will temporarily be mapped to
> the first CPU in system (normally CPU0) and then moved to the real
> scheduled CPU later.
> 
> 2. So there is a time window and a VLPI may be sent to CPU0 instead of
> the real scheduled vCPU, in a multi-CPU virtual machine.
> 
> 3. However, CPU0 may have not been scheduled as a virtual CPU after
> system boots up, so the value of its GICR_VPROPBASER is unknown at
> that moment.
> 
> 4. If the INTID of VLPI is larger than 2^(GICR_VPROPBASER.IDbits+1),
> while IDbits is also in unknown state, GIC will behave as if the VLPI
> is out of range and simply drop it, which results in interrupt missing
> in Guest.
> 
> 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.
> 
> We also clear Valid bit of GICR_VPENDBASER in case any ancient
> programming gets left in and causes memory corrupting.
> 
> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 3365d44..634c8a7 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2145,6 +2145,31 @@ static void its_cpu_init_lpis(void)
>  	val |= GICR_CTLR_ENABLE_LPIS;
>  	writel_relaxed(val, rbase + GICR_CTLR);
>  
> +	if (gic_rdists->has_vlpis) {
> +		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> +
> +		/*
> +		 * It's possible for CPU to receive VLPIs before it is
> +		 * sheduled as a vPE, especially for the first CPU, and the
> +		 * VLPI with INTID larger than 2^(IDbits+1) will be considered
> +		 * as out of range and dropped by GIC.
> +		 * So we initialize IDbits to known value to avoid VLPI drop.
> +		 */
> +		val = (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
> +		pr_debug("GICv4: CPU%d: Init IDbits to 0x%llx for GICR_VPROPBASER\n",
> +			smp_processor_id(), val);
> +		gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
> +
> +		/*
> +		 * Also clear Valid bit of GICR_VPENDBASER, in case some
> +		 * ancient programming gets left in and has possibility of
> +		 * corrupting memory.
> +		 */
> +		val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
> +		val &= ~GICR_VPENDBASER_Valid;
> +		gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);

I'm afraid it is not enough to just write back with the valid bit
clear. You need to make sure Dirty has been cleared before continuing,
which is why I was asking you to reuse some bits of its_vpe_deschedule.

Something like this:

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index db20e992a40f..1808cb5f6a3e 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2751,9 +2751,8 @@ static void its_vpe_schedule(struct its_vpe *vpe)
 	gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
 }
 
-static void its_vpe_deschedule(struct its_vpe *vpe)
+static u64 its_clear_vpend_valid(void __iomem *vlpi_base)
 {
-	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
 	u32 count = 1000000;	/* 1s! */
 	bool clean;
 	u64 val;
@@ -2773,7 +2772,17 @@ static void its_vpe_deschedule(struct its_vpe *vpe)
 		}
 	} while (!clean && count);
 
-	if (unlikely(!clean && !count)) {
+	return val;
+}
+
+static void its_vpe_deschedule(struct its_vpe *vpe)
+{
+	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
+	u64 val;
+
+	val = its_clear_vpend_valid(vlpi_base);
+
+	if (unlikely(val & GICR_VPENDBASER_Dirty)) {
 		pr_err_ratelimited("ITS virtual pending table not cleaning\n");
 		vpe->idai = false;
 		vpe->pending_last = true;

And you can the rewrite the above GICR_VPENDBASER_Valid clearing as:

	val = its_clear_vpend_valid(vlpi_base);
	WARN_ON(val & GICR_VPENDBASER_Dirty);

We'll at least get a warning if something goes wrong.

Thanks,

	M.

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

Re: [PATCH] irqchip/gic-v4: fix occasional VLPI drop

[Heyi Guo]

On 2019/1/24 18:33, Marc Zyngier wrote:
> On Wed, 23 Jan 2019 08:59:08 +0000,
> Heyi Guo <guoheyi@huawei.com> wrote:
>> 1. In current implementation, every VLPI will temporarily be mapped to
>> the first CPU in system (normally CPU0) and then moved to the real
>> scheduled CPU later.
>>
>> 2. So there is a time window and a VLPI may be sent to CPU0 instead of
>> the real scheduled vCPU, in a multi-CPU virtual machine.
>>
>> 3. However, CPU0 may have not been scheduled as a virtual CPU after
>> system boots up, so the value of its GICR_VPROPBASER is unknown at
>> that moment.
>>
>> 4. If the INTID of VLPI is larger than 2^(GICR_VPROPBASER.IDbits+1),
>> while IDbits is also in unknown state, GIC will behave as if the VLPI
>> is out of range and simply drop it, which results in interrupt missing
>> in Guest.
>>
>> 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.
>>
>> We also clear Valid bit of GICR_VPENDBASER in case any ancient
>> programming gets left in and causes memory corrupting.
>>
>> Signed-off-by: Heyi Guo <guoheyi@huawei.com>
>> Signed-off-by: Heyi Guo <heyi.guo@linaro.org>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 25 +++++++++++++++++++++++++
>>   1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 3365d44..634c8a7 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -2145,6 +2145,31 @@ static void its_cpu_init_lpis(void)
>>   	val |= GICR_CTLR_ENABLE_LPIS;
>>   	writel_relaxed(val, rbase + GICR_CTLR);
>>   
>> +	if (gic_rdists->has_vlpis) {
>> +		void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
>> +
>> +		/*
>> +		 * It's possible for CPU to receive VLPIs before it is
>> +		 * sheduled as a vPE, especially for the first CPU, and the
>> +		 * VLPI with INTID larger than 2^(IDbits+1) will be considered
>> +		 * as out of range and dropped by GIC.
>> +		 * So we initialize IDbits to known value to avoid VLPI drop.
>> +		 */
>> +		val = (LPI_NRBITS - 1) & GICR_VPROPBASER_IDBITS_MASK;
>> +		pr_debug("GICv4: CPU%d: Init IDbits to 0x%llx for GICR_VPROPBASER\n",
>> +			smp_processor_id(), val);
>> +		gits_write_vpropbaser(val, vlpi_base + GICR_VPROPBASER);
>> +
>> +		/*
>> +		 * Also clear Valid bit of GICR_VPENDBASER, in case some
>> +		 * ancient programming gets left in and has possibility of
>> +		 * corrupting memory.
>> +		 */
>> +		val = gits_read_vpendbaser(vlpi_base + GICR_VPENDBASER);
>> +		val &= ~GICR_VPENDBASER_Valid;
>> +		gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
> I'm afraid it is not enough to just write back with the valid bit
> clear. You need to make sure Dirty has been cleared before continuing,
> which is why I was asking you to reuse some bits of its_vpe_deschedule.
>
> Something like this:
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index db20e992a40f..1808cb5f6a3e 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2751,9 +2751,8 @@ static void its_vpe_schedule(struct its_vpe *vpe)
>   	gits_write_vpendbaser(val, vlpi_base + GICR_VPENDBASER);
>   }
>   
> -static void its_vpe_deschedule(struct its_vpe *vpe)
> +static u64 its_clear_vpend_valid(void __iomem *vlpi_base)
>   {
> -	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
>   	u32 count = 1000000;	/* 1s! */
>   	bool clean;
>   	u64 val;
> @@ -2773,7 +2772,17 @@ static void its_vpe_deschedule(struct its_vpe *vpe)
>   		}
>   	} while (!clean && count);
>   
> -	if (unlikely(!clean && !count)) {
> +	return val;
> +}
> +
> +static void its_vpe_deschedule(struct its_vpe *vpe)
> +{
> +	void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> +	u64 val;
> +
> +	val = its_clear_vpend_valid(vlpi_base);
> +
> +	if (unlikely(val & GICR_VPENDBASER_Dirty)) {
>   		pr_err_ratelimited("ITS virtual pending table not cleaning\n");
>   		vpe->idai = false;
>   		vpe->pending_last = true;
>
> And you can the rewrite the above GICR_VPENDBASER_Valid clearing as:
>
> 	val = its_clear_vpend_valid(vlpi_base);
> 	WARN_ON(val & GICR_VPENDBASER_Dirty);
>
> We'll at least get a warning if something goes wrong.
Got it. Will send a v2 ASAP :)

Thanks,

Heyi
>
> Thanks,
>
> 	M.
>