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@xxxxxxx>
> ---
> 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
