diff mbox series

[kvm-unit-tests,03/10] arm/arm64: gic: Remove memory synchronization from ipi_clear_active_handler()

Message ID 20201125155113.192079-4-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 gicv{2,3}-active test sends an IPI from the boot CPU to itself, then
checks that the interrupt has been received as expected. There is no need
to use inter-processor memory synchronization primitives on code that runs
on the same CPU, so remove the unneeded memory barriers.

The arrays are modified asynchronously (in the interrupt handler) and it is
possible for the compiler to infer that they won't be changed during normal
program flow and try to perform harmful optimizations (like stashing a
previous read in a register and reusing it). To prevent this, for GICv2,
the smp_wmb() in gicv2_ipi_send_self() is replaced with a compiler barrier.
For GICv3, the wmb() barrier in gic_ipi_send_single() already implies a
compiler barrier.

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

Comments

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

On 11/25/20 4:51 PM, Alexandru Elisei wrote:
> The gicv{2,3}-active test sends an IPI from the boot CPU to itself, then
> checks that the interrupt has been received as expected. There is no need
> to use inter-processor memory synchronization primitives on code that runs
> on the same CPU, so remove the unneeded memory barriers.
> 
> The arrays are modified asynchronously (in the interrupt handler) and it is
> possible for the compiler to infer that they won't be changed during normal
> program flow and try to perform harmful optimizations (like stashing a
> previous read in a register and reusing it). To prevent this, for GICv2,
> the smp_wmb() in gicv2_ipi_send_self() is replaced with a compiler barrier.
> For GICv3, the wmb() barrier in gic_ipi_send_single() already implies a
> compiler barrier.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/gic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index 401ffafe4299..4e947e8516a2 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -12,6 +12,7 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include <libcflat.h>
> +#include <linux/compiler.h>
>  #include <errata.h>
>  #include <asm/setup.h>
>  #include <asm/processor.h>
> @@ -260,7 +261,8 @@ static void check_lpi_hits(int *expected, const char *msg)
>  
>  static void gicv2_ipi_send_self(void)
>  {> -	smp_wmb();
nit: previous patch added it and this patch removes it. maybe squash the
modifs into the previous patch saying only a barrier() is needed for self()?
> +	/* Prevent the compiler from optimizing memory accesses */
> +	barrier();
>  	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
>  }
>  
> @@ -359,6 +361,7 @@ static struct gic gicv3 = {
>  	},
>  };
>  
> +/* Runs on the same CPU as the sender, no need for memory synchronization */
>  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
> @@ -375,13 +378,10 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -		smp_rmb(); /* pairs with wmb in stats_reset */
the comment says it is paired with wmd in stats_reset. So is it OK to
leave the associated wmb?
>  		++acked[smp_processor_id()];
>  		check_irqnr(irqnr);
> -		smp_wmb(); /* pairs with rmb in check_acked */
same here.
>  	} else {
>  		++spurious[smp_processor_id()];
> -		smp_wmb();
>  	}
>  }
>  
> 
Thanks

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

On 12/1/20 4:37 PM, Auger Eric wrote:
> Hi Alexandru,
>
> On 11/25/20 4:51 PM, Alexandru Elisei wrote:
>> The gicv{2,3}-active test sends an IPI from the boot CPU to itself, then
>> checks that the interrupt has been received as expected. There is no need
>> to use inter-processor memory synchronization primitives on code that runs
>> on the same CPU, so remove the unneeded memory barriers.
>>
>> The arrays are modified asynchronously (in the interrupt handler) and it is
>> possible for the compiler to infer that they won't be changed during normal
>> program flow and try to perform harmful optimizations (like stashing a
>> previous read in a register and reusing it). To prevent this, for GICv2,
>> the smp_wmb() in gicv2_ipi_send_self() is replaced with a compiler barrier.
>> For GICv3, the wmb() barrier in gic_ipi_send_single() already implies a
>> compiler barrier.
>>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/gic.c | 8 ++++----
>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index 401ffafe4299..4e947e8516a2 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -12,6 +12,7 @@
>>   * This work is licensed under the terms of the GNU LGPL, version 2.
>>   */
>>  #include <libcflat.h>
>> +#include <linux/compiler.h>
>>  #include <errata.h>
>>  #include <asm/setup.h>
>>  #include <asm/processor.h>
>> @@ -260,7 +261,8 @@ static void check_lpi_hits(int *expected, const char *msg)
>>  
>>  static void gicv2_ipi_send_self(void)
>>  {> -	smp_wmb();
> nit: previous patch added it and this patch removes it. maybe squash the
> modifs into the previous patch saying only a barrier() is needed for self()?
You're right, this does look out of place. I'll merge this change into the
previous patch.
>> +	/* Prevent the compiler from optimizing memory accesses */
>> +	barrier();
>>  	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
>>  }
>>  
>> @@ -359,6 +361,7 @@ static struct gic gicv3 = {
>>  	},
>>  };
>>  
>> +/* Runs on the same CPU as the sender, no need for memory synchronization */
>>  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>  {
>>  	u32 irqstat = gic_read_iar();
>> @@ -375,13 +378,10 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>  
>>  		writel(val, base + GICD_ICACTIVER);
>>  
>> -		smp_rmb(); /* pairs with wmb in stats_reset */
> the comment says it is paired with wmd in stats_reset. So is it OK to
> leave the associated wmb?

This patch removes multi-processor synchronization from the functions that run on
the same CPU. stats_reset() can be called from one CPU (the IPI_SENDER CPU) and
the variables it modifies accessed by the interrupt handlers running on different
CPUs, like it happens for the IPI tests. In that case we do need the proper
barriers in place.

Thanks,

Alex

>>  		++acked[smp_processor_id()];
>>  		check_irqnr(irqnr);
>> -		smp_wmb(); /* pairs with rmb in check_acked */
> same here.
>>  	} else {
>>  		++spurious[smp_processor_id()];
>> -		smp_wmb();
>>  	}
>>  }
>>  
>>
> Thanks
>
> Eric
>
Alexandru Elisei Dec. 2, 2020, 2:14 p.m. UTC | #3
Hi,

On 12/2/20 2:02 PM, Alexandru Elisei wrote:

> Hi Eric,
>
> On 12/1/20 4:37 PM, Auger Eric wrote:
>> Hi Alexandru,
>>
>> On 11/25/20 4:51 PM, Alexandru Elisei wrote:
>>> The gicv{2,3}-active test sends an IPI from the boot CPU to itself, then
>>> checks that the interrupt has been received as expected. There is no need
>>> to use inter-processor memory synchronization primitives on code that runs
>>> on the same CPU, so remove the unneeded memory barriers.
>>>
>>> The arrays are modified asynchronously (in the interrupt handler) and it is
>>> possible for the compiler to infer that they won't be changed during normal
>>> program flow and try to perform harmful optimizations (like stashing a
>>> previous read in a register and reusing it). To prevent this, for GICv2,
>>> the smp_wmb() in gicv2_ipi_send_self() is replaced with a compiler barrier.
>>> For GICv3, the wmb() barrier in gic_ipi_send_single() already implies a
>>> compiler barrier.
>>>
>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>> ---
>>>  arm/gic.c | 8 ++++----
>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index 401ffafe4299..4e947e8516a2 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -12,6 +12,7 @@
>>>   * This work is licensed under the terms of the GNU LGPL, version 2.
>>>   */
>>>  #include <libcflat.h>
>>> +#include <linux/compiler.h>
>>>  #include <errata.h>
>>>  #include <asm/setup.h>
>>>  #include <asm/processor.h>
>>> @@ -260,7 +261,8 @@ static void check_lpi_hits(int *expected, const char *msg)
>>>  
>>>  static void gicv2_ipi_send_self(void)
>>>  {> -	smp_wmb();
>> nit: previous patch added it and this patch removes it. maybe squash the
>> modifs into the previous patch saying only a barrier() is needed for self()?
> You're right, this does look out of place. I'll merge this change into the
> previous patch.
>>> +	/* Prevent the compiler from optimizing memory accesses */
>>> +	barrier();
>>>  	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
>>>  }
>>>  
>>> @@ -359,6 +361,7 @@ static struct gic gicv3 = {
>>>  	},
>>>  };
>>>  
>>> +/* Runs on the same CPU as the sender, no need for memory synchronization */
>>>  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>>  {
>>>  	u32 irqstat = gic_read_iar();
>>> @@ -375,13 +378,10 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>>  
>>>  		writel(val, base + GICD_ICACTIVER);
>>>  
>>> -		smp_rmb(); /* pairs with wmb in stats_reset */
>> the comment says it is paired with wmd in stats_reset. So is it OK to
>> leave the associated wmb?
> This patch removes multi-processor synchronization from the functions that run on
> the same CPU. stats_reset() can be called from one CPU (the IPI_SENDER CPU) and
> the variables it modifies accessed by the interrupt handlers running on different
> CPUs, like it happens for the IPI tests. In that case we do need the proper
> barriers in place.

Sorry, got confused about what you were asking. The next patch removes the
smp_wmb() from stats_reset() which became redundant after the barriers added to
the GIC functions that send IPIs. This patch is about removing barriers that were
never needed in the first place because the functions were running on the same
CPU, it's not dependent on anyGIC changes.

Thanks,
Alex
Auger Eric Dec. 3, 2020, 9:41 a.m. UTC | #4
Hi Alexandru,

On 12/2/20 3:14 PM, Alexandru Elisei wrote:
> Hi,
> 
> On 12/2/20 2:02 PM, Alexandru Elisei wrote:
> 
>> Hi Eric,
>>
>> On 12/1/20 4:37 PM, Auger Eric wrote:
>>> Hi Alexandru,
>>>
>>> On 11/25/20 4:51 PM, Alexandru Elisei wrote:
>>>> The gicv{2,3}-active test sends an IPI from the boot CPU to itself, then
>>>> checks that the interrupt has been received as expected. There is no need
>>>> to use inter-processor memory synchronization primitives on code that runs
>>>> on the same CPU, so remove the unneeded memory barriers.
>>>>
>>>> The arrays are modified asynchronously (in the interrupt handler) and it is
>>>> possible for the compiler to infer that they won't be changed during normal
>>>> program flow and try to perform harmful optimizations (like stashing a
>>>> previous read in a register and reusing it). To prevent this, for GICv2,
>>>> the smp_wmb() in gicv2_ipi_send_self() is replaced with a compiler barrier.
>>>> For GICv3, the wmb() barrier in gic_ipi_send_single() already implies a
>>>> compiler barrier.
>>>>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>>  arm/gic.c | 8 ++++----
>>>>  1 file changed, 4 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>> index 401ffafe4299..4e947e8516a2 100644
>>>> --- a/arm/gic.c
>>>> +++ b/arm/gic.c
>>>> @@ -12,6 +12,7 @@
>>>>   * This work is licensed under the terms of the GNU LGPL, version 2.
>>>>   */
>>>>  #include <libcflat.h>
>>>> +#include <linux/compiler.h>
>>>>  #include <errata.h>
>>>>  #include <asm/setup.h>
>>>>  #include <asm/processor.h>
>>>> @@ -260,7 +261,8 @@ static void check_lpi_hits(int *expected, const char *msg)
>>>>  
>>>>  static void gicv2_ipi_send_self(void)
>>>>  {> -	smp_wmb();
>>> nit: previous patch added it and this patch removes it. maybe squash the
>>> modifs into the previous patch saying only a barrier() is needed for self()?
>> You're right, this does look out of place. I'll merge this change into the
>> previous patch.
>>>> +	/* Prevent the compiler from optimizing memory accesses */
>>>> +	barrier();
>>>>  	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
>>>>  }
>>>>  
>>>> @@ -359,6 +361,7 @@ static struct gic gicv3 = {
>>>>  	},
>>>>  };
>>>>  
>>>> +/* Runs on the same CPU as the sender, no need for memory synchronization */
>>>>  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>>>  {
>>>>  	u32 irqstat = gic_read_iar();
>>>> @@ -375,13 +378,10 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>>>>  
>>>>  		writel(val, base + GICD_ICACTIVER);
>>>>  
>>>> -		smp_rmb(); /* pairs with wmb in stats_reset */
>>> the comment says it is paired with wmd in stats_reset. So is it OK to
>>> leave the associated wmb?
>> This patch removes multi-processor synchronization from the functions that run on
>> the same CPU. stats_reset() can be called from one CPU (the IPI_SENDER CPU) and
>> the variables it modifies accessed by the interrupt handlers running on different
>> CPUs, like it happens for the IPI tests. In that case we do need the proper
>> barriers in place.
> 
> Sorry, got confused about what you were asking. The next patch removes the
> smp_wmb() from stats_reset() which became redundant after the barriers added to
> the GIC functions that send IPIs. This patch is about removing barriers that were
> never needed in the first place because the functions were running on the same
> CPU, it's not dependent on anyGIC changes.

OK I get it. I was just confused by this pairing commment as we remove
one item and not the other but that's not an issue here as we do not
need the barrier in that case.

Feel free to add my R-b w/ or wo the squash:
Reviewed-by: Eric Auger <eric.auger@redhat.com>

Thanks

Eric
> 
> Thanks,
> Alex
>
diff mbox series

Patch

diff --git a/arm/gic.c b/arm/gic.c
index 401ffafe4299..4e947e8516a2 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -12,6 +12,7 @@ 
  * This work is licensed under the terms of the GNU LGPL, version 2.
  */
 #include <libcflat.h>
+#include <linux/compiler.h>
 #include <errata.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
@@ -260,7 +261,8 @@  static void check_lpi_hits(int *expected, const char *msg)
 
 static void gicv2_ipi_send_self(void)
 {
-	smp_wmb();
+	/* Prevent the compiler from optimizing memory accesses */
+	barrier();
 	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
 }
 
@@ -359,6 +361,7 @@  static struct gic gicv3 = {
 	},
 };
 
+/* Runs on the same CPU as the sender, no need for memory synchronization */
 static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
@@ -375,13 +378,10 @@  static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
-		smp_rmb(); /* pairs with wmb in stats_reset */
 		++acked[smp_processor_id()];
 		check_irqnr(irqnr);
-		smp_wmb(); /* pairs with rmb in check_acked */
 	} else {
 		++spurious[smp_processor_id()];
-		smp_wmb();
 	}
 }