From: Nianyao Tang <[email protected]>
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 <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
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,
--
2.30.0
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 <[email protected]> wrote:
>
> From: Nianyao Tang <[email protected]>
>
> 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 <[email protected]>
Please also add:
Fixes: 64edfaa9a234 ("irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP")
With the above fixed:
Reviewed-by: Marc Zyngier <[email protected]>
Thanks,
M.
--
Without deviation from the norm, progress is not possible.
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 <[email protected]> wrote:
>> From: Nianyao Tang <[email protected]>
>>
>> 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 <[email protected]>
> Please also add:
>
> Fixes: 64edfaa9a234 ("irqchip/gic-v4.1: Implement the v4.1 flavour of VMAPP")
>
> With the above fixed:
>
> Reviewed-by: Marc Zyngier <[email protected]>
>
> Thanks,
>
> M.
>