diff mbox series

[kvm-unit-tests,04/10] arm/arm64: gic: Remove unnecessary synchronization with stats_reset()

Message ID 20201125155113.192079-5-alexandru.elisei@arm.com (mailing list archive)
State New, archived
Headers show
Series GIC fixes and improvements | expand

Commit Message

Alexandru Elisei Nov. 25, 2020, 3:51 p.m. UTC
The GICv3 driver executes a DSB barrier before sending an IPI, which
ensures that memory accesses have completed. This removes the need to
enforce ordering with respect to stats_reset() in the IPI handler.

For GICv2, we still need the DMB to ensure ordering between the read of the
GICC_IAR MMIO register and the read from the acked array. It also matches
what the Linux GICv2 driver does in gic_handle_irq().

Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Auger Eric Dec. 1, 2020, 4:48 p.m. UTC | #1
Hi Alexandru,

On 11/25/20 4:51 PM, Alexandru Elisei wrote:
> The GICv3 driver executes a DSB barrier before sending an IPI, which
> ensures that memory accesses have completed. This removes the need to
> enforce ordering with respect to stats_reset() in the IPI handler.
> 
> For GICv2, we still need the DMB to ensure ordering between the read of the
> GICC_IAR MMIO register and the read from the acked array. It also matches
> what the Linux GICv2 driver does in gic_handle_irq().
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/gic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 4e947e8516a2..7befda2a8673 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -60,7 +60,6 @@ static void stats_reset(void)
>  		bad_sender[i] = -1;
>  		bad_irq[i] = -1;
>  	}
> -	smp_wmb();
Here we are (pair removed). Still the one in check_acked still exists.
>  }
>  
>  static void check_acked(const char *testname, cpumask_t *mask)
> @@ -150,7 +149,13 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		gic_write_eoir(irqstat);
> -		smp_rmb(); /* pairs with wmb in stats_reset */
> +		/*
> +		 * Make sure data written before the IPI was triggered is
> +		 * observed after the IAR is read. Pairs with the smp_wmb
> +		 * when sending the IPI.
> +		 */
> +		if (gic_version() == 2)
> +			smp_rmb();
>  		++acked[smp_processor_id()];
>  		check_ipi_sender(irqstat);
>  		check_irqnr(irqnr);
> 
Thanks

Eric
Alexandru Elisei Dec. 2, 2020, 2:06 p.m. UTC | #2
Hi Eric,

On 12/1/20 4:48 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 11/25/20 4:51 PM, Alexandru Elisei wrote:
>> The GICv3 driver executes a DSB barrier before sending an IPI, which
>> ensures that memory accesses have completed. This removes the need to
>> enforce ordering with respect to stats_reset() in the IPI handler.
>>
>> For GICv2, we still need the DMB to ensure ordering between the read of the
>> GICC_IAR MMIO register and the read from the acked array. It also matches
>> what the Linux GICv2 driver does in gic_handle_irq().
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/gic.c | 9 +++++++--
>>  1 file changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 4e947e8516a2..7befda2a8673 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -60,7 +60,6 @@ static void stats_reset(void)
>>  		bad_sender[i] = -1;
>>  		bad_irq[i] = -1;
>>  	}
>> -	smp_wmb();
> Here we are (pair removed). Still the one in check_acked still exists.

The smp_rmb() from check_acked() is there to implement the message passing pattern
wrt the writes from the ipi_handler() function, not the writes from stats_reset().
See the next patch where I try to explain how the barriers should work.

Thanks,

Alex

>>  }
>>  
>>  static void check_acked(const char *testname, cpumask_t *mask)
>> @@ -150,7 +149,13 @@ static void ipi_handler(struct pt_regs *regs __unused)
>>  
>>  	if (irqnr != GICC_INT_SPURIOUS) {
>>  		gic_write_eoir(irqstat);
>> -		smp_rmb(); /* pairs with wmb in stats_reset */
>> +		/*
>> +		 * Make sure data written before the IPI was triggered is
>> +		 * observed after the IAR is read. Pairs with the smp_wmb
>> +		 * when sending the IPI.
>> +		 */
>> +		if (gic_version() == 2)
>> +			smp_rmb();
>>  		++acked[smp_processor_id()];
>>  		check_ipi_sender(irqstat);
>>  		check_irqnr(irqnr);
>>
> Thanks
>
> Eric
>
Auger Eric Dec. 3, 2020, 1:10 p.m. UTC | #3
Hi,

On 11/25/20 4:51 PM, Alexandru Elisei wrote:
> The GICv3 driver executes a DSB barrier before sending an IPI, which
> ensures that memory accesses have completed. This removes the need to
> enforce ordering with respect to stats_reset() in the IPI handler.
> 
> For GICv2, we still need the DMB to ensure ordering between the read of the
> GICC_IAR MMIO register and the read from the acked array. It also matches
> what the Linux GICv2 driver does in gic_handle_irq().
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Eric
> ---
>  arm/gic.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 4e947e8516a2..7befda2a8673 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -60,7 +60,6 @@ static void stats_reset(void)
>  		bad_sender[i] = -1;
>  		bad_irq[i] = -1;
>  	}
> -	smp_wmb();
>  }
>  
>  static void check_acked(const char *testname, cpumask_t *mask)
> @@ -150,7 +149,13 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		gic_write_eoir(irqstat);
> -		smp_rmb(); /* pairs with wmb in stats_reset */
> +		/*
> +		 * Make sure data written before the IPI was triggered is
> +		 * observed after the IAR is read. Pairs with the smp_wmb
> +		 * when sending the IPI.
> +		 */
> +		if (gic_version() == 2)
> +			smp_rmb();
>  		++acked[smp_processor_id()];
>  		check_ipi_sender(irqstat);
>  		check_irqnr(irqnr);
>
diff mbox series

Patch

diff --git a/arm/gic.c b/arm/gic.c
index 4e947e8516a2..7befda2a8673 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -60,7 +60,6 @@  static void stats_reset(void)
 		bad_sender[i] = -1;
 		bad_irq[i] = -1;
 	}
-	smp_wmb();
 }
 
 static void check_acked(const char *testname, cpumask_t *mask)
@@ -150,7 +149,13 @@  static void ipi_handler(struct pt_regs *regs __unused)
 
 	if (irqnr != GICC_INT_SPURIOUS) {
 		gic_write_eoir(irqstat);
-		smp_rmb(); /* pairs with wmb in stats_reset */
+		/*
+		 * Make sure data written before the IPI was triggered is
+		 * observed after the IAR is read. Pairs with the smp_wmb
+		 * when sending the IPI.
+		 */
+		if (gic_version() == 2)
+			smp_rmb();
 		++acked[smp_processor_id()];
 		check_ipi_sender(irqstat);
 		check_irqnr(irqnr);