[PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[t00849498]

From GIC spec, 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. A VSYNC with unmapped
vpeid is not needed.

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

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index fca888b36680..a0ca5dcbb245 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -789,6 +789,7 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
 	unsigned long vpt_addr, vconf_addr;
 	u64 target;
 	bool alloc;
+	bool unmap_v4_1 = !desc->its_vmapp_cmd.valid && is_v4_1(its);
 
 	its_encode_cmd(cmd, GITS_CMD_VMAPP);
 	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
@@ -832,6 +833,9 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
 out:
 	its_fixup_cmd(cmd);
 
+	if (unmap_v4_1)
+		return NULL;
+
 	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
 }
 
-- 
2.30.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Marc Zyngier]

On Tue, 02 Apr 2024 12:41:47 +0100,
t00849498 <tangnianyao@huawei.com> wrote:
> 
> From GIC spec, a VMAPP with {V, Alloc}=={0, x} is self-synchronizing,

It'd be nice to quote the part of the spec (5.3.19).

> This means the ITS command queue does not show the command as
> consumed until all of its effects are completed. A VSYNC with unmapped
> vpeid is not needed.
> 
> Signed-off-by: t00849498 <tangnianyao@huawei.com>

Previous contributions with the same email address had the name
"Nianyao Tang" associated with it. Was it wrong in the past? Or is the
above wrong?

> ---
>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index fca888b36680..a0ca5dcbb245 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -789,6 +789,7 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>  	unsigned long vpt_addr, vconf_addr;
>  	u64 target;
>  	bool alloc;
> +	bool unmap_v4_1 = !desc->its_vmapp_cmd.valid && is_v4_1(its);
>
>  	its_encode_cmd(cmd, GITS_CMD_VMAPP);
>  	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
> @@ -832,6 +833,9 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>  out:
>  	its_fixup_cmd(cmd);
>  
> +	if (unmap_v4_1)
> +		return NULL;
> +
>  	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
>  }
>  

This is a bit ugly. We already have a whole block dedicated to
handling VMAPP with V=0 and GICv4.1, and it'd be more readable to keep
all that code together. Something like the untested patch below.

Thanks,

	M.

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,

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Tangnianyao]

On 4/2/2024 20:35, Marc Zyngier wrote:
> On Tue, 02 Apr 2024 12:41:47 +0100,
> t00849498 <tangnianyao@huawei.com> wrote:
>> From GIC spec, a VMAPP with {V, Alloc}=={0, x} is self-synchronizing,
> It'd be nice to quote the part of the spec (5.3.19).
yes, that's quote from GIC spec.
>> This means the ITS command queue does not show the command as
>> consumed until all of its effects are completed. A VSYNC with unmapped
>> vpeid is not needed.
>>
>> Signed-off-by: t00849498 <tangnianyao@huawei.com>
> Previous contributions with the same email address had the name
> "Nianyao Tang" associated with it. Was it wrong in the past? Or is the
> above wrong?
Sorry, the above name is wrong, should be "Nianyao Tang".
>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index fca888b36680..a0ca5dcbb245 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -789,6 +789,7 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>>  	unsigned long vpt_addr, vconf_addr;
>>  	u64 target;
>>  	bool alloc;
>> +	bool unmap_v4_1 = !desc->its_vmapp_cmd.valid && is_v4_1(its);
>>
>>  	its_encode_cmd(cmd, GITS_CMD_VMAPP);
>>  	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
>> @@ -832,6 +833,9 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>>  out:
>>  	its_fixup_cmd(cmd);
>>  
>> +	if (unmap_v4_1)
>> +		return NULL;
>> +
>>  	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
>>  }
>>  
> This is a bit ugly. We already have a whole block dedicated to
> handling VMAPP with V=0 and GICv4.1, and it'd be more readable to keep
> all that code together. Something like the untested patch below.

Thank you for quick fix, it would be great to remove this VSYNC. ITS handling VSYNC unmap
vpeid may waste some time, trigger exception and needed to be handled.

> Thanks,
>
> 	M.
>
> 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,
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Marc Zyngier]

On Tue, 02 Apr 2024 14:32:40 +0100,
Tangnianyao <tangnianyao@huawei.com> wrote:
> 
> 
> 
> On 4/2/2024 20:35, Marc Zyngier wrote:
> > On Tue, 02 Apr 2024 12:41:47 +0100,
> > t00849498 <tangnianyao@huawei.com> wrote:
> >> From GIC spec, a VMAPP with {V, Alloc}=={0, x} is self-synchronizing,
> > It'd be nice to quote the part of the spec (5.3.19).
> yes, that's quote from GIC spec.
> >> This means the ITS command queue does not show the command as
> >> consumed until all of its effects are completed. A VSYNC with unmapped
> >> vpeid is not needed.
> >>
> >> Signed-off-by: t00849498 <tangnianyao@huawei.com>
> > Previous contributions with the same email address had the name
> > "Nianyao Tang" associated with it. Was it wrong in the past? Or is the
> > above wrong?
> Sorry, the above name is wrong, should be "Nianyao Tang".
> >
> >> ---
> >>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
> >>  1 file changed, 4 insertions(+)
> >>
> >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> >> index fca888b36680..a0ca5dcbb245 100644
> >> --- a/drivers/irqchip/irq-gic-v3-its.c
> >> +++ b/drivers/irqchip/irq-gic-v3-its.c
> >> @@ -789,6 +789,7 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
> >>  	unsigned long vpt_addr, vconf_addr;
> >>  	u64 target;
> >>  	bool alloc;
> >> +	bool unmap_v4_1 = !desc->its_vmapp_cmd.valid && is_v4_1(its);
> >>
> >>  	its_encode_cmd(cmd, GITS_CMD_VMAPP);
> >>  	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
> >> @@ -832,6 +833,9 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
> >>  out:
> >>  	its_fixup_cmd(cmd);
> >>  
> >> +	if (unmap_v4_1)
> >> +		return NULL;
> >> +
> >>  	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
> >>  }
> >>  
> > This is a bit ugly. We already have a whole block dedicated to
> > handling VMAPP with V=0 and GICv4.1, and it'd be more readable to keep
> > all that code together. Something like the untested patch below.
> 
> Thank you for quick fix, it would be great to remove this VSYNC. ITS handling VSYNC unmap
> vpeid may waste some time, trigger exception and needed to be
> handled.

Do you actually see an exception being delivered from this?

In any case, feel free to respin the patch after having tested this
diff, with the commit message fixed and a Fixes: tag attached to it.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Tangnianyao]

On 4/2/2024 21:43, Marc Zyngier wrote:
> On Tue, 02 Apr 2024 14:32:40 +0100,
> Tangnianyao <tangnianyao@huawei.com> wrote:
>>
>>
>> On 4/2/2024 20:35, Marc Zyngier wrote:
>>> On Tue, 02 Apr 2024 12:41:47 +0100,
>>> t00849498 <tangnianyao@huawei.com> wrote:
>>>> From GIC spec, a VMAPP with {V, Alloc}=={0, x} is self-synchronizing,
>>> It'd be nice to quote the part of the spec (5.3.19).
>> yes, that's quote from GIC spec.
>>>> This means the ITS command queue does not show the command as
>>>> consumed until all of its effects are completed. A VSYNC with unmapped
>>>> vpeid is not needed.
>>>>
>>>> Signed-off-by: t00849498 <tangnianyao@huawei.com>
>>> Previous contributions with the same email address had the name
>>> "Nianyao Tang" associated with it. Was it wrong in the past? Or is the
>>> above wrong?
>> Sorry, the above name is wrong, should be "Nianyao Tang".
>>>> ---
>>>>  drivers/irqchip/irq-gic-v3-its.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>>>> index fca888b36680..a0ca5dcbb245 100644
>>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>>> @@ -789,6 +789,7 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>>>>  	unsigned long vpt_addr, vconf_addr;
>>>>  	u64 target;
>>>>  	bool alloc;
>>>> +	bool unmap_v4_1 = !desc->its_vmapp_cmd.valid && is_v4_1(its);
>>>>
>>>>  	its_encode_cmd(cmd, GITS_CMD_VMAPP);
>>>>  	its_encode_vpeid(cmd, desc->its_vmapp_cmd.vpe->vpe_id);
>>>> @@ -832,6 +833,9 @@ static struct its_vpe *its_build_vmapp_cmd(struct its_node *its,
>>>>  out:
>>>>  	its_fixup_cmd(cmd);
>>>>  
>>>> +	if (unmap_v4_1)
>>>> +		return NULL;
>>>> +
>>>>  	return valid_vpe(its, desc->its_vmapp_cmd.vpe);
>>>>  }
>>>>  
>>> This is a bit ugly. We already have a whole block dedicated to
>>> handling VMAPP with V=0 and GICv4.1, and it'd be more readable to keep
>>> all that code together. Something like the untested patch below.
>> Thank you for quick fix, it would be great to remove this VSYNC. ITS handling VSYNC unmap
>> vpeid may waste some time, trigger exception and needed to be
>> handled.
> Do you actually see an exception being delivered from this?
>
> In any case, feel free to respin the patch after having tested this
> diff, with the commit message fixed and a Fixes: tag attached to it.

In our developing implemenation, ITS would report RAS when doing vsync
and reaching an invalid vpe table entry. It is reasonable to report RAS, right?

It just reports, and kernel can still run normally regardless of this RAS message.

> Thanks,
>
> 	M.
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Marc Zyngier]

On Wed, 03 Apr 2024 03:18:51 +0100,
Tangnianyao <tangnianyao@huawei.com> wrote:
> 
> 
> 
> On 4/2/2024 21:43, Marc Zyngier wrote:
> > On Tue, 02 Apr 2024 14:32:40 +0100,
> > Tangnianyao <tangnianyao@huawei.com> wrote:
> >>
> >>
> >> Thank you for quick fix, it would be great to remove this VSYNC. ITS handling VSYNC unmap
> >> vpeid may waste some time, trigger exception and needed to be
> >> handled.
> > Do you actually see an exception being delivered from this?
> >
> > In any case, feel free to respin the patch after having tested this
> > diff, with the commit message fixed and a Fixes: tag attached to it.
> 
> In our developing implemenation, ITS would report RAS when doing vsync
> and reaching an invalid vpe table entry. It is reasonable to report RAS, right?

If a RAS error is the only reporting method you have, then I suspect
you don't have much of a choice (the pseudocode indicates an IMPDEF
SError for VSYNC). Shame this wasn't caught earlier.

> It just reports, and kernel can still run normally regardless of
> this RAS message.

That really depends on the policy behind RAS errors.

Looking forward to your respinning of this patch.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] irqchip/gic-v3-its: Don't need VSYNC if VMAPP with {V, Alloc}=={0, x}

[Tangnianyao]

On 4/3/2024 14:00, Marc Zyngier wrote:
> On Wed, 03 Apr 2024 03:18:51 +0100,
> Tangnianyao <tangnianyao@huawei.com> wrote:
>>
>>
>> On 4/2/2024 21:43, Marc Zyngier wrote:
>>> On Tue, 02 Apr 2024 14:32:40 +0100,
>>> Tangnianyao <tangnianyao@huawei.com> wrote:
>>>>
>>>> Thank you for quick fix, it would be great to remove this VSYNC. ITS handling VSYNC unmap
>>>> vpeid may waste some time, trigger exception and needed to be
>>>> handled.
>>> Do you actually see an exception being delivered from this?
>>>
>>> In any case, feel free to respin the patch after having tested this
>>> diff, with the commit message fixed and a Fixes: tag attached to it.
>> In our developing implemenation, ITS would report RAS when doing vsync
>> and reaching an invalid vpe table entry. It is reasonable to report RAS, right?
> If a RAS error is the only reporting method you have, then I suspect
> you don't have much of a choice (the pseudocode indicates an IMPDEF
> SError for VSYNC). Shame this wasn't caught earlier.
>
>> It just reports, and kernel can still run normally regardless of
>> this RAS message.
> That really depends on the policy behind RAS errors.
>
> Looking forward to your respinning of this patch.

I have tested your patch and it's ok to fix the above problem.
Respin patch will be sent later.


>
> Thanks,
>
> 	M.
>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel