diff mbox series

[RESPIN] irqchip/gic-v3-its:Fix GICv4.1 needless VSYNC after unmap VPE

Message ID 20240403083556.3862236-1-tangnianyao@huawei.com (mailing list archive)
State New, archived
Headers show
Series [RESPIN] irqchip/gic-v3-its:Fix GICv4.1 needless VSYNC after unmap VPE | expand

Commit Message

Tangnianyao April 3, 2024, 8:35 a.m. UTC 
From: Nianyao Tang <tangnianyao@huawei.com>

Quote from GIC spec 5.3.19, a VMAPP with {V, Alloc}=={0, x}
is self-synchronizing, This means the ITS command queue does not
show the command as consumed until all of its effects are completed.

We don't need VSYNC to guarantee unmap finish. And VSYNC after unmap VPE
will reach an invalid vpe table entry, which may trigger exception
like SError or RAS. Let's fix it.

Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

Comments

Marc Zyngier April 3, 2024, 10:09 a.m. UTC | #1 
Thanks for respinning this.

A few remarks:

The subject line could be improved. Something like:

"irqchip/gic-v4: Don't issue a VSYNC after VMAPP with V=0"

On Wed, 03 Apr 2024 09:35:56 +0100,
t00849498 <tangnianyao@huawei.com> wrote:
> 
> From: Nianyao Tang <tangnianyao@huawei.com>
> 
> Quote from GIC spec 5.3.19, a VMAPP with {V, Alloc}=={0, x}
> is self-synchronizing, This means the ITS command queue does not
> show the command as consumed until all of its effects are completed.

Since this is a direct quote, make it clear that it is so.

>
> We don't need VSYNC to guarantee unmap finish. And VSYNC after unmap VPE
> will reach an invalid vpe table entry, which may trigger exception
> like SError or RAS. Let's fix it.

This should be much stronger. It's not that we don't need VSYNC. It is
that VSYNC is actively wrong. I suggest that you rewrite the commit
message along these lines:

<msg>
As per the GICv4.1 spec (Arm IHI 0069H, 5.3.19):

"A VMAPP with {V, Alloc}=={0, x} is self-synchronizing. This means the
 ITS command queue does not show the command as consumed until all of
 its effects are completed."

Furthermore, VSYNC is allowed to deliver an SError when referencing a
non existent VPE.

By these definitions, a VMAPP followed by a VSYNC is a bug, as the
later references a VPE that has been unmapped by the former.

Fix it by eliding the VSYNC in this scenario.
</msg>

> 
> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>

Please also add:

Fixes: 64edfaa9a234 ("irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP")

With the above fixed:

Reviewed-by: Marc Zyngier <maz@kernel.org>

Thanks,

	M.
Tangnianyao April 6, 2024, 1:55 a.m. UTC | #2 
On 4/3/2024 18:09, Marc Zyngier wrote:
> Thanks for respinning this.
>
> A few remarks:
>
> The subject line could be improved. Something like:
>
> "irqchip/gic-v4: Don't issue a VSYNC after VMAPP with V=0"
>
> On Wed, 03 Apr 2024 09:35:56 +0100,
> t00849498 <tangnianyao@huawei.com> wrote:
>> From: Nianyao Tang <tangnianyao@huawei.com>
>>
>> Quote from GIC spec 5.3.19, a VMAPP with {V, Alloc}=={0, x}
>> is self-synchronizing, This means the ITS command queue does not
>> show the command as consumed until all of its effects are completed.
> Since this is a direct quote, make it clear that it is so.
>
>> We don't need VSYNC to guarantee unmap finish. And VSYNC after unmap VPE
>> will reach an invalid vpe table entry, which may trigger exception
>> like SError or RAS. Let's fix it.
> This should be much stronger. It's not that we don't need VSYNC. It is
> that VSYNC is actively wrong. I suggest that you rewrite the commit
> message along these lines:
>
> <msg>
> As per the GICv4.1 spec (Arm IHI 0069H, 5.3.19):
>
> "A VMAPP with {V, Alloc}=={0, x} is self-synchronizing. This means the
>  ITS command queue does not show the command as consumed until all of
>  its effects are completed."
>
> Furthermore, VSYNC is allowed to deliver an SError when referencing a
> non existent VPE.
>
> By these definitions, a VMAPP followed by a VSYNC is a bug, as the
> later references a VPE that has been unmapped by the former.
>
> Fix it by eliding the VSYNC in this scenario.
> </msg>

Thanks for the above comments, I will resend later.
>
>> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
> Please also add:
>
> Fixes: 64edfaa9a234 ("irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP")
>
> With the above fixed:
>
> Reviewed-by: Marc Zyngier <maz@kernel.org>
>
> Thanks,
>
> 	M.
>
diff mbox series

Patch

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..2a537cbfcb07 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -786,6 +786,7 @@  static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
 					   struct its_cmd_block *cmd,
 					   struct its_cmd_desc *desc)
 {
+	struct its_vpe *vpe = valid_vpe(its, desc->its_vmapp_cmd.vpe);
 	unsigned long vpt_addr, vconf_addr;
 	u64 target;
 	bool alloc;
@@ -798,6 +799,11 @@  static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
 		if (is_v4_1(its)) {
 			alloc = !atomic_dec_return(&desc->its_vmapp_cmd.vpe->vmapp_count);
 			its_encode_alloc(cmd, alloc);
+			/*
+			 * Unmapping a VPE is self-synchronizing on GICv4.1,
+			 * no need to issue a VSYNC.
+			 */
+			vpe = NULL;
 		}
 
 		goto out;
@@ -832,7 +838,7 @@  static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
 out:
 	its_fixup_cmd(cmd);
 
-	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
+	return vpe;
 }
 
 static struct its_vpe *its_build_vmapti_cmd(struct its_node *its,