[kvm-unit-tests PATCH v2 00/12] GIC fixes and improvements

[kvm-unit-tests PATCH v2 00/12] GIC fixes and improvements

[Alexandru Elisei]

What started this series is Andre's SPI and group interrupts tests [1],
which prompted me to attempt to rewrite check_acked() so it's more flexible
and not so complicated to review. When I was doing that I noticed that the
message passing pattern for accesses to the acked, bad_irq and bad_sender
arrays didn't look quite right, and that turned into the first 7 patches of
the series. Even though the diffs are relatively small, they are not
trivial and the reviewer can skip them for the more palatable patches that
follow. I would still appreciate someone having a look at the memory
ordering fixes.

Patch #8 ("Split check_acked() into two functions") is where check_acked()
is reworked with an eye towards supporting different timeout values or
silent reporting without adding too many arguments to check_acked().

After changing the IPI tests, I turned my attention to the LPI tests, which
followed the same memory synchronization patterns, but invented their own
interrupt handler and testing functions. Instead of redoing the work that I
did for the IPI tests, I decided to convert the LPI tests to use the same
infrastructure.

I ran tests on the following machines:

- Ampere EMAG: all arm64 tests 10,000+ times (combined) under qemu and
  kvmtool.

- rockpro64: the arm GICv2 and GICv3 tests 10,000+ times under kvmtool (I
  chose kvmtool because it's faster than qemu); the arm64 gic tests (GICv2
  and GICv3) 5000+ times with qemu (didn't realize that ./run_tests.sh -g
  gic doesn't include the its tests); the arm64 GICv2 and GICv3 and ITS
  tests under kvmtool 13,000+ times.

I  didn't see any issues.

Changes in v2:

* Gathered Reviewed-by tags, thank you for the feedback!

* Modified code comments in #1 ("lib: arm/arm64: gicv3: Add missing barrier
  when sending IPIs") as per review suggestions.

* Moved the barrier() in gicv2_ipi_send_self() from #3 ("arm/arm64: gic:
  Remove memory synchronization from ipi_clear_active_handler()") to #2
  ("lib: arm/arm64: gicv2: Add missing barrier when sending IPIs").

* Renamed #3, changed "[..] Remove memory synchronization [..]" to
  "[..] Remove SMP synchronization [..]".

* Moved the removal of smp_rmb() from check_spurious() from #5 ("arm/arm64:
  gic: Use correct memory ordering for the IPI test") to patch #7
  ("arm/arm64: gic: Wait for writes to acked or spurious to complete").

* Fixed typos in #8 ("arm/arm64: gic: Split check_acked() into two
  functions").

* Patch #10 ("arm64: gic: its-trigger: Don't trigger the LPI while it is
  pending") is new. It was added to fix an issue found in v1 [2].

* Patch #11 ("lib: arm64: gic-v3-its: Add wmb() barrier before INT
  command") is also new; it was split from #12 ("arm64: gic: Use IPI test
  checking for the LPI tests") following review comments.

* Removed the now redundant call to stats_reset() from its_prerequisites()
  in #12 ("arm64: gic: Use IPI test checking for the LPI tests").

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2019-November/037853.html
[2] https://www.spinics.net/lists/kvm-arm/msg43628.html

Alexandru Elisei (12):
  lib: arm/arm64: gicv3: Add missing barrier when sending IPIs
  lib: arm/arm64: gicv2: Add missing barrier when sending IPIs
  arm/arm64: gic: Remove SMP synchronization from
    ipi_clear_active_handler()
  arm/arm64: gic: Remove unnecessary synchronization with stats_reset()
  arm/arm64: gic: Use correct memory ordering for the IPI test
  arm/arm64: gic: Check spurious and bad_sender in the active test
  arm/arm64: gic: Wait for writes to acked or spurious to complete
  arm/arm64: gic: Split check_acked() into two functions
  arm/arm64: gic: Make check_acked() more generic
  arm64: gic: its-trigger: Don't trigger the LPI while it is pending
  lib: arm64: gic-v3-its: Add wmb() barrier before INT command
  arm64: gic: Use IPI test checking for the LPI tests

 lib/arm/gic-v2.c           |   4 +
 lib/arm/gic-v3.c           |   6 +
 lib/arm64/gic-v3-its-cmd.c |   6 +
 arm/gic.c                  | 341 ++++++++++++++++++++-----------------
 4 files changed, 197 insertions(+), 160 deletions(-)

-- 
2.29.2

[kvm-unit-tests PATCH v2 00/12] GIC fixes and improvements

[Alexandru Elisei]

What started this series is Andre's SPI and group interrupts tests [1],
which prompted me to attempt to rewrite check_acked() so it's more flexible
and not so complicated to review. When I was doing that I noticed that the
message passing pattern for accesses to the acked, bad_irq and bad_sender
arrays didn't look quite right, and that turned into the first 7 patches of
the series. Even though the diffs are relatively small, they are not
trivial and the reviewer can skip them for the more palatable patches that
follow. I would still appreciate someone having a look at the memory
ordering fixes.

Patch #8 ("Split check_acked() into two functions") is where check_acked()
is reworked with an eye towards supporting different timeout values or
silent reporting without adding too many arguments to check_acked().

After changing the IPI tests, I turned my attention to the LPI tests, which
followed the same memory synchronization patterns, but invented their own
interrupt handler and testing functions. Instead of redoing the work that I
did for the IPI tests, I decided to convert the LPI tests to use the same
infrastructure.

I ran tests on the following machines:

- Ampere EMAG: all arm64 tests 10,000+ times (combined) under qemu and
  kvmtool.

- rockpro64: the arm GICv2 and GICv3 tests 10,000+ times under kvmtool (I
  chose kvmtool because it's faster than qemu); the arm64 gic tests (GICv2
  and GICv3) 5000+ times with qemu (didn't realize that ./run_tests.sh -g
  gic doesn't include the its tests); the arm64 GICv2 and GICv3 and ITS
  tests under kvmtool 13,000+ times.

I  didn't see any issues.

Changes in v2:

* Gathered Reviewed-by tags, thank you for the feedback!

* Modified code comments in #1 ("lib: arm/arm64: gicv3: Add missing barrier
  when sending IPIs") as per review suggestions.

* Moved the barrier() in gicv2_ipi_send_self() from #3 ("arm/arm64: gic:
  Remove memory synchronization from ipi_clear_active_handler()") to #2
  ("lib: arm/arm64: gicv2: Add missing barrier when sending IPIs").

* Renamed #3, changed "[..] Remove memory synchronization [..]" to
  "[..] Remove SMP synchronization [..]".

* Moved the removal of smp_rmb() from check_spurious() from #5 ("arm/arm64:
  gic: Use correct memory ordering for the IPI test") to patch #7
  ("arm/arm64: gic: Wait for writes to acked or spurious to complete").

* Fixed typos in #8 ("arm/arm64: gic: Split check_acked() into two
  functions").

* Patch #10 ("arm64: gic: its-trigger: Don't trigger the LPI while it is
  pending") is new. It was added to fix an issue found in v1 [2].

* Patch #11 ("lib: arm64: gic-v3-its: Add wmb() barrier before INT
  command") is also new; it was split from #12 ("arm64: gic: Use IPI test
  checking for the LPI tests") following review comments.

* Removed the now redundant call to stats_reset() from its_prerequisites()
  in #12 ("arm64: gic: Use IPI test checking for the LPI tests").

[1] https://lists.cs.columbia.edu/pipermail/kvmarm/2019-November/037853.html
[2] https://www.spinics.net/lists/kvm-arm/msg43628.html

Alexandru Elisei (12):
  lib: arm/arm64: gicv3: Add missing barrier when sending IPIs
  lib: arm/arm64: gicv2: Add missing barrier when sending IPIs
  arm/arm64: gic: Remove SMP synchronization from
    ipi_clear_active_handler()
  arm/arm64: gic: Remove unnecessary synchronization with stats_reset()
  arm/arm64: gic: Use correct memory ordering for the IPI test
  arm/arm64: gic: Check spurious and bad_sender in the active test
  arm/arm64: gic: Wait for writes to acked or spurious to complete
  arm/arm64: gic: Split check_acked() into two functions
  arm/arm64: gic: Make check_acked() more generic
  arm64: gic: its-trigger: Don't trigger the LPI while it is pending
  lib: arm64: gic-v3-its: Add wmb() barrier before INT command
  arm64: gic: Use IPI test checking for the LPI tests

 lib/arm/gic-v2.c           |   4 +
 lib/arm/gic-v3.c           |   6 +
 lib/arm64/gic-v3-its-cmd.c |   6 +
 arm/gic.c                  | 341 ++++++++++++++++++++-----------------
 4 files changed, 197 insertions(+), 160 deletions(-)

-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 01/12] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs

[Alexandru Elisei]

One common usage for IPIs is for one CPU to write to a shared memory
location, send the IPI to kick another CPU, and the receiver to read from
the same location. Proper synchronization is needed to make sure that the
IPI receiver reads the most recent value and not stale data (for example,
the write from the sender CPU might still be in a store buffer).

For GICv3, IPIs are generated with a write to the ICC_SGI1R_EL1 register.
To make sure the memory stores are observable by other CPUs, we need a
wmb() barrier (DSB ST), which waits for stores to complete.

From the definition of DSB from ARM DDI 0487F.b, page B2-139:

"In addition, no instruction that appears in program order after the DSB
instruction can alter any state of the system or perform any part of its
functionality until the DSB completes other than:

- Being fetched from memory and decoded.
- Reading the general-purpose, SIMD and floating-point, Special-purpose, or
System registers that are directly or indirectly read without causing
side-effects."

Similar definition for armv7 (ARM DDI 0406C.d, page A3-150).

The DSB instruction is enough to prevent reordering of the GIC register
write which comes in program order after the memory access.

This also matches what the Linux GICv3 irqchip driver does (commit
21ec30c0ef52 ("irqchip/gic-v3: Use wmb() instead of smb_wmb() in
gic_raise_softirq()")).

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/gic-v3.c | 6 ++++++
 arm/gic.c        | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
index a7e2cb819746..2c067e4e9ba2 100644
--- a/lib/arm/gic-v3.c
+++ b/lib/arm/gic-v3.c
@@ -77,6 +77,12 @@ void gicv3_ipi_send_mask(int irq, const cpumask_t *dest)
 
 	assert(irq < 16);
 
+	/*
+	 * Ensure stores to Normal memory are visible to other CPUs before
+	 * sending the IPI.
+	 */
+	wmb();
+
 	/*
 	 * For each cpu in the mask collect its peers, which are also in
 	 * the mask, in order to form target lists.
diff --git a/arm/gic.c b/arm/gic.c
index acb060585fae..fee48f9b4ccb 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -275,6 +275,11 @@ static void gicv3_ipi_send_self(void)
 
 static void gicv3_ipi_send_broadcast(void)
 {
+	/*
+	 * Ensure stores to Normal memory are visible to other CPUs before
+	 * sending the IPI
+	 */
+	wmb();
 	gicv3_write_sgi1r(1ULL << 40 | IPI_IRQ << 24);
 	isb();
 }
-- 
2.29.2

[kvm-unit-tests PATCH v2 01/12] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs

[Alexandru Elisei]

One common usage for IPIs is for one CPU to write to a shared memory
location, send the IPI to kick another CPU, and the receiver to read from
the same location. Proper synchronization is needed to make sure that the
IPI receiver reads the most recent value and not stale data (for example,
the write from the sender CPU might still be in a store buffer).

For GICv3, IPIs are generated with a write to the ICC_SGI1R_EL1 register.
To make sure the memory stores are observable by other CPUs, we need a
wmb() barrier (DSB ST), which waits for stores to complete.

From the definition of DSB from ARM DDI 0487F.b, page B2-139:

"In addition, no instruction that appears in program order after the DSB
instruction can alter any state of the system or perform any part of its
functionality until the DSB completes other than:

- Being fetched from memory and decoded.
- Reading the general-purpose, SIMD and floating-point, Special-purpose, or
System registers that are directly or indirectly read without causing
side-effects."

Similar definition for armv7 (ARM DDI 0406C.d, page A3-150).

The DSB instruction is enough to prevent reordering of the GIC register
write which comes in program order after the memory access.

This also matches what the Linux GICv3 irqchip driver does (commit
21ec30c0ef52 ("irqchip/gic-v3: Use wmb() instead of smb_wmb() in
gic_raise_softirq()")).

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/gic-v3.c | 6 ++++++
 arm/gic.c        | 5 +++++
 2 files changed, 11 insertions(+)

diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
index a7e2cb819746..2c067e4e9ba2 100644
--- a/lib/arm/gic-v3.c
+++ b/lib/arm/gic-v3.c
@@ -77,6 +77,12 @@ void gicv3_ipi_send_mask(int irq, const cpumask_t *dest)
 
 	assert(irq < 16);
 
+	/*
+	 * Ensure stores to Normal memory are visible to other CPUs before
+	 * sending the IPI.
+	 */
+	wmb();
+
 	/*
 	 * For each cpu in the mask collect its peers, which are also in
 	 * the mask, in order to form target lists.
diff --git a/arm/gic.c b/arm/gic.c
index acb060585fae..fee48f9b4ccb 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -275,6 +275,11 @@ static void gicv3_ipi_send_self(void)
 
 static void gicv3_ipi_send_broadcast(void)
 {
+	/*
+	 * Ensure stores to Normal memory are visible to other CPUs before
+	 * sending the IPI
+	 */
+	wmb();
 	gicv3_write_sgi1r(1ULL << 40 | IPI_IRQ << 24);
 	isb();
 }
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 01/12] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> One common usage for IPIs is for one CPU to write to a shared memory
> location, send the IPI to kick another CPU, and the receiver to read from
> the same location. Proper synchronization is needed to make sure that the
> IPI receiver reads the most recent value and not stale data (for example,
> the write from the sender CPU might still be in a store buffer).
> 
> For GICv3, IPIs are generated with a write to the ICC_SGI1R_EL1 register.
> To make sure the memory stores are observable by other CPUs, we need a
> wmb() barrier (DSB ST), which waits for stores to complete.
> 
> From the definition of DSB from ARM DDI 0487F.b, page B2-139:
> 
> "In addition, no instruction that appears in program order after the DSB
> instruction can alter any state of the system or perform any part of its
> functionality until the DSB completes other than:
> 
> - Being fetched from memory and decoded.
> - Reading the general-purpose, SIMD and floating-point, Special-purpose, or
> System registers that are directly or indirectly read without causing
> side-effects."
> 
> Similar definition for armv7 (ARM DDI 0406C.d, page A3-150).
> 
> The DSB instruction is enough to prevent reordering of the GIC register
> write which comes in program order after the memory access.
> 
> This also matches what the Linux GICv3 irqchip driver does (commit
> 21ec30c0ef52 ("irqchip/gic-v3: Use wmb() instead of smb_wmb() in
> gic_raise_softirq()")).
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Yes, makes sense. Also verified the references in both the ARM ARM and
the Linux code.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  lib/arm/gic-v3.c | 6 ++++++
>  arm/gic.c        | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index a7e2cb819746..2c067e4e9ba2 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -77,6 +77,12 @@ void gicv3_ipi_send_mask(int irq, const cpumask_t *dest)
>  
>  	assert(irq < 16);
>  
> +	/*
> +	 * Ensure stores to Normal memory are visible to other CPUs before
> +	 * sending the IPI.
> +	 */
> +	wmb();
> +
>  	/*
>  	 * For each cpu in the mask collect its peers, which are also in
>  	 * the mask, in order to form target lists.
> diff --git a/arm/gic.c b/arm/gic.c
> index acb060585fae..fee48f9b4ccb 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -275,6 +275,11 @@ static void gicv3_ipi_send_self(void)
>  
>  static void gicv3_ipi_send_broadcast(void)
>  {
> +	/*
> +	 * Ensure stores to Normal memory are visible to other CPUs before
> +	 * sending the IPI
> +	 */
> +	wmb();
>  	gicv3_write_sgi1r(1ULL << 40 | IPI_IRQ << 24);
>  	isb();
>  }
> 

Re: [kvm-unit-tests PATCH v2 01/12] lib: arm/arm64: gicv3: Add missing barrier when sending IPIs

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> One common usage for IPIs is for one CPU to write to a shared memory
> location, send the IPI to kick another CPU, and the receiver to read from
> the same location. Proper synchronization is needed to make sure that the
> IPI receiver reads the most recent value and not stale data (for example,
> the write from the sender CPU might still be in a store buffer).
> 
> For GICv3, IPIs are generated with a write to the ICC_SGI1R_EL1 register.
> To make sure the memory stores are observable by other CPUs, we need a
> wmb() barrier (DSB ST), which waits for stores to complete.
> 
> From the definition of DSB from ARM DDI 0487F.b, page B2-139:
> 
> "In addition, no instruction that appears in program order after the DSB
> instruction can alter any state of the system or perform any part of its
> functionality until the DSB completes other than:
> 
> - Being fetched from memory and decoded.
> - Reading the general-purpose, SIMD and floating-point, Special-purpose, or
> System registers that are directly or indirectly read without causing
> side-effects."
> 
> Similar definition for armv7 (ARM DDI 0406C.d, page A3-150).
> 
> The DSB instruction is enough to prevent reordering of the GIC register
> write which comes in program order after the memory access.
> 
> This also matches what the Linux GICv3 irqchip driver does (commit
> 21ec30c0ef52 ("irqchip/gic-v3: Use wmb() instead of smb_wmb() in
> gic_raise_softirq()")).
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Yes, makes sense. Also verified the references in both the ARM ARM and
the Linux code.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  lib/arm/gic-v3.c | 6 ++++++
>  arm/gic.c        | 5 +++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index a7e2cb819746..2c067e4e9ba2 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -77,6 +77,12 @@ void gicv3_ipi_send_mask(int irq, const cpumask_t *dest)
>  
>  	assert(irq < 16);
>  
> +	/*
> +	 * Ensure stores to Normal memory are visible to other CPUs before
> +	 * sending the IPI.
> +	 */
> +	wmb();
> +
>  	/*
>  	 * For each cpu in the mask collect its peers, which are also in
>  	 * the mask, in order to form target lists.
> diff --git a/arm/gic.c b/arm/gic.c
> index acb060585fae..fee48f9b4ccb 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -275,6 +275,11 @@ static void gicv3_ipi_send_self(void)
>  
>  static void gicv3_ipi_send_broadcast(void)
>  {
> +	/*
> +	 * Ensure stores to Normal memory are visible to other CPUs before
> +	 * sending the IPI
> +	 */
> +	wmb();
>  	gicv3_write_sgi1r(1ULL << 40 | IPI_IRQ << 24);
>  	isb();
>  }
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 02/12] lib: arm/arm64: gicv2: Add missing barrier when sending IPIs

[Alexandru Elisei]

GICv2 generates IPIs with a MMIO write to the GICD_SGIR register. A common
pattern for IPI usage is for the IPI receiver to read data written to
memory by the sender. The armv7 and armv8 architectures implement a
weakly-ordered memory model, which means that barriers are required to make
sure that the expected values are observed.

It turns out that because the receiver CPU must observe the write to memory
that generated the IPI when reading the GICC_IAR MMIO register, we only
need to ensure ordering of memory accesses, and not completion. Use a
smp_wmb (DMB ISHST) barrier before sending the IPI.

This also matches what the Linux GICv2 irqchip driver does (more details
in commit 8adbf57fc429 ("irqchip: gic: use dmb ishst instead of dsb when
raising a softirq")).

The gicv2_ipi_send_self() function sends an IPI from a CPU to itself.
The tests that use this function rely on the interrupt handler to record
information about the interrupt by using several arrays. It is possible
for the compiler to infer that the arrays 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,
a compile barrier is added to gicv2_ipi_send_self(). For GICv3, the
wmb() barrier in gic_ipi_send_single() (which is also used when a CPU
sends an IPI to itself) already implies a compiler barrier.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/gic-v2.c | 4 ++++
 arm/gic.c        | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
index dc6a97c600ec..da244c82de34 100644
--- a/lib/arm/gic-v2.c
+++ b/lib/arm/gic-v2.c
@@ -45,6 +45,8 @@ void gicv2_ipi_send_single(int irq, int cpu)
 {
 	assert(cpu < 8);
 	assert(irq < 16);
+
+	smp_wmb();
 	writel(1 << (cpu + 16) | irq, gicv2_dist_base() + GICD_SGIR);
 }
 
@@ -53,5 +55,7 @@ void gicv2_ipi_send_mask(int irq, const cpumask_t *dest)
 	u8 tlist = (u8)cpumask_bits(dest)[0];
 
 	assert(irq < 16);
+
+	smp_wmb();
 	writel(tlist << 16 | irq, gicv2_dist_base() + GICD_SGIR);
 }
diff --git a/arm/gic.c b/arm/gic.c
index fee48f9b4ccb..ca61dba2986c 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -13,6 +13,7 @@
  */
 #include <libcflat.h>
 #include <errata.h>
+#include <linux/compiler.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
 #include <asm/delay.h>
@@ -260,11 +261,14 @@ static void check_lpi_hits(int *expected, const char *msg)
 
 static void gicv2_ipi_send_self(void)
 {
+	/* Prevent the compiler from optimizing memory accesses */
+	barrier();
 	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
 }
 
 static void gicv2_ipi_send_broadcast(void)
 {
+	smp_wmb();
 	writel(1 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
 }
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 02/12] lib: arm/arm64: gicv2: Add missing barrier when sending IPIs

[Alexandru Elisei]

GICv2 generates IPIs with a MMIO write to the GICD_SGIR register. A common
pattern for IPI usage is for the IPI receiver to read data written to
memory by the sender. The armv7 and armv8 architectures implement a
weakly-ordered memory model, which means that barriers are required to make
sure that the expected values are observed.

It turns out that because the receiver CPU must observe the write to memory
that generated the IPI when reading the GICC_IAR MMIO register, we only
need to ensure ordering of memory accesses, and not completion. Use a
smp_wmb (DMB ISHST) barrier before sending the IPI.

This also matches what the Linux GICv2 irqchip driver does (more details
in commit 8adbf57fc429 ("irqchip: gic: use dmb ishst instead of dsb when
raising a softirq")).

The gicv2_ipi_send_self() function sends an IPI from a CPU to itself.
The tests that use this function rely on the interrupt handler to record
information about the interrupt by using several arrays. It is possible
for the compiler to infer that the arrays 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,
a compile barrier is added to gicv2_ipi_send_self(). For GICv3, the
wmb() barrier in gic_ipi_send_single() (which is also used when a CPU
sends an IPI to itself) already implies a compiler barrier.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm/gic-v2.c | 4 ++++
 arm/gic.c        | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/lib/arm/gic-v2.c b/lib/arm/gic-v2.c
index dc6a97c600ec..da244c82de34 100644
--- a/lib/arm/gic-v2.c
+++ b/lib/arm/gic-v2.c
@@ -45,6 +45,8 @@ void gicv2_ipi_send_single(int irq, int cpu)
 {
 	assert(cpu < 8);
 	assert(irq < 16);
+
+	smp_wmb();
 	writel(1 << (cpu + 16) | irq, gicv2_dist_base() + GICD_SGIR);
 }
 
@@ -53,5 +55,7 @@ void gicv2_ipi_send_mask(int irq, const cpumask_t *dest)
 	u8 tlist = (u8)cpumask_bits(dest)[0];
 
 	assert(irq < 16);
+
+	smp_wmb();
 	writel(tlist << 16 | irq, gicv2_dist_base() + GICD_SGIR);
 }
diff --git a/arm/gic.c b/arm/gic.c
index fee48f9b4ccb..ca61dba2986c 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -13,6 +13,7 @@
  */
 #include <libcflat.h>
 #include <errata.h>
+#include <linux/compiler.h>
 #include <asm/setup.h>
 #include <asm/processor.h>
 #include <asm/delay.h>
@@ -260,11 +261,14 @@ static void check_lpi_hits(int *expected, const char *msg)
 
 static void gicv2_ipi_send_self(void)
 {
+	/* Prevent the compiler from optimizing memory accesses */
+	barrier();
 	writel(2 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
 }
 
 static void gicv2_ipi_send_broadcast(void)
 {
+	smp_wmb();
 	writel(1 << 24 | IPI_IRQ, gicv2_dist_base() + GICD_SGIR);
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 03/12] arm/arm64: gic: Remove SMP synchronization from ipi_clear_active_handler()

[Alexandru Elisei]

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.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index ca61dba2986c..1c9f4a01b6e4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -364,6 +364,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();
@@ -380,13 +381,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();
 	}
 }
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 03/12] arm/arm64: gic: Remove SMP synchronization from ipi_clear_active_handler()

[Alexandru Elisei]

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.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index ca61dba2986c..1c9f4a01b6e4 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -364,6 +364,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();
@@ -380,13 +381,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();
 	}
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 03/12] arm/arm64: gic: Remove SMP synchronization from ipi_clear_active_handler()

[André Przywara]

On 17/12/2020 14:13, 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.
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

I am not fully convinced this is a wise move. Indeed the existing
barriers look wrong, and *currently* we just use
ipi_clear_active_handler() for a single CPU test only, but the handler
function itself is not restricted to that use case, if I am not mistaken.
But I think for now this fix is fine, and the comment should point out
the limitation well enough, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
>  arm/gic.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ca61dba2986c..1c9f4a01b6e4 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -364,6 +364,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();
> @@ -380,13 +381,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();
>  	}
>  }
>  
> 

Re: [kvm-unit-tests PATCH v2 03/12] arm/arm64: gic: Remove SMP synchronization from ipi_clear_active_handler()

[André Przywara]

On 17/12/2020 14:13, 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.
> 
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

I am not fully convinced this is a wise move. Indeed the existing
barriers look wrong, and *currently* we just use
ipi_clear_active_handler() for a single CPU test only, but the handler
function itself is not restricted to that use case, if I am not mistaken.
But I think for now this fix is fine, and the comment should point out
the limitation well enough, so:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
>  arm/gic.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ca61dba2986c..1c9f4a01b6e4 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -364,6 +364,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();
> @@ -380,13 +381,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();
>  	}
>  }
>  
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 04/12] arm/arm64: gic: Remove unnecessary synchronization with stats_reset()

[Alexandru Elisei]

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().

Reviewed-by: Eric Auger <eric.auger@redhat.com>
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 1c9f4a01b6e4..d25529dd8b79 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);
-- 
2.29.2

[kvm-unit-tests PATCH v2 04/12] arm/arm64: gic: Remove unnecessary synchronization with stats_reset()

[Alexandru Elisei]

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().

Reviewed-by: Eric Auger <eric.auger@redhat.com>
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 1c9f4a01b6e4..d25529dd8b79 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);
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 05/12] arm/arm64: gic: Use correct memory ordering for the IPI test

[Alexandru Elisei]

The IPI test works by sending IPIs to even numbered CPUs from the
IPI_SENDER CPU (CPU1), and then checking that the other CPUs received the
interrupts as expected. The check is done in check_acked() by the
IPI_SENDER CPU with the help of three arrays:

- acked, where acked[i] == 1 means that CPU i received the interrupt.
- bad_irq, where bad_irq[i] == -1 means that the interrupt received by CPU
  i had the expected interrupt number (IPI_IRQ).
- bad_sender, where bad_sender[i] == -1 means that the interrupt received
  by CPU i was from the expected sender (IPI_SENDER, GICv2 only).

The assumption made by check_acked() is that if a CPU acked an interrupt,
then bad_sender and bad_irq have also been updated. This is a common
inter-thread communication pattern called message passing.  For message
passing to work correctly on weakly consistent memory model architectures,
like arm and arm64, barriers or address dependencies are required. This is
described in ARM DDI 0487F.b, in "Armv7 compatible approaches for ordering,
using DMB and DSB barriers" (page K11-7993), in the section with a single
observer, which is in our case the IPI_SENDER CPU.

The IPI test attempts to enforce the correct ordering using memory
barriers, but it's not enough. For example, the program execution below is
valid from an architectural point of view:

3 online CPUs, initial state (from stats_reset()):

acked[2] = 0;
bad_sender[2] = -1;
bad_irq[2] = -1;

CPU1 (in check_acked())		| CPU2 (in ipi_handler())
				|
smp_rmb() // DMB ISHLD		| acked[2]++;
read 1 from acked[2]		|
nr_pass++ // nr_pass = 3	|
read -1 from bad_sender[2]	|
read -1 from bad_irq[2]		|
				| // in check_ipi_sender()
				| bad_sender[2] = <bad ipi sender>
				| // in check_irqnr()
				| bad_irq[2] = <bad irq number>
				| smp_wmb() // DMB ISHST
nr_pass == nr_cpus, return	|

In this scenario, CPU1 will read the updated acked value, but it will read
the initial bad_sender and bad_irq values. This is permitted because the
memory barriers do not create a data dependency between the value read from
acked and the values read from bad_rq and bad_sender on CPU1, respectively
between the values written to acked, bad_sender and bad_irq on CPU2.

To avoid this situation, let's reorder the barriers and accesses to the
arrays to create the needed dependencies that ensure that message passing
behaves as expected.

In the interrupt handler, the writes to bad_sender and bad_irq are
reordered before the write to acked and a smp_wmb() barrier is added. This
ensures that if other PEs observe the write to acked, then they will also
observe the writes to the other two arrays.

In check_acked(), put the smp_rmb() barrier after the read from acked to
ensure that the subsequent reads from bad_sender, respectively bad_irq,
aren't reordered locally by the PE.

With these changes, the expected ordering of accesses is respected and we
end up with the pattern described in the Arm ARM and also in the Linux
litmus test MP+fencewmbonceonce+fencermbonceonce.litmus from
tools/memory-model/litmus-tests. More examples and explanations can be
found in the Linux source tree, in Documentation/memory-barriers.txt, in
the sections "SMP BARRIER PAIRING" and "READ MEMORY BARRIERS VS LOAD
SPECULATION".

For consistency with ipi_handler(), the array accesses in
ipi_clear_active_handler() have also been reordered. This shouldn't affect
the functionality of that test.

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 d25529dd8b79..34643a73bd04 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -73,9 +73,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
 		mdelay(100);
 		nr_pass = 0;
 		for_each_present_cpu(cpu) {
-			smp_rmb();
 			nr_pass += cpumask_test_cpu(cpu, mask) ?
 				acked[cpu] == 1 : acked[cpu] == 0;
+			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
 
 			if (bad_sender[cpu] != -1) {
 				printf("cpu%d received IPI from wrong sender %d\n",
@@ -156,10 +156,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		++acked[smp_processor_id()];
 		check_ipi_sender(irqstat);
 		check_irqnr(irqnr);
-		smp_wmb(); /* pairs with rmb in check_acked */
+		smp_wmb(); /* pairs with smp_rmb in check_acked */
+		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
 		smp_wmb();
@@ -386,8 +386,8 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
-		++acked[smp_processor_id()];
 		check_irqnr(irqnr);
+		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
 	}
-- 
2.29.2

[kvm-unit-tests PATCH v2 05/12] arm/arm64: gic: Use correct memory ordering for the IPI test

[Alexandru Elisei]

The IPI test works by sending IPIs to even numbered CPUs from the
IPI_SENDER CPU (CPU1), and then checking that the other CPUs received the
interrupts as expected. The check is done in check_acked() by the
IPI_SENDER CPU with the help of three arrays:

- acked, where acked[i] == 1 means that CPU i received the interrupt.
- bad_irq, where bad_irq[i] == -1 means that the interrupt received by CPU
  i had the expected interrupt number (IPI_IRQ).
- bad_sender, where bad_sender[i] == -1 means that the interrupt received
  by CPU i was from the expected sender (IPI_SENDER, GICv2 only).

The assumption made by check_acked() is that if a CPU acked an interrupt,
then bad_sender and bad_irq have also been updated. This is a common
inter-thread communication pattern called message passing.  For message
passing to work correctly on weakly consistent memory model architectures,
like arm and arm64, barriers or address dependencies are required. This is
described in ARM DDI 0487F.b, in "Armv7 compatible approaches for ordering,
using DMB and DSB barriers" (page K11-7993), in the section with a single
observer, which is in our case the IPI_SENDER CPU.

The IPI test attempts to enforce the correct ordering using memory
barriers, but it's not enough. For example, the program execution below is
valid from an architectural point of view:

3 online CPUs, initial state (from stats_reset()):

acked[2] = 0;
bad_sender[2] = -1;
bad_irq[2] = -1;

CPU1 (in check_acked())		| CPU2 (in ipi_handler())
				|
smp_rmb() // DMB ISHLD		| acked[2]++;
read 1 from acked[2]		|
nr_pass++ // nr_pass = 3	|
read -1 from bad_sender[2]	|
read -1 from bad_irq[2]		|
				| // in check_ipi_sender()
				| bad_sender[2] = <bad ipi sender>
				| // in check_irqnr()
				| bad_irq[2] = <bad irq number>
				| smp_wmb() // DMB ISHST
nr_pass == nr_cpus, return	|

In this scenario, CPU1 will read the updated acked value, but it will read
the initial bad_sender and bad_irq values. This is permitted because the
memory barriers do not create a data dependency between the value read from
acked and the values read from bad_rq and bad_sender on CPU1, respectively
between the values written to acked, bad_sender and bad_irq on CPU2.

To avoid this situation, let's reorder the barriers and accesses to the
arrays to create the needed dependencies that ensure that message passing
behaves as expected.

In the interrupt handler, the writes to bad_sender and bad_irq are
reordered before the write to acked and a smp_wmb() barrier is added. This
ensures that if other PEs observe the write to acked, then they will also
observe the writes to the other two arrays.

In check_acked(), put the smp_rmb() barrier after the read from acked to
ensure that the subsequent reads from bad_sender, respectively bad_irq,
aren't reordered locally by the PE.

With these changes, the expected ordering of accesses is respected and we
end up with the pattern described in the Arm ARM and also in the Linux
litmus test MP+fencewmbonceonce+fencermbonceonce.litmus from
tools/memory-model/litmus-tests. More examples and explanations can be
found in the Linux source tree, in Documentation/memory-barriers.txt, in
the sections "SMP BARRIER PAIRING" and "READ MEMORY BARRIERS VS LOAD
SPECULATION".

For consistency with ipi_handler(), the array accesses in
ipi_clear_active_handler() have also been reordered. This shouldn't affect
the functionality of that test.

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 d25529dd8b79..34643a73bd04 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -73,9 +73,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
 		mdelay(100);
 		nr_pass = 0;
 		for_each_present_cpu(cpu) {
-			smp_rmb();
 			nr_pass += cpumask_test_cpu(cpu, mask) ?
 				acked[cpu] == 1 : acked[cpu] == 0;
+			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
 
 			if (bad_sender[cpu] != -1) {
 				printf("cpu%d received IPI from wrong sender %d\n",
@@ -156,10 +156,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		++acked[smp_processor_id()];
 		check_ipi_sender(irqstat);
 		check_irqnr(irqnr);
-		smp_wmb(); /* pairs with rmb in check_acked */
+		smp_wmb(); /* pairs with smp_rmb in check_acked */
+		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
 		smp_wmb();
@@ -386,8 +386,8 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
-		++acked[smp_processor_id()];
 		check_irqnr(irqnr);
+		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
 	}
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 05/12] arm/arm64: gic: Use correct memory ordering for the IPI test

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The IPI test works by sending IPIs to even numbered CPUs from the
> IPI_SENDER CPU (CPU1), and then checking that the other CPUs received the
> interrupts as expected. The check is done in check_acked() by the
> IPI_SENDER CPU with the help of three arrays:
> 
> - acked, where acked[i] == 1 means that CPU i received the interrupt.
> - bad_irq, where bad_irq[i] == -1 means that the interrupt received by CPU
>   i had the expected interrupt number (IPI_IRQ).
> - bad_sender, where bad_sender[i] == -1 means that the interrupt received
>   by CPU i was from the expected sender (IPI_SENDER, GICv2 only).
> 
> The assumption made by check_acked() is that if a CPU acked an interrupt,
> then bad_sender and bad_irq have also been updated. This is a common
> inter-thread communication pattern called message passing.  For message
> passing to work correctly on weakly consistent memory model architectures,
> like arm and arm64, barriers or address dependencies are required. This is
> described in ARM DDI 0487F.b, in "Armv7 compatible approaches for ordering,
> using DMB and DSB barriers" (page K11-7993), in the section with a single
> observer, which is in our case the IPI_SENDER CPU.
> 
> The IPI test attempts to enforce the correct ordering using memory
> barriers, but it's not enough. For example, the program execution below is
> valid from an architectural point of view:
> 
> 3 online CPUs, initial state (from stats_reset()):
> 
> acked[2] = 0;
> bad_sender[2] = -1;
> bad_irq[2] = -1;
> 
> CPU1 (in check_acked())		| CPU2 (in ipi_handler())
> 				|
> smp_rmb() // DMB ISHLD		| acked[2]++;
> read 1 from acked[2]		|
> nr_pass++ // nr_pass = 3	|
> read -1 from bad_sender[2]	|
> read -1 from bad_irq[2]		|
> 				| // in check_ipi_sender()
> 				| bad_sender[2] = <bad ipi sender>
> 				| // in check_irqnr()
> 				| bad_irq[2] = <bad irq number>
> 				| smp_wmb() // DMB ISHST
> nr_pass == nr_cpus, return	|
> 
> In this scenario, CPU1 will read the updated acked value, but it will read
> the initial bad_sender and bad_irq values. This is permitted because the
> memory barriers do not create a data dependency between the value read from
> acked and the values read from bad_rq and bad_sender on CPU1, respectively
> between the values written to acked, bad_sender and bad_irq on CPU2.
> 
> To avoid this situation, let's reorder the barriers and accesses to the
> arrays to create the needed dependencies that ensure that message passing
> behaves as expected.
> 
> In the interrupt handler, the writes to bad_sender and bad_irq are
> reordered before the write to acked and a smp_wmb() barrier is added. This
> ensures that if other PEs observe the write to acked, then they will also
> observe the writes to the other two arrays.
> 
> In check_acked(), put the smp_rmb() barrier after the read from acked to
> ensure that the subsequent reads from bad_sender, respectively bad_irq,
> aren't reordered locally by the PE.
> 
> With these changes, the expected ordering of accesses is respected and we
> end up with the pattern described in the Arm ARM and also in the Linux
> litmus test MP+fencewmbonceonce+fencermbonceonce.litmus from
> tools/memory-model/litmus-tests. More examples and explanations can be
> found in the Linux source tree, in Documentation/memory-barriers.txt, in
> the sections "SMP BARRIER PAIRING" and "READ MEMORY BARRIERS VS LOAD
> SPECULATION".
> 
> For consistency with ipi_handler(), the array accesses in
> ipi_clear_active_handler() have also been reordered. This shouldn't affect
> the functionality of that test.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Yes, this is indeed a real ordering bug. Good find and analysis!

Meditated over the new code for a while and it now looks right to me.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
>  arm/gic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index d25529dd8b79..34643a73bd04 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -73,9 +73,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> -			smp_rmb();
>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>  				acked[cpu] == 1 : acked[cpu] == 0;
> +			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>  
>  			if (bad_sender[cpu] != -1) {
>  				printf("cpu%d received IPI from wrong sender %d\n",
> @@ -156,10 +156,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  		 */
>  		if (gic_version() == 2)
>  			smp_rmb();
> -		++acked[smp_processor_id()];
>  		check_ipi_sender(irqstat);
>  		check_irqnr(irqnr);
> -		smp_wmb(); /* pairs with rmb in check_acked */
> +		smp_wmb(); /* pairs with smp_rmb in check_acked */
> +		++acked[smp_processor_id()];
>  	} else {
>  		++spurious[smp_processor_id()];
>  		smp_wmb();
> @@ -386,8 +386,8 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -		++acked[smp_processor_id()];
>  		check_irqnr(irqnr);
> +		++acked[smp_processor_id()];
>  	} else {
>  		++spurious[smp_processor_id()];
>  	}
> 

Re: [kvm-unit-tests PATCH v2 05/12] arm/arm64: gic: Use correct memory ordering for the IPI test

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The IPI test works by sending IPIs to even numbered CPUs from the
> IPI_SENDER CPU (CPU1), and then checking that the other CPUs received the
> interrupts as expected. The check is done in check_acked() by the
> IPI_SENDER CPU with the help of three arrays:
> 
> - acked, where acked[i] == 1 means that CPU i received the interrupt.
> - bad_irq, where bad_irq[i] == -1 means that the interrupt received by CPU
>   i had the expected interrupt number (IPI_IRQ).
> - bad_sender, where bad_sender[i] == -1 means that the interrupt received
>   by CPU i was from the expected sender (IPI_SENDER, GICv2 only).
> 
> The assumption made by check_acked() is that if a CPU acked an interrupt,
> then bad_sender and bad_irq have also been updated. This is a common
> inter-thread communication pattern called message passing.  For message
> passing to work correctly on weakly consistent memory model architectures,
> like arm and arm64, barriers or address dependencies are required. This is
> described in ARM DDI 0487F.b, in "Armv7 compatible approaches for ordering,
> using DMB and DSB barriers" (page K11-7993), in the section with a single
> observer, which is in our case the IPI_SENDER CPU.
> 
> The IPI test attempts to enforce the correct ordering using memory
> barriers, but it's not enough. For example, the program execution below is
> valid from an architectural point of view:
> 
> 3 online CPUs, initial state (from stats_reset()):
> 
> acked[2] = 0;
> bad_sender[2] = -1;
> bad_irq[2] = -1;
> 
> CPU1 (in check_acked())		| CPU2 (in ipi_handler())
> 				|
> smp_rmb() // DMB ISHLD		| acked[2]++;
> read 1 from acked[2]		|
> nr_pass++ // nr_pass = 3	|
> read -1 from bad_sender[2]	|
> read -1 from bad_irq[2]		|
> 				| // in check_ipi_sender()
> 				| bad_sender[2] = <bad ipi sender>
> 				| // in check_irqnr()
> 				| bad_irq[2] = <bad irq number>
> 				| smp_wmb() // DMB ISHST
> nr_pass == nr_cpus, return	|
> 
> In this scenario, CPU1 will read the updated acked value, but it will read
> the initial bad_sender and bad_irq values. This is permitted because the
> memory barriers do not create a data dependency between the value read from
> acked and the values read from bad_rq and bad_sender on CPU1, respectively
> between the values written to acked, bad_sender and bad_irq on CPU2.
> 
> To avoid this situation, let's reorder the barriers and accesses to the
> arrays to create the needed dependencies that ensure that message passing
> behaves as expected.
> 
> In the interrupt handler, the writes to bad_sender and bad_irq are
> reordered before the write to acked and a smp_wmb() barrier is added. This
> ensures that if other PEs observe the write to acked, then they will also
> observe the writes to the other two arrays.
> 
> In check_acked(), put the smp_rmb() barrier after the read from acked to
> ensure that the subsequent reads from bad_sender, respectively bad_irq,
> aren't reordered locally by the PE.
> 
> With these changes, the expected ordering of accesses is respected and we
> end up with the pattern described in the Arm ARM and also in the Linux
> litmus test MP+fencewmbonceonce+fencermbonceonce.litmus from
> tools/memory-model/litmus-tests. More examples and explanations can be
> found in the Linux source tree, in Documentation/memory-barriers.txt, in
> the sections "SMP BARRIER PAIRING" and "READ MEMORY BARRIERS VS LOAD
> SPECULATION".
> 
> For consistency with ipi_handler(), the array accesses in
> ipi_clear_active_handler() have also been reordered. This shouldn't affect
> the functionality of that test.
> 
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Yes, this is indeed a real ordering bug. Good find and analysis!

Meditated over the new code for a while and it now looks right to me.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


> ---
>  arm/gic.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index d25529dd8b79..34643a73bd04 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -73,9 +73,9 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> -			smp_rmb();
>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>  				acked[cpu] == 1 : acked[cpu] == 0;
> +			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>  
>  			if (bad_sender[cpu] != -1) {
>  				printf("cpu%d received IPI from wrong sender %d\n",
> @@ -156,10 +156,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  		 */
>  		if (gic_version() == 2)
>  			smp_rmb();
> -		++acked[smp_processor_id()];
>  		check_ipi_sender(irqstat);
>  		check_irqnr(irqnr);
> -		smp_wmb(); /* pairs with rmb in check_acked */
> +		smp_wmb(); /* pairs with smp_rmb in check_acked */
> +		++acked[smp_processor_id()];
>  	} else {
>  		++spurious[smp_processor_id()];
>  		smp_wmb();
> @@ -386,8 +386,8 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -		++acked[smp_processor_id()];
>  		check_irqnr(irqnr);
> +		++acked[smp_processor_id()];
>  	} else {
>  		++spurious[smp_processor_id()];
>  	}
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 06/12] arm/arm64: gic: Check spurious and bad_sender in the active test

[Alexandru Elisei]

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. The
ipi_clear_active_handler() clears the active state of the interrupt with a
write to the GICD_ICACTIVER register instead of writing the to EOI
register.

When acknowledging the interrupt it is possible to get back an spurious
interrupt ID (ID 1023), and the interrupt handler increments the number of
spurious interrupts received on the current processor. However, this is not
checked at the end of the test. Let's also check for spurious interrupts,
like the IPI test does.

For IPIs on GICv2, the value returned by a read of the GICC_IAR register
performed when acknowledging the interrupt also contains the sender CPU
ID. Add a check for that too.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 34643a73bd04..a42b5afdba65 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -126,12 +126,12 @@ static void check_spurious(void)
 	}
 }
 
-static void check_ipi_sender(u32 irqstat)
+static void check_ipi_sender(u32 irqstat, int sender)
 {
 	if (gic_version() == 2) {
 		int src = (irqstat >> 10) & 7;
 
-		if (src != IPI_SENDER)
+		if (src != sender)
 			bad_sender[smp_processor_id()] = src;
 	}
 }
@@ -156,7 +156,7 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		check_ipi_sender(irqstat);
+		check_ipi_sender(irqstat, IPI_SENDER);
 		check_irqnr(irqnr);
 		smp_wmb(); /* pairs with smp_rmb in check_acked */
 		++acked[smp_processor_id()];
@@ -386,6 +386,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
+		check_ipi_sender(irqstat, smp_processor_id());
 		check_irqnr(irqnr);
 		++acked[smp_processor_id()];
 	} else {
@@ -398,6 +399,7 @@ static void run_active_clear_test(void)
 	report_prefix_push("active");
 	setup_irq(ipi_clear_active_handler);
 	ipi_test_self();
+	check_spurious();
 	report_prefix_pop();
 }
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 06/12] arm/arm64: gic: Check spurious and bad_sender in the active test

[Alexandru Elisei]

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. The
ipi_clear_active_handler() clears the active state of the interrupt with a
write to the GICD_ICACTIVER register instead of writing the to EOI
register.

When acknowledging the interrupt it is possible to get back an spurious
interrupt ID (ID 1023), and the interrupt handler increments the number of
spurious interrupts received on the current processor. However, this is not
checked at the end of the test. Let's also check for spurious interrupts,
like the IPI test does.

For IPIs on GICv2, the value returned by a read of the GICC_IAR register
performed when acknowledging the interrupt also contains the sender CPU
ID. Add a check for that too.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index 34643a73bd04..a42b5afdba65 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -126,12 +126,12 @@ static void check_spurious(void)
 	}
 }
 
-static void check_ipi_sender(u32 irqstat)
+static void check_ipi_sender(u32 irqstat, int sender)
 {
 	if (gic_version() == 2) {
 		int src = (irqstat >> 10) & 7;
 
-		if (src != IPI_SENDER)
+		if (src != sender)
 			bad_sender[smp_processor_id()] = src;
 	}
 }
@@ -156,7 +156,7 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		check_ipi_sender(irqstat);
+		check_ipi_sender(irqstat, IPI_SENDER);
 		check_irqnr(irqnr);
 		smp_wmb(); /* pairs with smp_rmb in check_acked */
 		++acked[smp_processor_id()];
@@ -386,6 +386,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
+		check_ipi_sender(irqstat, smp_processor_id());
 		check_irqnr(irqnr);
 		++acked[smp_processor_id()];
 	} else {
@@ -398,6 +399,7 @@ static void run_active_clear_test(void)
 	report_prefix_push("active");
 	setup_irq(ipi_clear_active_handler);
 	ipi_test_self();
+	check_spurious();
 	report_prefix_pop();
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 07/12] arm/arm64: gic: Wait for writes to acked or spurious to complete

[Alexandru Elisei]

The IPI test has two parts: in the first part, it tests that the sender CPU
can send an IPI to itself (ipi_test_self()), and in the second part it
sends interrupts to even-numbered CPUs (ipi_test_smp()). When acknowledging
an interrupt, if we read back a spurious interrupt ID (1023), the handler
increments the index in the static array spurious corresponding to the CPU
ID that the handler is running on; if we get the expected interrupt ID, we
increment the same index in the acked array.

Reads of the spurious and acked arrays are synchronized with writes
performed before sending the IPI. The synchronization is done either in the
IPI sender function (GICv3), either by creating a data dependency (GICv2).

At the end of the test, the sender CPU reads from the acked and spurious
arrays to check against the expected behaviour. We need to make sure the
that writes in ipi_handler() are observable by the sender CPU. Use a DSB
ISHST to make sure that the writes have completed.

One might rightfully argue that there are no guarantees regarding when the
DSB instruction completes, just like there are no guarantees regarding when
the value is observed by the other CPUs. However, let's do our best and
instruct the CPU to complete the memory access when we know that it will be
needed.

We still need to follow the message passing pattern for the acked,
respectively bad_irq and bad_sender, because DSB guarantees that all memory
accesses that come before the barrier have completed, not that they have
completed in program order.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a42b5afdba65..ec733719c776 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -118,7 +118,6 @@ static void check_spurious(void)
 {
 	int cpu;
 
-	smp_rmb();
 	for_each_present_cpu(cpu) {
 		if (spurious[cpu])
 			report_info("WARN: cpu%d got %d spurious interrupts",
@@ -162,8 +161,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
-		smp_wmb();
 	}
+
+	/* Wait for writes to acked/spurious to complete */
+	dsb(ishst);
 }
 
 static void setup_irq(irq_handler_fn handler)
-- 
2.29.2

[kvm-unit-tests PATCH v2 07/12] arm/arm64: gic: Wait for writes to acked or spurious to complete

[Alexandru Elisei]

The IPI test has two parts: in the first part, it tests that the sender CPU
can send an IPI to itself (ipi_test_self()), and in the second part it
sends interrupts to even-numbered CPUs (ipi_test_smp()). When acknowledging
an interrupt, if we read back a spurious interrupt ID (1023), the handler
increments the index in the static array spurious corresponding to the CPU
ID that the handler is running on; if we get the expected interrupt ID, we
increment the same index in the acked array.

Reads of the spurious and acked arrays are synchronized with writes
performed before sending the IPI. The synchronization is done either in the
IPI sender function (GICv3), either by creating a data dependency (GICv2).

At the end of the test, the sender CPU reads from the acked and spurious
arrays to check against the expected behaviour. We need to make sure the
that writes in ipi_handler() are observable by the sender CPU. Use a DSB
ISHST to make sure that the writes have completed.

One might rightfully argue that there are no guarantees regarding when the
DSB instruction completes, just like there are no guarantees regarding when
the value is observed by the other CPUs. However, let's do our best and
instruct the CPU to complete the memory access when we know that it will be
needed.

We still need to follow the message passing pattern for the acked,
respectively bad_irq and bad_sender, because DSB guarantees that all memory
accesses that come before the barrier have completed, not that they have
completed in program order.

Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a42b5afdba65..ec733719c776 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -118,7 +118,6 @@ static void check_spurious(void)
 {
 	int cpu;
 
-	smp_rmb();
 	for_each_present_cpu(cpu) {
 		if (spurious[cpu])
 			report_info("WARN: cpu%d got %d spurious interrupts",
@@ -162,8 +161,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		++acked[smp_processor_id()];
 	} else {
 		++spurious[smp_processor_id()];
-		smp_wmb();
 	}
+
+	/* Wait for writes to acked/spurious to complete */
+	dsb(ishst);
 }
 
 static void setup_irq(irq_handler_fn handler)
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

check_acked() has several peculiarities: is the only function among the
check_* functions which calls report() directly, it does two things
(waits for interrupts and checks for misfired interrupts) and it also
mixes printf, report_info and report calls.

check_acked() also reports a pass and returns as soon all the target CPUs
have received interrupts, However, a CPU not having received an interrupt
*now* does not guarantee not receiving an erroneous interrupt if we wait
long enough.

Rework the function by splitting it into two separate functions, each with
a single responsibility: wait_for_interrupts(), which waits for the
expected interrupts to fire, and check_acked() which checks that interrupts
have been received as expected.

wait_for_interrupts() also waits an extra 100 milliseconds after the
expected interrupts have been received in an effort to make sure we don't
miss misfiring interrupts.

Splitting check_acked() into two functions will also allow us to
customize the behavior of each function in the future more easily
without using an unnecessarily long list of arguments for check_acked().

CC: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 26 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index ec733719c776..a9ef1a5def56 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -62,41 +62,42 @@ static void stats_reset(void)
 	}
 }
 
-static void check_acked(const char *testname, cpumask_t *mask)
+static void wait_for_interrupts(cpumask_t *mask)
 {
-	int missing = 0, extra = 0, unexpected = 0;
 	int nr_pass, cpu, i;
-	bool bad = false;
 
 	/* Wait up to 5s for all interrupts to be delivered */
-	for (i = 0; i < 50; ++i) {
+	for (i = 0; i < 50; i++) {
 		mdelay(100);
 		nr_pass = 0;
 		for_each_present_cpu(cpu) {
+			/*
+			 * A CPU having received more than one interrupts will
+			 * show up in check_acked(), and no matter how long we
+			 * wait it cannot un-receive it. Consider at least one
+			 * interrupt as a pass.
+			 */
 			nr_pass += cpumask_test_cpu(cpu, mask) ?
-				acked[cpu] == 1 : acked[cpu] == 0;
-			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
-
-			if (bad_sender[cpu] != -1) {
-				printf("cpu%d received IPI from wrong sender %d\n",
-					cpu, bad_sender[cpu]);
-				bad = true;
-			}
-
-			if (bad_irq[cpu] != -1) {
-				printf("cpu%d received wrong irq %d\n",
-					cpu, bad_irq[cpu]);
-				bad = true;
-			}
+				acked[cpu] >= 1 : acked[cpu] == 0;
 		}
+
 		if (nr_pass == nr_cpus) {
-			report(!bad, "%s", testname);
 			if (i)
-				report_info("took more than %d ms", i * 100);
+				report_info("interrupts took more than %d ms", i * 100);
+			mdelay(100);
 			return;
 		}
 	}
 
+	report_info("interrupts timed-out (5s)");
+}
+
+static bool check_acked(cpumask_t *mask)
+{
+	int missing = 0, extra = 0, unexpected = 0;
+	bool pass = true;
+	int cpu;
+
 	for_each_present_cpu(cpu) {
 		if (cpumask_test_cpu(cpu, mask)) {
 			if (!acked[cpu])
@@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
 			if (acked[cpu])
 				++unexpected;
 		}
+		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
+
+		if (bad_sender[cpu] != -1) {
+			report_info("cpu%d received IPI from wrong sender %d",
+					cpu, bad_sender[cpu]);
+			pass = false;
+		}
+
+		if (bad_irq[cpu] != -1) {
+			report_info("cpu%d received wrong irq %d",
+					cpu, bad_irq[cpu]);
+			pass = false;
+		}
+	}
+
+	if (missing || extra || unexpected) {
+		report_info("ACKS: missing=%d extra=%d unexpected=%d",
+				missing, extra, unexpected);
+		pass = false;
 	}
 
-	report(false, "%s", testname);
-	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
-		    missing, extra, unexpected);
+	return pass;
 }
 
 static void check_spurious(void)
@@ -303,7 +321,8 @@ static void ipi_test_self(void)
 	cpumask_clear(&mask);
 	cpumask_set_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_self();
-	check_acked("IPI: self", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 }
 
@@ -318,7 +337,8 @@ static void ipi_test_smp(void)
 	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
 		cpumask_clear_cpu(i, &mask);
 	gic_ipi_send_mask(IPI_IRQ, &mask);
-	check_acked("IPI: directed", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 
 	report_prefix_push("broadcast");
@@ -326,7 +346,8 @@ static void ipi_test_smp(void)
 	cpumask_copy(&mask, &cpu_present_mask);
 	cpumask_clear_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_broadcast();
-	check_acked("IPI: broadcast", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 }
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

check_acked() has several peculiarities: is the only function among the
check_* functions which calls report() directly, it does two things
(waits for interrupts and checks for misfired interrupts) and it also
mixes printf, report_info and report calls.

check_acked() also reports a pass and returns as soon all the target CPUs
have received interrupts, However, a CPU not having received an interrupt
*now* does not guarantee not receiving an erroneous interrupt if we wait
long enough.

Rework the function by splitting it into two separate functions, each with
a single responsibility: wait_for_interrupts(), which waits for the
expected interrupts to fire, and check_acked() which checks that interrupts
have been received as expected.

wait_for_interrupts() also waits an extra 100 milliseconds after the
expected interrupts have been received in an effort to make sure we don't
miss misfiring interrupts.

Splitting check_acked() into two functions will also allow us to
customize the behavior of each function in the future more easily
without using an unnecessarily long list of arguments for check_acked().

CC: Andre Przywara <andre.przywara@arm.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
 1 file changed, 47 insertions(+), 26 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index ec733719c776..a9ef1a5def56 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -62,41 +62,42 @@ static void stats_reset(void)
 	}
 }
 
-static void check_acked(const char *testname, cpumask_t *mask)
+static void wait_for_interrupts(cpumask_t *mask)
 {
-	int missing = 0, extra = 0, unexpected = 0;
 	int nr_pass, cpu, i;
-	bool bad = false;
 
 	/* Wait up to 5s for all interrupts to be delivered */
-	for (i = 0; i < 50; ++i) {
+	for (i = 0; i < 50; i++) {
 		mdelay(100);
 		nr_pass = 0;
 		for_each_present_cpu(cpu) {
+			/*
+			 * A CPU having received more than one interrupts will
+			 * show up in check_acked(), and no matter how long we
+			 * wait it cannot un-receive it. Consider at least one
+			 * interrupt as a pass.
+			 */
 			nr_pass += cpumask_test_cpu(cpu, mask) ?
-				acked[cpu] == 1 : acked[cpu] == 0;
-			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
-
-			if (bad_sender[cpu] != -1) {
-				printf("cpu%d received IPI from wrong sender %d\n",
-					cpu, bad_sender[cpu]);
-				bad = true;
-			}
-
-			if (bad_irq[cpu] != -1) {
-				printf("cpu%d received wrong irq %d\n",
-					cpu, bad_irq[cpu]);
-				bad = true;
-			}
+				acked[cpu] >= 1 : acked[cpu] == 0;
 		}
+
 		if (nr_pass == nr_cpus) {
-			report(!bad, "%s", testname);
 			if (i)
-				report_info("took more than %d ms", i * 100);
+				report_info("interrupts took more than %d ms", i * 100);
+			mdelay(100);
 			return;
 		}
 	}
 
+	report_info("interrupts timed-out (5s)");
+}
+
+static bool check_acked(cpumask_t *mask)
+{
+	int missing = 0, extra = 0, unexpected = 0;
+	bool pass = true;
+	int cpu;
+
 	for_each_present_cpu(cpu) {
 		if (cpumask_test_cpu(cpu, mask)) {
 			if (!acked[cpu])
@@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
 			if (acked[cpu])
 				++unexpected;
 		}
+		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
+
+		if (bad_sender[cpu] != -1) {
+			report_info("cpu%d received IPI from wrong sender %d",
+					cpu, bad_sender[cpu]);
+			pass = false;
+		}
+
+		if (bad_irq[cpu] != -1) {
+			report_info("cpu%d received wrong irq %d",
+					cpu, bad_irq[cpu]);
+			pass = false;
+		}
+	}
+
+	if (missing || extra || unexpected) {
+		report_info("ACKS: missing=%d extra=%d unexpected=%d",
+				missing, extra, unexpected);
+		pass = false;
 	}
 
-	report(false, "%s", testname);
-	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
-		    missing, extra, unexpected);
+	return pass;
 }
 
 static void check_spurious(void)
@@ -303,7 +321,8 @@ static void ipi_test_self(void)
 	cpumask_clear(&mask);
 	cpumask_set_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_self();
-	check_acked("IPI: self", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 }
 
@@ -318,7 +337,8 @@ static void ipi_test_smp(void)
 	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
 		cpumask_clear_cpu(i, &mask);
 	gic_ipi_send_mask(IPI_IRQ, &mask);
-	check_acked("IPI: directed", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 
 	report_prefix_push("broadcast");
@@ -326,7 +346,8 @@ static void ipi_test_smp(void)
 	cpumask_copy(&mask, &cpu_present_mask);
 	cpumask_clear_cpu(smp_processor_id(), &mask);
 	gic->ipi.send_broadcast();
-	check_acked("IPI: broadcast", &mask);
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask), "Interrupts received");
 	report_prefix_pop();
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> check_acked() has several peculiarities: is the only function among the
> check_* functions which calls report() directly, it does two things
> (waits for interrupts and checks for misfired interrupts) and it also
> mixes printf, report_info and report calls.
> 
> check_acked() also reports a pass and returns as soon all the target CPUs
> have received interrupts, However, a CPU not having received an interrupt
> *now* does not guarantee not receiving an erroneous interrupt if we wait
> long enough.
> 
> Rework the function by splitting it into two separate functions, each with
> a single responsibility: wait_for_interrupts(), which waits for the
> expected interrupts to fire, and check_acked() which checks that interrupts
> have been received as expected.
> 
> wait_for_interrupts() also waits an extra 100 milliseconds after the
> expected interrupts have been received in an effort to make sure we don't
> miss misfiring interrupts.
> 
> Splitting check_acked() into two functions will also allow us to
> customize the behavior of each function in the future more easily
> without using an unnecessarily long list of arguments for check_acked().

Yes, splitting this up looks much better, in general this is a nice
cleanup, thank you!

Some comments below:

> 
> CC: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ec733719c776..a9ef1a5def56 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -62,41 +62,42 @@ static void stats_reset(void)
>  	}
>  }
>  
> -static void check_acked(const char *testname, cpumask_t *mask)
> +static void wait_for_interrupts(cpumask_t *mask)
>  {
> -	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
> -	bool bad = false;
>  
>  	/* Wait up to 5s for all interrupts to be delivered */
> -	for (i = 0; i < 50; ++i) {
> +	for (i = 0; i < 50; i++) {
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> +			/*
> +			 * A CPU having received more than one interrupts will
> +			 * show up in check_acked(), and no matter how long we
> +			 * wait it cannot un-receive it. Consider at least one
> +			 * interrupt as a pass.
> +			 */
>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> -				acked[cpu] == 1 : acked[cpu] == 0;
> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> -
> -			if (bad_sender[cpu] != -1) {
> -				printf("cpu%d received IPI from wrong sender %d\n",
> -					cpu, bad_sender[cpu]);
> -				bad = true;
> -			}
> -
> -			if (bad_irq[cpu] != -1) {
> -				printf("cpu%d received wrong irq %d\n",
> -					cpu, bad_irq[cpu]);
> -				bad = true;
> -			}
> +				acked[cpu] >= 1 : acked[cpu] == 0;


I wonder if this logic was already flawed to begin with: For interrupts
we expect to fire, we wait for up to 5 seconds (really that long?), but
for interrupts we expect *not* to fire we are OK if they don't show up
in the first 100 ms. That does not sound consistent.

I am wondering if we should *not* have the initial 100ms wait at all,
since most interrupts will fire immediately (especially in KVM). And
then have *one* extra wait for, say 100ms, to cover latecomers and
spurious interrupts.

But this might be a topic for some extra work/patch?

>  		}
> +
>  		if (nr_pass == nr_cpus) {
> -			report(!bad, "%s", testname);
>  			if (i)
> -				report_info("took more than %d ms", i * 100);
> +				report_info("interrupts took more than %d ms", i * 100);
> +			mdelay(100);

So this is the extra 100ms you mention in the commit message? I am not
convinced this is the right way (see above) or even the right place
(rather at the call site?) to wait. But at least it deserves a comment,
I believe.

>  			return;
>  		}
>  	}
>  
> +	report_info("interrupts timed-out (5s)");
> +}
> +
> +static bool check_acked(cpumask_t *mask)
> +{
> +	int missing = 0, extra = 0, unexpected = 0;
> +	bool pass = true;
> +	int cpu;
> +
>  	for_each_present_cpu(cpu) {
>  		if (cpumask_test_cpu(cpu, mask)) {
>  			if (!acked[cpu])
> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  			if (acked[cpu])
>  				++unexpected;
>  		}
> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> +
> +		if (bad_sender[cpu] != -1) {
> +			report_info("cpu%d received IPI from wrong sender %d",
> +					cpu, bad_sender[cpu]);
> +			pass = false;
> +		}
> +
> +		if (bad_irq[cpu] != -1) {
> +			report_info("cpu%d received wrong irq %d",
> +					cpu, bad_irq[cpu]);
> +			pass = false;
> +		}
> +	}
> +
> +	if (missing || extra || unexpected) {
> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> +				missing, extra, unexpected);
> +		pass = false;

Thanks, that so much easier to read now.

Cheers,
Andre

>  	}
>  
> -	report(false, "%s", testname);
> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> -		    missing, extra, unexpected);
> +	return pass;
>  }
>  
>  static void check_spurious(void)
> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>  	cpumask_clear(&mask);
>  	cpumask_set_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_self();
> -	check_acked("IPI: self", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>  		cpumask_clear_cpu(i, &mask);
>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> -	check_acked("IPI: directed", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  
>  	report_prefix_push("broadcast");
> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>  	cpumask_copy(&mask, &cpu_present_mask);
>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_broadcast();
> -	check_acked("IPI: broadcast", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> 

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> check_acked() has several peculiarities: is the only function among the
> check_* functions which calls report() directly, it does two things
> (waits for interrupts and checks for misfired interrupts) and it also
> mixes printf, report_info and report calls.
> 
> check_acked() also reports a pass and returns as soon all the target CPUs
> have received interrupts, However, a CPU not having received an interrupt
> *now* does not guarantee not receiving an erroneous interrupt if we wait
> long enough.
> 
> Rework the function by splitting it into two separate functions, each with
> a single responsibility: wait_for_interrupts(), which waits for the
> expected interrupts to fire, and check_acked() which checks that interrupts
> have been received as expected.
> 
> wait_for_interrupts() also waits an extra 100 milliseconds after the
> expected interrupts have been received in an effort to make sure we don't
> miss misfiring interrupts.
> 
> Splitting check_acked() into two functions will also allow us to
> customize the behavior of each function in the future more easily
> without using an unnecessarily long list of arguments for check_acked().

Yes, splitting this up looks much better, in general this is a nice
cleanup, thank you!

Some comments below:

> 
> CC: Andre Przywara <andre.przywara@arm.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>  1 file changed, 47 insertions(+), 26 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index ec733719c776..a9ef1a5def56 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -62,41 +62,42 @@ static void stats_reset(void)
>  	}
>  }
>  
> -static void check_acked(const char *testname, cpumask_t *mask)
> +static void wait_for_interrupts(cpumask_t *mask)
>  {
> -	int missing = 0, extra = 0, unexpected = 0;
>  	int nr_pass, cpu, i;
> -	bool bad = false;
>  
>  	/* Wait up to 5s for all interrupts to be delivered */
> -	for (i = 0; i < 50; ++i) {
> +	for (i = 0; i < 50; i++) {
>  		mdelay(100);
>  		nr_pass = 0;
>  		for_each_present_cpu(cpu) {
> +			/*
> +			 * A CPU having received more than one interrupts will
> +			 * show up in check_acked(), and no matter how long we
> +			 * wait it cannot un-receive it. Consider at least one
> +			 * interrupt as a pass.
> +			 */
>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> -				acked[cpu] == 1 : acked[cpu] == 0;
> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> -
> -			if (bad_sender[cpu] != -1) {
> -				printf("cpu%d received IPI from wrong sender %d\n",
> -					cpu, bad_sender[cpu]);
> -				bad = true;
> -			}
> -
> -			if (bad_irq[cpu] != -1) {
> -				printf("cpu%d received wrong irq %d\n",
> -					cpu, bad_irq[cpu]);
> -				bad = true;
> -			}
> +				acked[cpu] >= 1 : acked[cpu] == 0;


I wonder if this logic was already flawed to begin with: For interrupts
we expect to fire, we wait for up to 5 seconds (really that long?), but
for interrupts we expect *not* to fire we are OK if they don't show up
in the first 100 ms. That does not sound consistent.

I am wondering if we should *not* have the initial 100ms wait at all,
since most interrupts will fire immediately (especially in KVM). And
then have *one* extra wait for, say 100ms, to cover latecomers and
spurious interrupts.

But this might be a topic for some extra work/patch?

>  		}
> +
>  		if (nr_pass == nr_cpus) {
> -			report(!bad, "%s", testname);
>  			if (i)
> -				report_info("took more than %d ms", i * 100);
> +				report_info("interrupts took more than %d ms", i * 100);
> +			mdelay(100);

So this is the extra 100ms you mention in the commit message? I am not
convinced this is the right way (see above) or even the right place
(rather at the call site?) to wait. But at least it deserves a comment,
I believe.

>  			return;
>  		}
>  	}
>  
> +	report_info("interrupts timed-out (5s)");
> +}
> +
> +static bool check_acked(cpumask_t *mask)
> +{
> +	int missing = 0, extra = 0, unexpected = 0;
> +	bool pass = true;
> +	int cpu;
> +
>  	for_each_present_cpu(cpu) {
>  		if (cpumask_test_cpu(cpu, mask)) {
>  			if (!acked[cpu])
> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>  			if (acked[cpu])
>  				++unexpected;
>  		}
> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> +
> +		if (bad_sender[cpu] != -1) {
> +			report_info("cpu%d received IPI from wrong sender %d",
> +					cpu, bad_sender[cpu]);
> +			pass = false;
> +		}
> +
> +		if (bad_irq[cpu] != -1) {
> +			report_info("cpu%d received wrong irq %d",
> +					cpu, bad_irq[cpu]);
> +			pass = false;
> +		}
> +	}
> +
> +	if (missing || extra || unexpected) {
> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> +				missing, extra, unexpected);
> +		pass = false;

Thanks, that so much easier to read now.

Cheers,
Andre

>  	}
>  
> -	report(false, "%s", testname);
> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> -		    missing, extra, unexpected);
> +	return pass;
>  }
>  
>  static void check_spurious(void)
> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>  	cpumask_clear(&mask);
>  	cpumask_set_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_self();
> -	check_acked("IPI: self", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>  		cpumask_clear_cpu(i, &mask);
>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> -	check_acked("IPI: directed", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  
>  	report_prefix_push("broadcast");
> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>  	cpumask_copy(&mask, &cpu_present_mask);
>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>  	gic->ipi.send_broadcast();
> -	check_acked("IPI: broadcast", &mask);
> +	wait_for_interrupts(&mask);
> +	report(check_acked(&mask), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

Hi Andre,

On 12/18/20 3:52 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> check_acked() has several peculiarities: is the only function among the
>> check_* functions which calls report() directly, it does two things
>> (waits for interrupts and checks for misfired interrupts) and it also
>> mixes printf, report_info and report calls.
>>
>> check_acked() also reports a pass and returns as soon all the target CPUs
>> have received interrupts, However, a CPU not having received an interrupt
>> *now* does not guarantee not receiving an erroneous interrupt if we wait
>> long enough.
>>
>> Rework the function by splitting it into two separate functions, each with
>> a single responsibility: wait_for_interrupts(), which waits for the
>> expected interrupts to fire, and check_acked() which checks that interrupts
>> have been received as expected.
>>
>> wait_for_interrupts() also waits an extra 100 milliseconds after the
>> expected interrupts have been received in an effort to make sure we don't
>> miss misfiring interrupts.
>>
>> Splitting check_acked() into two functions will also allow us to
>> customize the behavior of each function in the future more easily
>> without using an unnecessarily long list of arguments for check_acked().
> Yes, splitting this up looks much better, in general this is a nice
> cleanup, thank you!
>
> Some comments below:
>
>> CC: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 47 insertions(+), 26 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index ec733719c776..a9ef1a5def56 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -62,41 +62,42 @@ static void stats_reset(void)
>>  	}
>>  }
>>  
>> -static void check_acked(const char *testname, cpumask_t *mask)
>> +static void wait_for_interrupts(cpumask_t *mask)
>>  {
>> -	int missing = 0, extra = 0, unexpected = 0;
>>  	int nr_pass, cpu, i;
>> -	bool bad = false;
>>  
>>  	/* Wait up to 5s for all interrupts to be delivered */
>> -	for (i = 0; i < 50; ++i) {
>> +	for (i = 0; i < 50; i++) {
>>  		mdelay(100);
>>  		nr_pass = 0;
>>  		for_each_present_cpu(cpu) {
>> +			/*
>> +			 * A CPU having received more than one interrupts will
>> +			 * show up in check_acked(), and no matter how long we
>> +			 * wait it cannot un-receive it. Consider at least one
>> +			 * interrupt as a pass.
>> +			 */
>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>> -				acked[cpu] == 1 : acked[cpu] == 0;
>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>> -
>> -			if (bad_sender[cpu] != -1) {
>> -				printf("cpu%d received IPI from wrong sender %d\n",
>> -					cpu, bad_sender[cpu]);
>> -				bad = true;
>> -			}
>> -
>> -			if (bad_irq[cpu] != -1) {
>> -				printf("cpu%d received wrong irq %d\n",
>> -					cpu, bad_irq[cpu]);
>> -				bad = true;
>> -			}
>> +				acked[cpu] >= 1 : acked[cpu] == 0;
>
> I wonder if this logic was already flawed to begin with: For interrupts
> we expect to fire, we wait for up to 5 seconds (really that long?), but
> for interrupts we expect *not* to fire we are OK if they don't show up
> in the first 100 ms. That does not sound consistent.

There are two ways that I see to fix this:

- Have the caller wait for however long it sees fit, and *after* that waiting
period call wait_for_interrupts().

- Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
wait for the entire duration instead of until the expected interrupts have been
received.

Neither sounds appealing to me for inclusion in this patch set, since I want to
concentrate on reworking check_acked() while keeping much of the current behaviour
intact.

>
> I am wondering if we should *not* have the initial 100ms wait at all,
> since most interrupts will fire immediately (especially in KVM). And
> then have *one* extra wait for, say 100ms, to cover latecomers and
> spurious interrupts.

I don't think it really matters where the 100 millisecond delay is in the loop. If
we call wait_for_interrupts() to actually check that interrupts have fired (as
opposed to checking that they haven't been asserted), then at most we save 100ms
when they are asserted before the start of the loop. I don't think the GIC spec
guarantees that interrupts written to the LR registers will be presented to the
CPU after the guest resumes, so it is conceivable that there might be a delay,
thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.

There are two reasons I chose the approach of having the delay at the start of the
loop:

1. To preserve the current behaviour.

2. To match what the timer test those (see gic_timer_check_state()). I am also
thinking that maybe at some point we could unify these test-independent functions
in the gic driver.

As for the 5 seconds delay, I think we can come up with a patch to pass the delay
as a parameter to the function if needed (if I remember correctly, you needed a
shorter waiting period for your GIC tests).

>
> But this might be a topic for some extra work/patch?

Yes, I would rather make this changes when we have an actual test that needs them.

>
>>  		}
>> +
>>  		if (nr_pass == nr_cpus) {
>> -			report(!bad, "%s", testname);
>>  			if (i)
>> -				report_info("took more than %d ms", i * 100);
>> +				report_info("interrupts took more than %d ms", i * 100);
>> +			mdelay(100);
> So this is the extra 100ms you mention in the commit message? I am not
> convinced this is the right way (see above) or even the right place
> (rather at the call site?) to wait. But at least it deserves a comment,
> I believe.

I'm not sure moving it into the caller is the right thing to do. This is something
that has to do with how interrupts are asserted, not something that is specific to
one test.

You are right about the comment, I'll add one.

Thanks,
Alex
>>  			return;
>>  		}
>>  	}
>>  
>> +	report_info("interrupts timed-out (5s)");
>> +}
>> +
>> +static bool check_acked(cpumask_t *mask)
>> +{
>> +	int missing = 0, extra = 0, unexpected = 0;
>> +	bool pass = true;
>> +	int cpu;
>> +
>>  	for_each_present_cpu(cpu) {
>>  		if (cpumask_test_cpu(cpu, mask)) {
>>  			if (!acked[cpu])
>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>  			if (acked[cpu])
>>  				++unexpected;
>>  		}
>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>> +
>> +		if (bad_sender[cpu] != -1) {
>> +			report_info("cpu%d received IPI from wrong sender %d",
>> +					cpu, bad_sender[cpu]);
>> +			pass = false;
>> +		}
>> +
>> +		if (bad_irq[cpu] != -1) {
>> +			report_info("cpu%d received wrong irq %d",
>> +					cpu, bad_irq[cpu]);
>> +			pass = false;
>> +		}
>> +	}
>> +
>> +	if (missing || extra || unexpected) {
>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
>> +				missing, extra, unexpected);
>> +		pass = false;
> Thanks, that so much easier to read now.
>
> Cheers,
> Andre
>
>>  	}
>>  
>> -	report(false, "%s", testname);
>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>> -		    missing, extra, unexpected);
>> +	return pass;
>>  }
>>  
>>  static void check_spurious(void)
>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>>  	cpumask_clear(&mask);
>>  	cpumask_set_cpu(smp_processor_id(), &mask);
>>  	gic->ipi.send_self();
>> -	check_acked("IPI: self", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  }
>>  
>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>>  		cpumask_clear_cpu(i, &mask);
>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>> -	check_acked("IPI: directed", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  
>>  	report_prefix_push("broadcast");
>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>>  	cpumask_copy(&mask, &cpu_present_mask);
>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>>  	gic->ipi.send_broadcast();
>> -	check_acked("IPI: broadcast", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  }
>>  
>>

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

Hi Andre,

On 12/18/20 3:52 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> check_acked() has several peculiarities: is the only function among the
>> check_* functions which calls report() directly, it does two things
>> (waits for interrupts and checks for misfired interrupts) and it also
>> mixes printf, report_info and report calls.
>>
>> check_acked() also reports a pass and returns as soon all the target CPUs
>> have received interrupts, However, a CPU not having received an interrupt
>> *now* does not guarantee not receiving an erroneous interrupt if we wait
>> long enough.
>>
>> Rework the function by splitting it into two separate functions, each with
>> a single responsibility: wait_for_interrupts(), which waits for the
>> expected interrupts to fire, and check_acked() which checks that interrupts
>> have been received as expected.
>>
>> wait_for_interrupts() also waits an extra 100 milliseconds after the
>> expected interrupts have been received in an effort to make sure we don't
>> miss misfiring interrupts.
>>
>> Splitting check_acked() into two functions will also allow us to
>> customize the behavior of each function in the future more easily
>> without using an unnecessarily long list of arguments for check_acked().
> Yes, splitting this up looks much better, in general this is a nice
> cleanup, thank you!
>
> Some comments below:
>
>> CC: Andre Przywara <andre.przywara@arm.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>>  1 file changed, 47 insertions(+), 26 deletions(-)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index ec733719c776..a9ef1a5def56 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -62,41 +62,42 @@ static void stats_reset(void)
>>  	}
>>  }
>>  
>> -static void check_acked(const char *testname, cpumask_t *mask)
>> +static void wait_for_interrupts(cpumask_t *mask)
>>  {
>> -	int missing = 0, extra = 0, unexpected = 0;
>>  	int nr_pass, cpu, i;
>> -	bool bad = false;
>>  
>>  	/* Wait up to 5s for all interrupts to be delivered */
>> -	for (i = 0; i < 50; ++i) {
>> +	for (i = 0; i < 50; i++) {
>>  		mdelay(100);
>>  		nr_pass = 0;
>>  		for_each_present_cpu(cpu) {
>> +			/*
>> +			 * A CPU having received more than one interrupts will
>> +			 * show up in check_acked(), and no matter how long we
>> +			 * wait it cannot un-receive it. Consider at least one
>> +			 * interrupt as a pass.
>> +			 */
>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>> -				acked[cpu] == 1 : acked[cpu] == 0;
>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>> -
>> -			if (bad_sender[cpu] != -1) {
>> -				printf("cpu%d received IPI from wrong sender %d\n",
>> -					cpu, bad_sender[cpu]);
>> -				bad = true;
>> -			}
>> -
>> -			if (bad_irq[cpu] != -1) {
>> -				printf("cpu%d received wrong irq %d\n",
>> -					cpu, bad_irq[cpu]);
>> -				bad = true;
>> -			}
>> +				acked[cpu] >= 1 : acked[cpu] == 0;
>
> I wonder if this logic was already flawed to begin with: For interrupts
> we expect to fire, we wait for up to 5 seconds (really that long?), but
> for interrupts we expect *not* to fire we are OK if they don't show up
> in the first 100 ms. That does not sound consistent.

There are two ways that I see to fix this:

- Have the caller wait for however long it sees fit, and *after* that waiting
period call wait_for_interrupts().

- Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
wait for the entire duration instead of until the expected interrupts have been
received.

Neither sounds appealing to me for inclusion in this patch set, since I want to
concentrate on reworking check_acked() while keeping much of the current behaviour
intact.

>
> I am wondering if we should *not* have the initial 100ms wait at all,
> since most interrupts will fire immediately (especially in KVM). And
> then have *one* extra wait for, say 100ms, to cover latecomers and
> spurious interrupts.

I don't think it really matters where the 100 millisecond delay is in the loop. If
we call wait_for_interrupts() to actually check that interrupts have fired (as
opposed to checking that they haven't been asserted), then at most we save 100ms
when they are asserted before the start of the loop. I don't think the GIC spec
guarantees that interrupts written to the LR registers will be presented to the
CPU after the guest resumes, so it is conceivable that there might be a delay,
thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.

There are two reasons I chose the approach of having the delay at the start of the
loop:

1. To preserve the current behaviour.

2. To match what the timer test those (see gic_timer_check_state()). I am also
thinking that maybe at some point we could unify these test-independent functions
in the gic driver.

As for the 5 seconds delay, I think we can come up with a patch to pass the delay
as a parameter to the function if needed (if I remember correctly, you needed a
shorter waiting period for your GIC tests).

>
> But this might be a topic for some extra work/patch?

Yes, I would rather make this changes when we have an actual test that needs them.

>
>>  		}
>> +
>>  		if (nr_pass == nr_cpus) {
>> -			report(!bad, "%s", testname);
>>  			if (i)
>> -				report_info("took more than %d ms", i * 100);
>> +				report_info("interrupts took more than %d ms", i * 100);
>> +			mdelay(100);
> So this is the extra 100ms you mention in the commit message? I am not
> convinced this is the right way (see above) or even the right place
> (rather at the call site?) to wait. But at least it deserves a comment,
> I believe.

I'm not sure moving it into the caller is the right thing to do. This is something
that has to do with how interrupts are asserted, not something that is specific to
one test.

You are right about the comment, I'll add one.

Thanks,
Alex
>>  			return;
>>  		}
>>  	}
>>  
>> +	report_info("interrupts timed-out (5s)");
>> +}
>> +
>> +static bool check_acked(cpumask_t *mask)
>> +{
>> +	int missing = 0, extra = 0, unexpected = 0;
>> +	bool pass = true;
>> +	int cpu;
>> +
>>  	for_each_present_cpu(cpu) {
>>  		if (cpumask_test_cpu(cpu, mask)) {
>>  			if (!acked[cpu])
>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>  			if (acked[cpu])
>>  				++unexpected;
>>  		}
>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>> +
>> +		if (bad_sender[cpu] != -1) {
>> +			report_info("cpu%d received IPI from wrong sender %d",
>> +					cpu, bad_sender[cpu]);
>> +			pass = false;
>> +		}
>> +
>> +		if (bad_irq[cpu] != -1) {
>> +			report_info("cpu%d received wrong irq %d",
>> +					cpu, bad_irq[cpu]);
>> +			pass = false;
>> +		}
>> +	}
>> +
>> +	if (missing || extra || unexpected) {
>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
>> +				missing, extra, unexpected);
>> +		pass = false;
> Thanks, that so much easier to read now.
>
> Cheers,
> Andre
>
>>  	}
>>  
>> -	report(false, "%s", testname);
>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>> -		    missing, extra, unexpected);
>> +	return pass;
>>  }
>>  
>>  static void check_spurious(void)
>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>>  	cpumask_clear(&mask);
>>  	cpumask_set_cpu(smp_processor_id(), &mask);
>>  	gic->ipi.send_self();
>> -	check_acked("IPI: self", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  }
>>  
>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>>  		cpumask_clear_cpu(i, &mask);
>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>> -	check_acked("IPI: directed", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  
>>  	report_prefix_push("broadcast");
>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>>  	cpumask_copy(&mask, &cpu_present_mask);
>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>>  	gic->ipi.send_broadcast();
>> -	check_acked("IPI: broadcast", &mask);
>> +	wait_for_interrupts(&mask);
>> +	report(check_acked(&mask), "Interrupts received");
>>  	report_prefix_pop();
>>  }
>>  
>>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Andre Przywara]

On Mon, 25 Jan 2021 17:27:35 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> On 12/18/20 3:52 PM, André Przywara wrote:
> > On 17/12/2020 14:13, Alexandru Elisei wrote:  
> >> check_acked() has several peculiarities: is the only function among the
> >> check_* functions which calls report() directly, it does two things
> >> (waits for interrupts and checks for misfired interrupts) and it also
> >> mixes printf, report_info and report calls.
> >>
> >> check_acked() also reports a pass and returns as soon all the target CPUs
> >> have received interrupts, However, a CPU not having received an interrupt
> >> *now* does not guarantee not receiving an erroneous interrupt if we wait
> >> long enough.
> >>
> >> Rework the function by splitting it into two separate functions, each with
> >> a single responsibility: wait_for_interrupts(), which waits for the
> >> expected interrupts to fire, and check_acked() which checks that interrupts
> >> have been received as expected.
> >>
> >> wait_for_interrupts() also waits an extra 100 milliseconds after the
> >> expected interrupts have been received in an effort to make sure we don't
> >> miss misfiring interrupts.
> >>
> >> Splitting check_acked() into two functions will also allow us to
> >> customize the behavior of each function in the future more easily
> >> without using an unnecessarily long list of arguments for check_acked().  
> > Yes, splitting this up looks much better, in general this is a nice
> > cleanup, thank you!
> >
> > Some comments below:
> >  
> >> CC: Andre Przywara <andre.przywara@arm.com>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 47 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/arm/gic.c b/arm/gic.c
> >> index ec733719c776..a9ef1a5def56 100644
> >> --- a/arm/gic.c
> >> +++ b/arm/gic.c
> >> @@ -62,41 +62,42 @@ static void stats_reset(void)
> >>  	}
> >>  }
> >>  
> >> -static void check_acked(const char *testname, cpumask_t *mask)
> >> +static void wait_for_interrupts(cpumask_t *mask)
> >>  {
> >> -	int missing = 0, extra = 0, unexpected = 0;
> >>  	int nr_pass, cpu, i;
> >> -	bool bad = false;
> >>  
> >>  	/* Wait up to 5s for all interrupts to be delivered */
> >> -	for (i = 0; i < 50; ++i) {
> >> +	for (i = 0; i < 50; i++) {
> >>  		mdelay(100);
> >>  		nr_pass = 0;
> >>  		for_each_present_cpu(cpu) {
> >> +			/*
> >> +			 * A CPU having received more than one interrupts will
> >> +			 * show up in check_acked(), and no matter how long we
> >> +			 * wait it cannot un-receive it. Consider at least one
> >> +			 * interrupt as a pass.
> >> +			 */
> >>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> >> -				acked[cpu] == 1 : acked[cpu] == 0;
> >> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >> -
> >> -			if (bad_sender[cpu] != -1) {
> >> -				printf("cpu%d received IPI from wrong sender %d\n",
> >> -					cpu, bad_sender[cpu]);
> >> -				bad = true;
> >> -			}
> >> -
> >> -			if (bad_irq[cpu] != -1) {
> >> -				printf("cpu%d received wrong irq %d\n",
> >> -					cpu, bad_irq[cpu]);
> >> -				bad = true;
> >> -			}
> >> +				acked[cpu] >= 1 : acked[cpu] == 0;  
> >
> > I wonder if this logic was already flawed to begin with: For interrupts
> > we expect to fire, we wait for up to 5 seconds (really that long?), but
> > for interrupts we expect *not* to fire we are OK if they don't show up
> > in the first 100 ms. That does not sound consistent.  
> 
> There are two ways that I see to fix this:
> 
> - Have the caller wait for however long it sees fit, and *after* that waiting
> period call wait_for_interrupts().
> 
> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
> wait for the entire duration instead of until the expected interrupts have been
> received.
> 
> Neither sounds appealing to me for inclusion in this patch set, since I want to
> concentrate on reworking check_acked() while keeping much of the current behaviour
> intact.
> 
> >
> > I am wondering if we should *not* have the initial 100ms wait at all,
> > since most interrupts will fire immediately (especially in KVM). And
> > then have *one* extra wait for, say 100ms, to cover latecomers and
> > spurious interrupts.  
> 
> I don't think it really matters where the 100 millisecond delay is in the loop.

I think it does. I ran tests with 256 vCPUs, I think we support even
more, and running k-u-t on those setups is one of the cases where it
really matters and we can find real bugs.
So 100ms on their own does not sound much, but it means we wait at least
25.6 seconds, even if everything is fine. I found this scary, confusing
and annoying (in that order), so was wondering if we can avoid that.
 
> If
> we call wait_for_interrupts() to actually check that interrupts have fired (as
> opposed to checking that they haven't been asserted), then at most we save 100ms
> when they are asserted before the start of the loop. I don't think the GIC spec
> guarantees that interrupts written to the LR registers will be presented to the
> CPU after the guest resumes,

I don't know if the spec says anything about it, I guess it would be
out of scope to do so there anyway, but AFAIK this is exactly how it's
implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
before the guest executes the first instruction (that ELR_EL2 points
to), and raises the IRQ exception in EL1.

> so it is conceivable that there might be a delay,

The only practical reason for a delay would be PSTATE.I being set, or
the GICV being disabled, I think.

I would say one would expect interrupts to fire *immediately*, and
allowing them 100ms slack does not sound like the right thing. If there
is some delay, I would at least like to know about it. And I would
grant them a few instructions delay, at best.

If you still think you need that delay, because everything else would
be too complicated (at least for this iteration), then please make it
*much* smaller (< 1us).

Cheers,
Andre


> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
> 
> There are two reasons I chose the approach of having the delay at the start of the
> loop:
> 
> 1. To preserve the current behaviour.
> 
> 2. To match what the timer test those (see gic_timer_check_state()). I am also
> thinking that maybe at some point we could unify these test-independent functions
> in the gic driver.
> 
> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
> as a parameter to the function if needed (if I remember correctly, you needed a
> shorter waiting period for your GIC tests).
> 
> >
> > But this might be a topic for some extra work/patch?  
> 
> Yes, I would rather make this changes when we have an actual test that needs them.
> 
> >  
> >>  		}
> >> +
> >>  		if (nr_pass == nr_cpus) {
> >> -			report(!bad, "%s", testname);
> >>  			if (i)
> >> -				report_info("took more than %d ms", i * 100);
> >> +				report_info("interrupts took more than %d ms", i * 100);
> >> +			mdelay(100);  
> > So this is the extra 100ms you mention in the commit message? I am not
> > convinced this is the right way (see above) or even the right place
> > (rather at the call site?) to wait. But at least it deserves a comment,
> > I believe.  
> 
> I'm not sure moving it into the caller is the right thing to do. This is something
> that has to do with how interrupts are asserted, not something that is specific to
> one test.
> 
> You are right about the comment, I'll add one.
> 
> Thanks,
> Alex
> >>  			return;
> >>  		}
> >>  	}
> >>  
> >> +	report_info("interrupts timed-out (5s)");
> >> +}
> >> +
> >> +static bool check_acked(cpumask_t *mask)
> >> +{
> >> +	int missing = 0, extra = 0, unexpected = 0;
> >> +	bool pass = true;
> >> +	int cpu;
> >> +
> >>  	for_each_present_cpu(cpu) {
> >>  		if (cpumask_test_cpu(cpu, mask)) {
> >>  			if (!acked[cpu])
> >> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
> >>  			if (acked[cpu])
> >>  				++unexpected;
> >>  		}
> >> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >> +
> >> +		if (bad_sender[cpu] != -1) {
> >> +			report_info("cpu%d received IPI from wrong sender %d",
> >> +					cpu, bad_sender[cpu]);
> >> +			pass = false;
> >> +		}
> >> +
> >> +		if (bad_irq[cpu] != -1) {
> >> +			report_info("cpu%d received wrong irq %d",
> >> +					cpu, bad_irq[cpu]);
> >> +			pass = false;
> >> +		}
> >> +	}
> >> +
> >> +	if (missing || extra || unexpected) {
> >> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> >> +				missing, extra, unexpected);
> >> +		pass = false;  
> > Thanks, that so much easier to read now.
> >
> > Cheers,
> > Andre
> >  
> >>  	}
> >>  
> >> -	report(false, "%s", testname);
> >> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> >> -		    missing, extra, unexpected);
> >> +	return pass;
> >>  }
> >>  
> >>  static void check_spurious(void)
> >> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
> >>  	cpumask_clear(&mask);
> >>  	cpumask_set_cpu(smp_processor_id(), &mask);
> >>  	gic->ipi.send_self();
> >> -	check_acked("IPI: self", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  }
> >>  
> >> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
> >>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> >>  		cpumask_clear_cpu(i, &mask);
> >>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> >> -	check_acked("IPI: directed", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  
> >>  	report_prefix_push("broadcast");
> >> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
> >>  	cpumask_copy(&mask, &cpu_present_mask);
> >>  	cpumask_clear_cpu(smp_processor_id(), &mask);
> >>  	gic->ipi.send_broadcast();
> >> -	check_acked("IPI: broadcast", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  }
> >>  
> >>  

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Andre Przywara]

On Mon, 25 Jan 2021 17:27:35 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> On 12/18/20 3:52 PM, André Przywara wrote:
> > On 17/12/2020 14:13, Alexandru Elisei wrote:  
> >> check_acked() has several peculiarities: is the only function among the
> >> check_* functions which calls report() directly, it does two things
> >> (waits for interrupts and checks for misfired interrupts) and it also
> >> mixes printf, report_info and report calls.
> >>
> >> check_acked() also reports a pass and returns as soon all the target CPUs
> >> have received interrupts, However, a CPU not having received an interrupt
> >> *now* does not guarantee not receiving an erroneous interrupt if we wait
> >> long enough.
> >>
> >> Rework the function by splitting it into two separate functions, each with
> >> a single responsibility: wait_for_interrupts(), which waits for the
> >> expected interrupts to fire, and check_acked() which checks that interrupts
> >> have been received as expected.
> >>
> >> wait_for_interrupts() also waits an extra 100 milliseconds after the
> >> expected interrupts have been received in an effort to make sure we don't
> >> miss misfiring interrupts.
> >>
> >> Splitting check_acked() into two functions will also allow us to
> >> customize the behavior of each function in the future more easily
> >> without using an unnecessarily long list of arguments for check_acked().  
> > Yes, splitting this up looks much better, in general this is a nice
> > cleanup, thank you!
> >
> > Some comments below:
> >  
> >> CC: Andre Przywara <andre.przywara@arm.com>
> >> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >> ---
> >>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
> >>  1 file changed, 47 insertions(+), 26 deletions(-)
> >>
> >> diff --git a/arm/gic.c b/arm/gic.c
> >> index ec733719c776..a9ef1a5def56 100644
> >> --- a/arm/gic.c
> >> +++ b/arm/gic.c
> >> @@ -62,41 +62,42 @@ static void stats_reset(void)
> >>  	}
> >>  }
> >>  
> >> -static void check_acked(const char *testname, cpumask_t *mask)
> >> +static void wait_for_interrupts(cpumask_t *mask)
> >>  {
> >> -	int missing = 0, extra = 0, unexpected = 0;
> >>  	int nr_pass, cpu, i;
> >> -	bool bad = false;
> >>  
> >>  	/* Wait up to 5s for all interrupts to be delivered */
> >> -	for (i = 0; i < 50; ++i) {
> >> +	for (i = 0; i < 50; i++) {
> >>  		mdelay(100);
> >>  		nr_pass = 0;
> >>  		for_each_present_cpu(cpu) {
> >> +			/*
> >> +			 * A CPU having received more than one interrupts will
> >> +			 * show up in check_acked(), and no matter how long we
> >> +			 * wait it cannot un-receive it. Consider at least one
> >> +			 * interrupt as a pass.
> >> +			 */
> >>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> >> -				acked[cpu] == 1 : acked[cpu] == 0;
> >> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >> -
> >> -			if (bad_sender[cpu] != -1) {
> >> -				printf("cpu%d received IPI from wrong sender %d\n",
> >> -					cpu, bad_sender[cpu]);
> >> -				bad = true;
> >> -			}
> >> -
> >> -			if (bad_irq[cpu] != -1) {
> >> -				printf("cpu%d received wrong irq %d\n",
> >> -					cpu, bad_irq[cpu]);
> >> -				bad = true;
> >> -			}
> >> +				acked[cpu] >= 1 : acked[cpu] == 0;  
> >
> > I wonder if this logic was already flawed to begin with: For interrupts
> > we expect to fire, we wait for up to 5 seconds (really that long?), but
> > for interrupts we expect *not* to fire we are OK if they don't show up
> > in the first 100 ms. That does not sound consistent.  
> 
> There are two ways that I see to fix this:
> 
> - Have the caller wait for however long it sees fit, and *after* that waiting
> period call wait_for_interrupts().
> 
> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
> wait for the entire duration instead of until the expected interrupts have been
> received.
> 
> Neither sounds appealing to me for inclusion in this patch set, since I want to
> concentrate on reworking check_acked() while keeping much of the current behaviour
> intact.
> 
> >
> > I am wondering if we should *not* have the initial 100ms wait at all,
> > since most interrupts will fire immediately (especially in KVM). And
> > then have *one* extra wait for, say 100ms, to cover latecomers and
> > spurious interrupts.  
> 
> I don't think it really matters where the 100 millisecond delay is in the loop.

I think it does. I ran tests with 256 vCPUs, I think we support even
more, and running k-u-t on those setups is one of the cases where it
really matters and we can find real bugs.
So 100ms on their own does not sound much, but it means we wait at least
25.6 seconds, even if everything is fine. I found this scary, confusing
and annoying (in that order), so was wondering if we can avoid that.
 
> If
> we call wait_for_interrupts() to actually check that interrupts have fired (as
> opposed to checking that they haven't been asserted), then at most we save 100ms
> when they are asserted before the start of the loop. I don't think the GIC spec
> guarantees that interrupts written to the LR registers will be presented to the
> CPU after the guest resumes,

I don't know if the spec says anything about it, I guess it would be
out of scope to do so there anyway, but AFAIK this is exactly how it's
implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
before the guest executes the first instruction (that ELR_EL2 points
to), and raises the IRQ exception in EL1.

> so it is conceivable that there might be a delay,

The only practical reason for a delay would be PSTATE.I being set, or
the GICV being disabled, I think.

I would say one would expect interrupts to fire *immediately*, and
allowing them 100ms slack does not sound like the right thing. If there
is some delay, I would at least like to know about it. And I would
grant them a few instructions delay, at best.

If you still think you need that delay, because everything else would
be too complicated (at least for this iteration), then please make it
*much* smaller (< 1us).

Cheers,
Andre


> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
> 
> There are two reasons I chose the approach of having the delay at the start of the
> loop:
> 
> 1. To preserve the current behaviour.
> 
> 2. To match what the timer test those (see gic_timer_check_state()). I am also
> thinking that maybe at some point we could unify these test-independent functions
> in the gic driver.
> 
> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
> as a parameter to the function if needed (if I remember correctly, you needed a
> shorter waiting period for your GIC tests).
> 
> >
> > But this might be a topic for some extra work/patch?  
> 
> Yes, I would rather make this changes when we have an actual test that needs them.
> 
> >  
> >>  		}
> >> +
> >>  		if (nr_pass == nr_cpus) {
> >> -			report(!bad, "%s", testname);
> >>  			if (i)
> >> -				report_info("took more than %d ms", i * 100);
> >> +				report_info("interrupts took more than %d ms", i * 100);
> >> +			mdelay(100);  
> > So this is the extra 100ms you mention in the commit message? I am not
> > convinced this is the right way (see above) or even the right place
> > (rather at the call site?) to wait. But at least it deserves a comment,
> > I believe.  
> 
> I'm not sure moving it into the caller is the right thing to do. This is something
> that has to do with how interrupts are asserted, not something that is specific to
> one test.
> 
> You are right about the comment, I'll add one.
> 
> Thanks,
> Alex
> >>  			return;
> >>  		}
> >>  	}
> >>  
> >> +	report_info("interrupts timed-out (5s)");
> >> +}
> >> +
> >> +static bool check_acked(cpumask_t *mask)
> >> +{
> >> +	int missing = 0, extra = 0, unexpected = 0;
> >> +	bool pass = true;
> >> +	int cpu;
> >> +
> >>  	for_each_present_cpu(cpu) {
> >>  		if (cpumask_test_cpu(cpu, mask)) {
> >>  			if (!acked[cpu])
> >> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
> >>  			if (acked[cpu])
> >>  				++unexpected;
> >>  		}
> >> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >> +
> >> +		if (bad_sender[cpu] != -1) {
> >> +			report_info("cpu%d received IPI from wrong sender %d",
> >> +					cpu, bad_sender[cpu]);
> >> +			pass = false;
> >> +		}
> >> +
> >> +		if (bad_irq[cpu] != -1) {
> >> +			report_info("cpu%d received wrong irq %d",
> >> +					cpu, bad_irq[cpu]);
> >> +			pass = false;
> >> +		}
> >> +	}
> >> +
> >> +	if (missing || extra || unexpected) {
> >> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> >> +				missing, extra, unexpected);
> >> +		pass = false;  
> > Thanks, that so much easier to read now.
> >
> > Cheers,
> > Andre
> >  
> >>  	}
> >>  
> >> -	report(false, "%s", testname);
> >> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> >> -		    missing, extra, unexpected);
> >> +	return pass;
> >>  }
> >>  
> >>  static void check_spurious(void)
> >> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
> >>  	cpumask_clear(&mask);
> >>  	cpumask_set_cpu(smp_processor_id(), &mask);
> >>  	gic->ipi.send_self();
> >> -	check_acked("IPI: self", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  }
> >>  
> >> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
> >>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> >>  		cpumask_clear_cpu(i, &mask);
> >>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> >> -	check_acked("IPI: directed", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  
> >>  	report_prefix_push("broadcast");
> >> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
> >>  	cpumask_copy(&mask, &cpu_present_mask);
> >>  	cpumask_clear_cpu(smp_processor_id(), &mask);
> >>  	gic->ipi.send_broadcast();
> >> -	check_acked("IPI: broadcast", &mask);
> >> +	wait_for_interrupts(&mask);
> >> +	report(check_acked(&mask), "Interrupts received");
> >>  	report_prefix_pop();
> >>  }
> >>  
> >>  

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

Hi Andre,

On 1/27/21 3:10 PM, Andre Przywara wrote:
> On Mon, 25 Jan 2021 17:27:35 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi Alex,
>
>> On 12/18/20 3:52 PM, André Przywara wrote:
>>> On 17/12/2020 14:13, Alexandru Elisei wrote:  
>>>> check_acked() has several peculiarities: is the only function among the
>>>> check_* functions which calls report() directly, it does two things
>>>> (waits for interrupts and checks for misfired interrupts) and it also
>>>> mixes printf, report_info and report calls.
>>>>
>>>> check_acked() also reports a pass and returns as soon all the target CPUs
>>>> have received interrupts, However, a CPU not having received an interrupt
>>>> *now* does not guarantee not receiving an erroneous interrupt if we wait
>>>> long enough.
>>>>
>>>> Rework the function by splitting it into two separate functions, each with
>>>> a single responsibility: wait_for_interrupts(), which waits for the
>>>> expected interrupts to fire, and check_acked() which checks that interrupts
>>>> have been received as expected.
>>>>
>>>> wait_for_interrupts() also waits an extra 100 milliseconds after the
>>>> expected interrupts have been received in an effort to make sure we don't
>>>> miss misfiring interrupts.
>>>>
>>>> Splitting check_acked() into two functions will also allow us to
>>>> customize the behavior of each function in the future more easily
>>>> without using an unnecessarily long list of arguments for check_acked().  
>>> Yes, splitting this up looks much better, in general this is a nice
>>> cleanup, thank you!
>>>
>>> Some comments below:
>>>  
>>>> CC: Andre Przywara <andre.przywara@arm.com>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>>>>  1 file changed, 47 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>> index ec733719c776..a9ef1a5def56 100644
>>>> --- a/arm/gic.c
>>>> +++ b/arm/gic.c
>>>> @@ -62,41 +62,42 @@ static void stats_reset(void)
>>>>  	}
>>>>  }
>>>>  
>>>> -static void check_acked(const char *testname, cpumask_t *mask)
>>>> +static void wait_for_interrupts(cpumask_t *mask)
>>>>  {
>>>> -	int missing = 0, extra = 0, unexpected = 0;
>>>>  	int nr_pass, cpu, i;
>>>> -	bool bad = false;
>>>>  
>>>>  	/* Wait up to 5s for all interrupts to be delivered */
>>>> -	for (i = 0; i < 50; ++i) {
>>>> +	for (i = 0; i < 50; i++) {
>>>>  		mdelay(100);
>>>>  		nr_pass = 0;
>>>>  		for_each_present_cpu(cpu) {
>>>> +			/*
>>>> +			 * A CPU having received more than one interrupts will
>>>> +			 * show up in check_acked(), and no matter how long we
>>>> +			 * wait it cannot un-receive it. Consider at least one
>>>> +			 * interrupt as a pass.
>>>> +			 */
>>>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>>>> -				acked[cpu] == 1 : acked[cpu] == 0;
>>>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>>>> -
>>>> -			if (bad_sender[cpu] != -1) {
>>>> -				printf("cpu%d received IPI from wrong sender %d\n",
>>>> -					cpu, bad_sender[cpu]);
>>>> -				bad = true;
>>>> -			}
>>>> -
>>>> -			if (bad_irq[cpu] != -1) {
>>>> -				printf("cpu%d received wrong irq %d\n",
>>>> -					cpu, bad_irq[cpu]);
>>>> -				bad = true;
>>>> -			}
>>>> +				acked[cpu] >= 1 : acked[cpu] == 0;  
>>> I wonder if this logic was already flawed to begin with: For interrupts
>>> we expect to fire, we wait for up to 5 seconds (really that long?), but
>>> for interrupts we expect *not* to fire we are OK if they don't show up
>>> in the first 100 ms. That does not sound consistent.  
>> There are two ways that I see to fix this:
>>
>> - Have the caller wait for however long it sees fit, and *after* that waiting
>> period call wait_for_interrupts().
>>
>> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
>> wait for the entire duration instead of until the expected interrupts have been
>> received.
>>
>> Neither sounds appealing to me for inclusion in this patch set, since I want to
>> concentrate on reworking check_acked() while keeping much of the current behaviour
>> intact.
>>
>>> I am wondering if we should *not* have the initial 100ms wait at all,
>>> since most interrupts will fire immediately (especially in KVM). And
>>> then have *one* extra wait for, say 100ms, to cover latecomers and
>>> spurious interrupts.  
>> I don't think it really matters where the 100 millisecond delay is in the loop.
> I think it does. I ran tests with 256 vCPUs, I think we support even
> more, and running k-u-t on those setups is one of the cases where it
> really matters and we can find real bugs.
> So 100ms on their own does not sound much, but it means we wait at least
> 25.6 seconds, even if everything is fine. I found this scary, confusing
> and annoying (in that order), so was wondering if we can avoid that.

I'm not sure where that 25.6 second delay is coming from. The mdelay() is at the
start of the for loop, *before* the for_each_cpu() loop, so it's not executed for
each VCPU.

I've also run the ipi test on my rockpro64 with 256 vcpus with kvmtool and I
didn't notice any unexpected delays.

>  
>> If
>> we call wait_for_interrupts() to actually check that interrupts have fired (as
>> opposed to checking that they haven't been asserted), then at most we save 100ms
>> when they are asserted before the start of the loop. I don't think the GIC spec
>> guarantees that interrupts written to the LR registers will be presented to the
>> CPU after the guest resumes,
> I don't know if the spec says anything about it, I guess it would be
> out of scope to do so there anyway, but AFAIK this is exactly how it's
> implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
> before the guest executes the first instruction (that ELR_EL2 points
> to), and raises the IRQ exception in EL1.
>
>> so it is conceivable that there might be a delay,
> The only practical reason for a delay would be PSTATE.I being set, or
> the GICV being disabled, I think.
>
> I would say one would expect interrupts to fire *immediately*, and
> allowing them 100ms slack does not sound like the right thing. If there
> is some delay, I would at least like to know about it. And I would
> grant them a few instructions delay, at best.

I think you're forgetting the fact that the interrupts are delivered to the other
VCPUs, not to the VCPU that is calling wait_for_interrupts(). So it's the
interrupt handlers running on the other VCPUs that update acked, which afterwards
is read in wait_for_interrupts().

As an experiment, I moved the mdelay() at the end of the loop and I tried running
the IPI test with 2 and 256 VCPUs:

$ taskset -c 4,5 ~/lkvm-static run -c2 -m128 -f arm/gic.flat -p 'ipi'
  # lkvm run --firmware arm/gic.flat -m 128 -c 2 --name guest-1720
  Info: Placing fdt at 0x80200000 - 0x80210000
chr_testdev_init: chr-testdev: can't find a virtio-console
PASS: gicv3: ipi: self: Interrupts received
INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
PASS: gicv3: ipi: target-list: Interrupts received
INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
PASS: gicv3: ipi: broadcast: Interrupts received
SUMMARY: 3 tests

$ taskset -c 4,5 ~/lkvm-static run -c256 -m128 -f arm/gic.flat -p 'ipi'
  # lkvm run --firmware arm/gic.flat -m 128 -c 256 --name guest-2000
  Info: Placing fdt at 0x80200000 - 0x80210000
  # Warning: The maximum recommended amount of VCPUs is 6
chr_testdev_init: chr-testdev: can't find a virtio-console
PASS: gicv3: ipi: self: Interrupts received
INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
PASS: gicv3: ipi: target-list: Interrupts received
INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
PASS: gicv3: ipi: broadcast: Interrupts received
SUMMARY: 3 tests

For the 256 VCPUs test, on two runs I got the "interrupts took more than 100 ms"
message for target-list, on one test I didn't (but for broadcast I always got the
message).

For the 2 VCPUs test, I always got the message (tried it 5 times).

Thanks,
Alex
>
> If you still think you need that delay, because everything else would
> be too complicated (at least for this iteration), then please make it
> *much* smaller (< 1us).
>
> Cheers,
> Andre
>
>
>> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
>>
>> There are two reasons I chose the approach of having the delay at the start of the
>> loop:
>>
>> 1. To preserve the current behaviour.
>>
>> 2. To match what the timer test those (see gic_timer_check_state()). I am also
>> thinking that maybe at some point we could unify these test-independent functions
>> in the gic driver.
>>
>> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
>> as a parameter to the function if needed (if I remember correctly, you needed a
>> shorter waiting period for your GIC tests).
>>
>>> But this might be a topic for some extra work/patch?  
>> Yes, I would rather make this changes when we have an actual test that needs them.
>>
>>>  
>>>>  		}
>>>> +
>>>>  		if (nr_pass == nr_cpus) {
>>>> -			report(!bad, "%s", testname);
>>>>  			if (i)
>>>> -				report_info("took more than %d ms", i * 100);
>>>> +				report_info("interrupts took more than %d ms", i * 100);
>>>> +			mdelay(100);  
>>> So this is the extra 100ms you mention in the commit message? I am not
>>> convinced this is the right way (see above) or even the right place
>>> (rather at the call site?) to wait. But at least it deserves a comment,
>>> I believe.  
>> I'm not sure moving it into the caller is the right thing to do. This is something
>> that has to do with how interrupts are asserted, not something that is specific to
>> one test.
>>
>> You are right about the comment, I'll add one.
>>
>> Thanks,
>> Alex
>>>>  			return;
>>>>  		}
>>>>  	}
>>>>  
>>>> +	report_info("interrupts timed-out (5s)");
>>>> +}
>>>> +
>>>> +static bool check_acked(cpumask_t *mask)
>>>> +{
>>>> +	int missing = 0, extra = 0, unexpected = 0;
>>>> +	bool pass = true;
>>>> +	int cpu;
>>>> +
>>>>  	for_each_present_cpu(cpu) {
>>>>  		if (cpumask_test_cpu(cpu, mask)) {
>>>>  			if (!acked[cpu])
>>>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>>>  			if (acked[cpu])
>>>>  				++unexpected;
>>>>  		}
>>>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>>>> +
>>>> +		if (bad_sender[cpu] != -1) {
>>>> +			report_info("cpu%d received IPI from wrong sender %d",
>>>> +					cpu, bad_sender[cpu]);
>>>> +			pass = false;
>>>> +		}
>>>> +
>>>> +		if (bad_irq[cpu] != -1) {
>>>> +			report_info("cpu%d received wrong irq %d",
>>>> +					cpu, bad_irq[cpu]);
>>>> +			pass = false;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (missing || extra || unexpected) {
>>>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
>>>> +				missing, extra, unexpected);
>>>> +		pass = false;  
>>> Thanks, that so much easier to read now.
>>>
>>> Cheers,
>>> Andre
>>>  
>>>>  	}
>>>>  
>>>> -	report(false, "%s", testname);
>>>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>>>> -		    missing, extra, unexpected);
>>>> +	return pass;
>>>>  }
>>>>  
>>>>  static void check_spurious(void)
>>>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>>>>  	cpumask_clear(&mask);
>>>>  	cpumask_set_cpu(smp_processor_id(), &mask);
>>>>  	gic->ipi.send_self();
>>>> -	check_acked("IPI: self", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  }
>>>>  
>>>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>>>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>>>>  		cpumask_clear_cpu(i, &mask);
>>>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>>>> -	check_acked("IPI: directed", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  
>>>>  	report_prefix_push("broadcast");
>>>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>>>>  	cpumask_copy(&mask, &cpu_present_mask);
>>>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>>>>  	gic->ipi.send_broadcast();
>>>> -	check_acked("IPI: broadcast", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  }
>>>>  
>>>>  

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Alexandru Elisei]

Hi Andre,

On 1/27/21 3:10 PM, Andre Przywara wrote:
> On Mon, 25 Jan 2021 17:27:35 +0000
> Alexandru Elisei <alexandru.elisei@arm.com> wrote:
>
> Hi Alex,
>
>> On 12/18/20 3:52 PM, André Przywara wrote:
>>> On 17/12/2020 14:13, Alexandru Elisei wrote:  
>>>> check_acked() has several peculiarities: is the only function among the
>>>> check_* functions which calls report() directly, it does two things
>>>> (waits for interrupts and checks for misfired interrupts) and it also
>>>> mixes printf, report_info and report calls.
>>>>
>>>> check_acked() also reports a pass and returns as soon all the target CPUs
>>>> have received interrupts, However, a CPU not having received an interrupt
>>>> *now* does not guarantee not receiving an erroneous interrupt if we wait
>>>> long enough.
>>>>
>>>> Rework the function by splitting it into two separate functions, each with
>>>> a single responsibility: wait_for_interrupts(), which waits for the
>>>> expected interrupts to fire, and check_acked() which checks that interrupts
>>>> have been received as expected.
>>>>
>>>> wait_for_interrupts() also waits an extra 100 milliseconds after the
>>>> expected interrupts have been received in an effort to make sure we don't
>>>> miss misfiring interrupts.
>>>>
>>>> Splitting check_acked() into two functions will also allow us to
>>>> customize the behavior of each function in the future more easily
>>>> without using an unnecessarily long list of arguments for check_acked().  
>>> Yes, splitting this up looks much better, in general this is a nice
>>> cleanup, thank you!
>>>
>>> Some comments below:
>>>  
>>>> CC: Andre Przywara <andre.przywara@arm.com>
>>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>>>> ---
>>>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
>>>>  1 file changed, 47 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/arm/gic.c b/arm/gic.c
>>>> index ec733719c776..a9ef1a5def56 100644
>>>> --- a/arm/gic.c
>>>> +++ b/arm/gic.c
>>>> @@ -62,41 +62,42 @@ static void stats_reset(void)
>>>>  	}
>>>>  }
>>>>  
>>>> -static void check_acked(const char *testname, cpumask_t *mask)
>>>> +static void wait_for_interrupts(cpumask_t *mask)
>>>>  {
>>>> -	int missing = 0, extra = 0, unexpected = 0;
>>>>  	int nr_pass, cpu, i;
>>>> -	bool bad = false;
>>>>  
>>>>  	/* Wait up to 5s for all interrupts to be delivered */
>>>> -	for (i = 0; i < 50; ++i) {
>>>> +	for (i = 0; i < 50; i++) {
>>>>  		mdelay(100);
>>>>  		nr_pass = 0;
>>>>  		for_each_present_cpu(cpu) {
>>>> +			/*
>>>> +			 * A CPU having received more than one interrupts will
>>>> +			 * show up in check_acked(), and no matter how long we
>>>> +			 * wait it cannot un-receive it. Consider at least one
>>>> +			 * interrupt as a pass.
>>>> +			 */
>>>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
>>>> -				acked[cpu] == 1 : acked[cpu] == 0;
>>>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>>>> -
>>>> -			if (bad_sender[cpu] != -1) {
>>>> -				printf("cpu%d received IPI from wrong sender %d\n",
>>>> -					cpu, bad_sender[cpu]);
>>>> -				bad = true;
>>>> -			}
>>>> -
>>>> -			if (bad_irq[cpu] != -1) {
>>>> -				printf("cpu%d received wrong irq %d\n",
>>>> -					cpu, bad_irq[cpu]);
>>>> -				bad = true;
>>>> -			}
>>>> +				acked[cpu] >= 1 : acked[cpu] == 0;  
>>> I wonder if this logic was already flawed to begin with: For interrupts
>>> we expect to fire, we wait for up to 5 seconds (really that long?), but
>>> for interrupts we expect *not* to fire we are OK if they don't show up
>>> in the first 100 ms. That does not sound consistent.  
>> There are two ways that I see to fix this:
>>
>> - Have the caller wait for however long it sees fit, and *after* that waiting
>> period call wait_for_interrupts().
>>
>> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
>> wait for the entire duration instead of until the expected interrupts have been
>> received.
>>
>> Neither sounds appealing to me for inclusion in this patch set, since I want to
>> concentrate on reworking check_acked() while keeping much of the current behaviour
>> intact.
>>
>>> I am wondering if we should *not* have the initial 100ms wait at all,
>>> since most interrupts will fire immediately (especially in KVM). And
>>> then have *one* extra wait for, say 100ms, to cover latecomers and
>>> spurious interrupts.  
>> I don't think it really matters where the 100 millisecond delay is in the loop.
> I think it does. I ran tests with 256 vCPUs, I think we support even
> more, and running k-u-t on those setups is one of the cases where it
> really matters and we can find real bugs.
> So 100ms on their own does not sound much, but it means we wait at least
> 25.6 seconds, even if everything is fine. I found this scary, confusing
> and annoying (in that order), so was wondering if we can avoid that.

I'm not sure where that 25.6 second delay is coming from. The mdelay() is at the
start of the for loop, *before* the for_each_cpu() loop, so it's not executed for
each VCPU.

I've also run the ipi test on my rockpro64 with 256 vcpus with kvmtool and I
didn't notice any unexpected delays.

>  
>> If
>> we call wait_for_interrupts() to actually check that interrupts have fired (as
>> opposed to checking that they haven't been asserted), then at most we save 100ms
>> when they are asserted before the start of the loop. I don't think the GIC spec
>> guarantees that interrupts written to the LR registers will be presented to the
>> CPU after the guest resumes,
> I don't know if the spec says anything about it, I guess it would be
> out of scope to do so there anyway, but AFAIK this is exactly how it's
> implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
> before the guest executes the first instruction (that ELR_EL2 points
> to), and raises the IRQ exception in EL1.
>
>> so it is conceivable that there might be a delay,
> The only practical reason for a delay would be PSTATE.I being set, or
> the GICV being disabled, I think.
>
> I would say one would expect interrupts to fire *immediately*, and
> allowing them 100ms slack does not sound like the right thing. If there
> is some delay, I would at least like to know about it. And I would
> grant them a few instructions delay, at best.

I think you're forgetting the fact that the interrupts are delivered to the other
VCPUs, not to the VCPU that is calling wait_for_interrupts(). So it's the
interrupt handlers running on the other VCPUs that update acked, which afterwards
is read in wait_for_interrupts().

As an experiment, I moved the mdelay() at the end of the loop and I tried running
the IPI test with 2 and 256 VCPUs:

$ taskset -c 4,5 ~/lkvm-static run -c2 -m128 -f arm/gic.flat -p 'ipi'
  # lkvm run --firmware arm/gic.flat -m 128 -c 2 --name guest-1720
  Info: Placing fdt at 0x80200000 - 0x80210000
chr_testdev_init: chr-testdev: can't find a virtio-console
PASS: gicv3: ipi: self: Interrupts received
INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
PASS: gicv3: ipi: target-list: Interrupts received
INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
PASS: gicv3: ipi: broadcast: Interrupts received
SUMMARY: 3 tests

$ taskset -c 4,5 ~/lkvm-static run -c256 -m128 -f arm/gic.flat -p 'ipi'
  # lkvm run --firmware arm/gic.flat -m 128 -c 256 --name guest-2000
  Info: Placing fdt at 0x80200000 - 0x80210000
  # Warning: The maximum recommended amount of VCPUs is 6
chr_testdev_init: chr-testdev: can't find a virtio-console
PASS: gicv3: ipi: self: Interrupts received
INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
PASS: gicv3: ipi: target-list: Interrupts received
INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
PASS: gicv3: ipi: broadcast: Interrupts received
SUMMARY: 3 tests

For the 256 VCPUs test, on two runs I got the "interrupts took more than 100 ms"
message for target-list, on one test I didn't (but for broadcast I always got the
message).

For the 2 VCPUs test, I always got the message (tried it 5 times).

Thanks,
Alex
>
> If you still think you need that delay, because everything else would
> be too complicated (at least for this iteration), then please make it
> *much* smaller (< 1us).
>
> Cheers,
> Andre
>
>
>> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
>>
>> There are two reasons I chose the approach of having the delay at the start of the
>> loop:
>>
>> 1. To preserve the current behaviour.
>>
>> 2. To match what the timer test those (see gic_timer_check_state()). I am also
>> thinking that maybe at some point we could unify these test-independent functions
>> in the gic driver.
>>
>> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
>> as a parameter to the function if needed (if I remember correctly, you needed a
>> shorter waiting period for your GIC tests).
>>
>>> But this might be a topic for some extra work/patch?  
>> Yes, I would rather make this changes when we have an actual test that needs them.
>>
>>>  
>>>>  		}
>>>> +
>>>>  		if (nr_pass == nr_cpus) {
>>>> -			report(!bad, "%s", testname);
>>>>  			if (i)
>>>> -				report_info("took more than %d ms", i * 100);
>>>> +				report_info("interrupts took more than %d ms", i * 100);
>>>> +			mdelay(100);  
>>> So this is the extra 100ms you mention in the commit message? I am not
>>> convinced this is the right way (see above) or even the right place
>>> (rather at the call site?) to wait. But at least it deserves a comment,
>>> I believe.  
>> I'm not sure moving it into the caller is the right thing to do. This is something
>> that has to do with how interrupts are asserted, not something that is specific to
>> one test.
>>
>> You are right about the comment, I'll add one.
>>
>> Thanks,
>> Alex
>>>>  			return;
>>>>  		}
>>>>  	}
>>>>  
>>>> +	report_info("interrupts timed-out (5s)");
>>>> +}
>>>> +
>>>> +static bool check_acked(cpumask_t *mask)
>>>> +{
>>>> +	int missing = 0, extra = 0, unexpected = 0;
>>>> +	bool pass = true;
>>>> +	int cpu;
>>>> +
>>>>  	for_each_present_cpu(cpu) {
>>>>  		if (cpumask_test_cpu(cpu, mask)) {
>>>>  			if (!acked[cpu])
>>>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
>>>>  			if (acked[cpu])
>>>>  				++unexpected;
>>>>  		}
>>>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>>>> +
>>>> +		if (bad_sender[cpu] != -1) {
>>>> +			report_info("cpu%d received IPI from wrong sender %d",
>>>> +					cpu, bad_sender[cpu]);
>>>> +			pass = false;
>>>> +		}
>>>> +
>>>> +		if (bad_irq[cpu] != -1) {
>>>> +			report_info("cpu%d received wrong irq %d",
>>>> +					cpu, bad_irq[cpu]);
>>>> +			pass = false;
>>>> +		}
>>>> +	}
>>>> +
>>>> +	if (missing || extra || unexpected) {
>>>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
>>>> +				missing, extra, unexpected);
>>>> +		pass = false;  
>>> Thanks, that so much easier to read now.
>>>
>>> Cheers,
>>> Andre
>>>  
>>>>  	}
>>>>  
>>>> -	report(false, "%s", testname);
>>>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
>>>> -		    missing, extra, unexpected);
>>>> +	return pass;
>>>>  }
>>>>  
>>>>  static void check_spurious(void)
>>>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
>>>>  	cpumask_clear(&mask);
>>>>  	cpumask_set_cpu(smp_processor_id(), &mask);
>>>>  	gic->ipi.send_self();
>>>> -	check_acked("IPI: self", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  }
>>>>  
>>>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
>>>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
>>>>  		cpumask_clear_cpu(i, &mask);
>>>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>>>> -	check_acked("IPI: directed", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  
>>>>  	report_prefix_push("broadcast");
>>>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
>>>>  	cpumask_copy(&mask, &cpu_present_mask);
>>>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
>>>>  	gic->ipi.send_broadcast();
>>>> -	check_acked("IPI: broadcast", &mask);
>>>> +	wait_for_interrupts(&mask);
>>>> +	report(check_acked(&mask), "Interrupts received");
>>>>  	report_prefix_pop();
>>>>  }
>>>>  
>>>>  
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Andre Przywara]

On Wed, 27 Jan 2021 16:00:46 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> On 1/27/21 3:10 PM, Andre Przywara wrote:
> > On Mon, 25 Jan 2021 17:27:35 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >
> > Hi Alex,
> >  
> >> On 12/18/20 3:52 PM, André Przywara wrote:  
> >>> On 17/12/2020 14:13, Alexandru Elisei wrote:    
> >>>> check_acked() has several peculiarities: is the only function among the
> >>>> check_* functions which calls report() directly, it does two things
> >>>> (waits for interrupts and checks for misfired interrupts) and it also
> >>>> mixes printf, report_info and report calls.
> >>>>
> >>>> check_acked() also reports a pass and returns as soon all the target CPUs
> >>>> have received interrupts, However, a CPU not having received an interrupt
> >>>> *now* does not guarantee not receiving an erroneous interrupt if we wait
> >>>> long enough.
> >>>>
> >>>> Rework the function by splitting it into two separate functions, each with
> >>>> a single responsibility: wait_for_interrupts(), which waits for the
> >>>> expected interrupts to fire, and check_acked() which checks that interrupts
> >>>> have been received as expected.
> >>>>
> >>>> wait_for_interrupts() also waits an extra 100 milliseconds after the
> >>>> expected interrupts have been received in an effort to make sure we don't
> >>>> miss misfiring interrupts.
> >>>>
> >>>> Splitting check_acked() into two functions will also allow us to
> >>>> customize the behavior of each function in the future more easily
> >>>> without using an unnecessarily long list of arguments for check_acked().    
> >>> Yes, splitting this up looks much better, in general this is a nice
> >>> cleanup, thank you!
> >>>
> >>> Some comments below:
> >>>    
> >>>> CC: Andre Przywara <andre.przywara@arm.com>
> >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>>> ---
> >>>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
> >>>>  1 file changed, 47 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/arm/gic.c b/arm/gic.c
> >>>> index ec733719c776..a9ef1a5def56 100644
> >>>> --- a/arm/gic.c
> >>>> +++ b/arm/gic.c
> >>>> @@ -62,41 +62,42 @@ static void stats_reset(void)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -static void check_acked(const char *testname, cpumask_t *mask)
> >>>> +static void wait_for_interrupts(cpumask_t *mask)
> >>>>  {
> >>>> -	int missing = 0, extra = 0, unexpected = 0;
> >>>>  	int nr_pass, cpu, i;
> >>>> -	bool bad = false;
> >>>>  
> >>>>  	/* Wait up to 5s for all interrupts to be delivered */
> >>>> -	for (i = 0; i < 50; ++i) {
> >>>> +	for (i = 0; i < 50; i++) {
> >>>>  		mdelay(100);
> >>>>  		nr_pass = 0;
> >>>>  		for_each_present_cpu(cpu) {
> >>>> +			/*
> >>>> +			 * A CPU having received more than one interrupts will
> >>>> +			 * show up in check_acked(), and no matter how long we
> >>>> +			 * wait it cannot un-receive it. Consider at least one
> >>>> +			 * interrupt as a pass.
> >>>> +			 */
> >>>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> >>>> -				acked[cpu] == 1 : acked[cpu] == 0;
> >>>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >>>> -
> >>>> -			if (bad_sender[cpu] != -1) {
> >>>> -				printf("cpu%d received IPI from wrong sender %d\n",
> >>>> -					cpu, bad_sender[cpu]);
> >>>> -				bad = true;
> >>>> -			}
> >>>> -
> >>>> -			if (bad_irq[cpu] != -1) {
> >>>> -				printf("cpu%d received wrong irq %d\n",
> >>>> -					cpu, bad_irq[cpu]);
> >>>> -				bad = true;
> >>>> -			}
> >>>> +				acked[cpu] >= 1 : acked[cpu] == 0;    
> >>> I wonder if this logic was already flawed to begin with: For interrupts
> >>> we expect to fire, we wait for up to 5 seconds (really that long?), but
> >>> for interrupts we expect *not* to fire we are OK if they don't show up
> >>> in the first 100 ms. That does not sound consistent.    
> >> There are two ways that I see to fix this:
> >>
> >> - Have the caller wait for however long it sees fit, and *after* that waiting
> >> period call wait_for_interrupts().
> >>
> >> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
> >> wait for the entire duration instead of until the expected interrupts have been
> >> received.
> >>
> >> Neither sounds appealing to me for inclusion in this patch set, since I want to
> >> concentrate on reworking check_acked() while keeping much of the current behaviour
> >> intact.
> >>  
> >>> I am wondering if we should *not* have the initial 100ms wait at all,
> >>> since most interrupts will fire immediately (especially in KVM). And
> >>> then have *one* extra wait for, say 100ms, to cover latecomers and
> >>> spurious interrupts.    
> >> I don't think it really matters where the 100 millisecond delay is in the loop.  
> > I think it does. I ran tests with 256 vCPUs, I think we support even
> > more, and running k-u-t on those setups is one of the cases where it
> > really matters and we can find real bugs.
> > So 100ms on their own does not sound much, but it means we wait at least
> > 25.6 seconds, even if everything is fine. I found this scary, confusing
> > and annoying (in that order), so was wondering if we can avoid that.  
> 
> I'm not sure where that 25.6 second delay is coming from. The mdelay() is at the
> start of the for loop, *before* the for_each_cpu() loop, so it's not executed for
> each VCPU.

I think that comes from some other patches of mine, which call the test
on *each* VCPU. There this small delay adds up.

> I've also run the ipi test on my rockpro64 with 256 vcpus with kvmtool and I
> didn't notice any unexpected delays.

Thanks for doing that.

> >> If
> >> we call wait_for_interrupts() to actually check that interrupts have fired (as
> >> opposed to checking that they haven't been asserted), then at most we save 100ms
> >> when they are asserted before the start of the loop. I don't think the GIC spec
> >> guarantees that interrupts written to the LR registers will be presented to the
> >> CPU after the guest resumes,  
> > I don't know if the spec says anything about it, I guess it would be
> > out of scope to do so there anyway, but AFAIK this is exactly how it's
> > implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
> > before the guest executes the first instruction (that ELR_EL2 points
> > to), and raises the IRQ exception in EL1.
> >  
> >> so it is conceivable that there might be a delay,  
> > The only practical reason for a delay would be PSTATE.I being set, or
> > the GICV being disabled, I think.
> >
> > I would say one would expect interrupts to fire *immediately*, and
> > allowing them 100ms slack does not sound like the right thing. If there
> > is some delay, I would at least like to know about it. And I would
> > grant them a few instructions delay, at best.  
> 
> I think you're forgetting the fact that the interrupts are delivered to the other
> VCPUs, not to the VCPU that is calling wait_for_interrupts().

That is one use case, but for the IPI self test we do this on the
same VCPU. My concern was just that those functions are generic, so
should cater for any kind of interrupt tests thrown at them. And as
mentioned above, I had tests which got delayed by the mandatory wait.

For the records: I was coming from my manual testing during the VGIC
development/rework a few years back, where some bugs only showed under
(heavy) *parallel* interrupt load. So I guess I am a bit biased when it
comes to that.

But this is indeed of no concern for what we currently have, so I guess
I will just revisit this as needed, should we get more sophisticated
tests.

Which means the changes here are fine on their own and are definitely
an improvement, so we should go ahead with that.

Cheers,
Andre


> So it's the
> interrupt handlers running on the other VCPUs that update acked, which afterwards
> is read in wait_for_interrupts().
> 
> As an experiment, I moved the mdelay() at the end of the loop and I tried running
> the IPI test with 2 and 256 VCPUs:
> 
> $ taskset -c 4,5 ~/lkvm-static run -c2 -m128 -f arm/gic.flat -p 'ipi'
>   # lkvm run --firmware arm/gic.flat -m 128 -c 2 --name guest-1720
>   Info: Placing fdt at 0x80200000 - 0x80210000
> chr_testdev_init: chr-testdev: can't find a virtio-console
> PASS: gicv3: ipi: self: Interrupts received
> INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
> PASS: gicv3: ipi: target-list: Interrupts received
> INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
> PASS: gicv3: ipi: broadcast: Interrupts received
> SUMMARY: 3 tests
> 
> $ taskset -c 4,5 ~/lkvm-static run -c256 -m128 -f arm/gic.flat -p 'ipi'
>   # lkvm run --firmware arm/gic.flat -m 128 -c 256 --name guest-2000
>   Info: Placing fdt at 0x80200000 - 0x80210000
>   # Warning: The maximum recommended amount of VCPUs is 6
> chr_testdev_init: chr-testdev: can't find a virtio-console
> PASS: gicv3: ipi: self: Interrupts received
> INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
> PASS: gicv3: ipi: target-list: Interrupts received
> INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
> PASS: gicv3: ipi: broadcast: Interrupts received
> SUMMARY: 3 tests
> 
> For the 256 VCPUs test, on two runs I got the "interrupts took more than 100 ms"
> message for target-list, on one test I didn't (but for broadcast I always got the
> message).
> 
> For the 2 VCPUs test, I always got the message (tried it 5 times).
> 
> Thanks,
> Alex
> >
> > If you still think you need that delay, because everything else would
> > be too complicated (at least for this iteration), then please make it
> > *much* smaller (< 1us).
> >
> > Cheers,
> > Andre
> >
> >  
> >> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
> >>
> >> There are two reasons I chose the approach of having the delay at the start of the
> >> loop:
> >>
> >> 1. To preserve the current behaviour.
> >>
> >> 2. To match what the timer test those (see gic_timer_check_state()). I am also
> >> thinking that maybe at some point we could unify these test-independent functions
> >> in the gic driver.
> >>
> >> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
> >> as a parameter to the function if needed (if I remember correctly, you needed a
> >> shorter waiting period for your GIC tests).
> >>  
> >>> But this might be a topic for some extra work/patch?    
> >> Yes, I would rather make this changes when we have an actual test that needs them.
> >>  
> >>>    
> >>>>  		}
> >>>> +
> >>>>  		if (nr_pass == nr_cpus) {
> >>>> -			report(!bad, "%s", testname);
> >>>>  			if (i)
> >>>> -				report_info("took more than %d ms", i * 100);
> >>>> +				report_info("interrupts took more than %d ms", i * 100);
> >>>> +			mdelay(100);    
> >>> So this is the extra 100ms you mention in the commit message? I am not
> >>> convinced this is the right way (see above) or even the right place
> >>> (rather at the call site?) to wait. But at least it deserves a comment,
> >>> I believe.    
> >> I'm not sure moving it into the caller is the right thing to do. This is something
> >> that has to do with how interrupts are asserted, not something that is specific to
> >> one test.
> >>
> >> You are right about the comment, I'll add one.
> >>
> >> Thanks,
> >> Alex  
> >>>>  			return;
> >>>>  		}
> >>>>  	}
> >>>>  
> >>>> +	report_info("interrupts timed-out (5s)");
> >>>> +}
> >>>> +
> >>>> +static bool check_acked(cpumask_t *mask)
> >>>> +{
> >>>> +	int missing = 0, extra = 0, unexpected = 0;
> >>>> +	bool pass = true;
> >>>> +	int cpu;
> >>>> +
> >>>>  	for_each_present_cpu(cpu) {
> >>>>  		if (cpumask_test_cpu(cpu, mask)) {
> >>>>  			if (!acked[cpu])
> >>>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
> >>>>  			if (acked[cpu])
> >>>>  				++unexpected;
> >>>>  		}
> >>>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >>>> +
> >>>> +		if (bad_sender[cpu] != -1) {
> >>>> +			report_info("cpu%d received IPI from wrong sender %d",
> >>>> +					cpu, bad_sender[cpu]);
> >>>> +			pass = false;
> >>>> +		}
> >>>> +
> >>>> +		if (bad_irq[cpu] != -1) {
> >>>> +			report_info("cpu%d received wrong irq %d",
> >>>> +					cpu, bad_irq[cpu]);
> >>>> +			pass = false;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	if (missing || extra || unexpected) {
> >>>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> >>>> +				missing, extra, unexpected);
> >>>> +		pass = false;    
> >>> Thanks, that so much easier to read now.
> >>>
> >>> Cheers,
> >>> Andre
> >>>    
> >>>>  	}
> >>>>  
> >>>> -	report(false, "%s", testname);
> >>>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> >>>> -		    missing, extra, unexpected);
> >>>> +	return pass;
> >>>>  }
> >>>>  
> >>>>  static void check_spurious(void)
> >>>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
> >>>>  	cpumask_clear(&mask);
> >>>>  	cpumask_set_cpu(smp_processor_id(), &mask);
> >>>>  	gic->ipi.send_self();
> >>>> -	check_acked("IPI: self", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  }
> >>>>  
> >>>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
> >>>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> >>>>  		cpumask_clear_cpu(i, &mask);
> >>>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> >>>> -	check_acked("IPI: directed", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  
> >>>>  	report_prefix_push("broadcast");
> >>>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
> >>>>  	cpumask_copy(&mask, &cpu_present_mask);
> >>>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
> >>>>  	gic->ipi.send_broadcast();
> >>>> -	check_acked("IPI: broadcast", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  }
> >>>>  
> >>>>    

Re: [kvm-unit-tests PATCH v2 08/12] arm/arm64: gic: Split check_acked() into two functions

[Andre Przywara]

On Wed, 27 Jan 2021 16:00:46 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

> On 1/27/21 3:10 PM, Andre Przywara wrote:
> > On Mon, 25 Jan 2021 17:27:35 +0000
> > Alexandru Elisei <alexandru.elisei@arm.com> wrote:
> >
> > Hi Alex,
> >  
> >> On 12/18/20 3:52 PM, André Przywara wrote:  
> >>> On 17/12/2020 14:13, Alexandru Elisei wrote:    
> >>>> check_acked() has several peculiarities: is the only function among the
> >>>> check_* functions which calls report() directly, it does two things
> >>>> (waits for interrupts and checks for misfired interrupts) and it also
> >>>> mixes printf, report_info and report calls.
> >>>>
> >>>> check_acked() also reports a pass and returns as soon all the target CPUs
> >>>> have received interrupts, However, a CPU not having received an interrupt
> >>>> *now* does not guarantee not receiving an erroneous interrupt if we wait
> >>>> long enough.
> >>>>
> >>>> Rework the function by splitting it into two separate functions, each with
> >>>> a single responsibility: wait_for_interrupts(), which waits for the
> >>>> expected interrupts to fire, and check_acked() which checks that interrupts
> >>>> have been received as expected.
> >>>>
> >>>> wait_for_interrupts() also waits an extra 100 milliseconds after the
> >>>> expected interrupts have been received in an effort to make sure we don't
> >>>> miss misfiring interrupts.
> >>>>
> >>>> Splitting check_acked() into two functions will also allow us to
> >>>> customize the behavior of each function in the future more easily
> >>>> without using an unnecessarily long list of arguments for check_acked().    
> >>> Yes, splitting this up looks much better, in general this is a nice
> >>> cleanup, thank you!
> >>>
> >>> Some comments below:
> >>>    
> >>>> CC: Andre Przywara <andre.przywara@arm.com>
> >>>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> >>>> ---
> >>>>  arm/gic.c | 73 +++++++++++++++++++++++++++++++++++--------------------
> >>>>  1 file changed, 47 insertions(+), 26 deletions(-)
> >>>>
> >>>> diff --git a/arm/gic.c b/arm/gic.c
> >>>> index ec733719c776..a9ef1a5def56 100644
> >>>> --- a/arm/gic.c
> >>>> +++ b/arm/gic.c
> >>>> @@ -62,41 +62,42 @@ static void stats_reset(void)
> >>>>  	}
> >>>>  }
> >>>>  
> >>>> -static void check_acked(const char *testname, cpumask_t *mask)
> >>>> +static void wait_for_interrupts(cpumask_t *mask)
> >>>>  {
> >>>> -	int missing = 0, extra = 0, unexpected = 0;
> >>>>  	int nr_pass, cpu, i;
> >>>> -	bool bad = false;
> >>>>  
> >>>>  	/* Wait up to 5s for all interrupts to be delivered */
> >>>> -	for (i = 0; i < 50; ++i) {
> >>>> +	for (i = 0; i < 50; i++) {
> >>>>  		mdelay(100);
> >>>>  		nr_pass = 0;
> >>>>  		for_each_present_cpu(cpu) {
> >>>> +			/*
> >>>> +			 * A CPU having received more than one interrupts will
> >>>> +			 * show up in check_acked(), and no matter how long we
> >>>> +			 * wait it cannot un-receive it. Consider at least one
> >>>> +			 * interrupt as a pass.
> >>>> +			 */
> >>>>  			nr_pass += cpumask_test_cpu(cpu, mask) ?
> >>>> -				acked[cpu] == 1 : acked[cpu] == 0;
> >>>> -			smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >>>> -
> >>>> -			if (bad_sender[cpu] != -1) {
> >>>> -				printf("cpu%d received IPI from wrong sender %d\n",
> >>>> -					cpu, bad_sender[cpu]);
> >>>> -				bad = true;
> >>>> -			}
> >>>> -
> >>>> -			if (bad_irq[cpu] != -1) {
> >>>> -				printf("cpu%d received wrong irq %d\n",
> >>>> -					cpu, bad_irq[cpu]);
> >>>> -				bad = true;
> >>>> -			}
> >>>> +				acked[cpu] >= 1 : acked[cpu] == 0;    
> >>> I wonder if this logic was already flawed to begin with: For interrupts
> >>> we expect to fire, we wait for up to 5 seconds (really that long?), but
> >>> for interrupts we expect *not* to fire we are OK if they don't show up
> >>> in the first 100 ms. That does not sound consistent.    
> >> There are two ways that I see to fix this:
> >>
> >> - Have the caller wait for however long it sees fit, and *after* that waiting
> >> period call wait_for_interrupts().
> >>
> >> - Pass a flag to wait_for_interrupts() to specify that the behaviour should be to
> >> wait for the entire duration instead of until the expected interrupts have been
> >> received.
> >>
> >> Neither sounds appealing to me for inclusion in this patch set, since I want to
> >> concentrate on reworking check_acked() while keeping much of the current behaviour
> >> intact.
> >>  
> >>> I am wondering if we should *not* have the initial 100ms wait at all,
> >>> since most interrupts will fire immediately (especially in KVM). And
> >>> then have *one* extra wait for, say 100ms, to cover latecomers and
> >>> spurious interrupts.    
> >> I don't think it really matters where the 100 millisecond delay is in the loop.  
> > I think it does. I ran tests with 256 vCPUs, I think we support even
> > more, and running k-u-t on those setups is one of the cases where it
> > really matters and we can find real bugs.
> > So 100ms on their own does not sound much, but it means we wait at least
> > 25.6 seconds, even if everything is fine. I found this scary, confusing
> > and annoying (in that order), so was wondering if we can avoid that.  
> 
> I'm not sure where that 25.6 second delay is coming from. The mdelay() is at the
> start of the for loop, *before* the for_each_cpu() loop, so it's not executed for
> each VCPU.

I think that comes from some other patches of mine, which call the test
on *each* VCPU. There this small delay adds up.

> I've also run the ipi test on my rockpro64 with 256 vcpus with kvmtool and I
> didn't notice any unexpected delays.

Thanks for doing that.

> >> If
> >> we call wait_for_interrupts() to actually check that interrupts have fired (as
> >> opposed to checking that they haven't been asserted), then at most we save 100ms
> >> when they are asserted before the start of the loop. I don't think the GIC spec
> >> guarantees that interrupts written to the LR registers will be presented to the
> >> CPU after the guest resumes,  
> > I don't know if the spec says anything about it, I guess it would be
> > out of scope to do so there anyway, but AFAIK this is exactly how it's
> > implemented: when we drop to EL1 with the VGIC armed, the GIC jumps in
> > before the guest executes the first instruction (that ELR_EL2 points
> > to), and raises the IRQ exception in EL1.
> >  
> >> so it is conceivable that there might be a delay,  
> > The only practical reason for a delay would be PSTATE.I being set, or
> > the GICV being disabled, I think.
> >
> > I would say one would expect interrupts to fire *immediately*, and
> > allowing them 100ms slack does not sound like the right thing. If there
> > is some delay, I would at least like to know about it. And I would
> > grant them a few instructions delay, at best.  
> 
> I think you're forgetting the fact that the interrupts are delivered to the other
> VCPUs, not to the VCPU that is calling wait_for_interrupts().

That is one use case, but for the IPI self test we do this on the
same VCPU. My concern was just that those functions are generic, so
should cater for any kind of interrupt tests thrown at them. And as
mentioned above, I had tests which got delayed by the mandatory wait.

For the records: I was coming from my manual testing during the VGIC
development/rework a few years back, where some bugs only showed under
(heavy) *parallel* interrupt load. So I guess I am a bit biased when it
comes to that.

But this is indeed of no concern for what we currently have, so I guess
I will just revisit this as needed, should we get more sophisticated
tests.

Which means the changes here are fine on their own and are definitely
an improvement, so we should go ahead with that.

Cheers,
Andre


> So it's the
> interrupt handlers running on the other VCPUs that update acked, which afterwards
> is read in wait_for_interrupts().
> 
> As an experiment, I moved the mdelay() at the end of the loop and I tried running
> the IPI test with 2 and 256 VCPUs:
> 
> $ taskset -c 4,5 ~/lkvm-static run -c2 -m128 -f arm/gic.flat -p 'ipi'
>   # lkvm run --firmware arm/gic.flat -m 128 -c 2 --name guest-1720
>   Info: Placing fdt at 0x80200000 - 0x80210000
> chr_testdev_init: chr-testdev: can't find a virtio-console
> PASS: gicv3: ipi: self: Interrupts received
> INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
> PASS: gicv3: ipi: target-list: Interrupts received
> INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
> PASS: gicv3: ipi: broadcast: Interrupts received
> SUMMARY: 3 tests
> 
> $ taskset -c 4,5 ~/lkvm-static run -c256 -m128 -f arm/gic.flat -p 'ipi'
>   # lkvm run --firmware arm/gic.flat -m 128 -c 256 --name guest-2000
>   Info: Placing fdt at 0x80200000 - 0x80210000
>   # Warning: The maximum recommended amount of VCPUs is 6
> chr_testdev_init: chr-testdev: can't find a virtio-console
> PASS: gicv3: ipi: self: Interrupts received
> INFO: gicv3: ipi: target-list: interrupts took more than 100 ms
> PASS: gicv3: ipi: target-list: Interrupts received
> INFO: gicv3: ipi: broadcast: interrupts took more than 100 ms
> PASS: gicv3: ipi: broadcast: Interrupts received
> SUMMARY: 3 tests
> 
> For the 256 VCPUs test, on two runs I got the "interrupts took more than 100 ms"
> message for target-list, on one test I didn't (but for broadcast I always got the
> message).
> 
> For the 2 VCPUs test, I always got the message (tried it 5 times).
> 
> Thanks,
> Alex
> >
> > If you still think you need that delay, because everything else would
> > be too complicated (at least for this iteration), then please make it
> > *much* smaller (< 1us).
> >
> > Cheers,
> > Andre
> >
> >  
> >> thus ending up in waiting the extra 100ms even if the delay is at the end of the loop.
> >>
> >> There are two reasons I chose the approach of having the delay at the start of the
> >> loop:
> >>
> >> 1. To preserve the current behaviour.
> >>
> >> 2. To match what the timer test those (see gic_timer_check_state()). I am also
> >> thinking that maybe at some point we could unify these test-independent functions
> >> in the gic driver.
> >>
> >> As for the 5 seconds delay, I think we can come up with a patch to pass the delay
> >> as a parameter to the function if needed (if I remember correctly, you needed a
> >> shorter waiting period for your GIC tests).
> >>  
> >>> But this might be a topic for some extra work/patch?    
> >> Yes, I would rather make this changes when we have an actual test that needs them.
> >>  
> >>>    
> >>>>  		}
> >>>> +
> >>>>  		if (nr_pass == nr_cpus) {
> >>>> -			report(!bad, "%s", testname);
> >>>>  			if (i)
> >>>> -				report_info("took more than %d ms", i * 100);
> >>>> +				report_info("interrupts took more than %d ms", i * 100);
> >>>> +			mdelay(100);    
> >>> So this is the extra 100ms you mention in the commit message? I am not
> >>> convinced this is the right way (see above) or even the right place
> >>> (rather at the call site?) to wait. But at least it deserves a comment,
> >>> I believe.    
> >> I'm not sure moving it into the caller is the right thing to do. This is something
> >> that has to do with how interrupts are asserted, not something that is specific to
> >> one test.
> >>
> >> You are right about the comment, I'll add one.
> >>
> >> Thanks,
> >> Alex  
> >>>>  			return;
> >>>>  		}
> >>>>  	}
> >>>>  
> >>>> +	report_info("interrupts timed-out (5s)");
> >>>> +}
> >>>> +
> >>>> +static bool check_acked(cpumask_t *mask)
> >>>> +{
> >>>> +	int missing = 0, extra = 0, unexpected = 0;
> >>>> +	bool pass = true;
> >>>> +	int cpu;
> >>>> +
> >>>>  	for_each_present_cpu(cpu) {
> >>>>  		if (cpumask_test_cpu(cpu, mask)) {
> >>>>  			if (!acked[cpu])
> >>>> @@ -107,11 +108,28 @@ static void check_acked(const char *testname, cpumask_t *mask)
> >>>>  			if (acked[cpu])
> >>>>  				++unexpected;
> >>>>  		}
> >>>> +		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
> >>>> +
> >>>> +		if (bad_sender[cpu] != -1) {
> >>>> +			report_info("cpu%d received IPI from wrong sender %d",
> >>>> +					cpu, bad_sender[cpu]);
> >>>> +			pass = false;
> >>>> +		}
> >>>> +
> >>>> +		if (bad_irq[cpu] != -1) {
> >>>> +			report_info("cpu%d received wrong irq %d",
> >>>> +					cpu, bad_irq[cpu]);
> >>>> +			pass = false;
> >>>> +		}
> >>>> +	}
> >>>> +
> >>>> +	if (missing || extra || unexpected) {
> >>>> +		report_info("ACKS: missing=%d extra=%d unexpected=%d",
> >>>> +				missing, extra, unexpected);
> >>>> +		pass = false;    
> >>> Thanks, that so much easier to read now.
> >>>
> >>> Cheers,
> >>> Andre
> >>>    
> >>>>  	}
> >>>>  
> >>>> -	report(false, "%s", testname);
> >>>> -	report_info("Timed-out (5s). ACKS: missing=%d extra=%d unexpected=%d",
> >>>> -		    missing, extra, unexpected);
> >>>> +	return pass;
> >>>>  }
> >>>>  
> >>>>  static void check_spurious(void)
> >>>> @@ -303,7 +321,8 @@ static void ipi_test_self(void)
> >>>>  	cpumask_clear(&mask);
> >>>>  	cpumask_set_cpu(smp_processor_id(), &mask);
> >>>>  	gic->ipi.send_self();
> >>>> -	check_acked("IPI: self", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  }
> >>>>  
> >>>> @@ -318,7 +337,8 @@ static void ipi_test_smp(void)
> >>>>  	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> >>>>  		cpumask_clear_cpu(i, &mask);
> >>>>  	gic_ipi_send_mask(IPI_IRQ, &mask);
> >>>> -	check_acked("IPI: directed", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  
> >>>>  	report_prefix_push("broadcast");
> >>>> @@ -326,7 +346,8 @@ static void ipi_test_smp(void)
> >>>>  	cpumask_copy(&mask, &cpu_present_mask);
> >>>>  	cpumask_clear_cpu(smp_processor_id(), &mask);
> >>>>  	gic->ipi.send_broadcast();
> >>>> -	check_acked("IPI: broadcast", &mask);
> >>>> +	wait_for_interrupts(&mask);
> >>>> +	report(check_acked(&mask), "Interrupts received");
> >>>>  	report_prefix_pop();
> >>>>  }
> >>>>  
> >>>>    

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 09/12] arm/arm64: gic: Make check_acked() more generic

[Alexandru Elisei]

Testing that an interrupt is received as expected is done in three places:
in check_ipi_sender(), check_irqnr() and check_acked(). check_irqnr()
compares the interrupt ID with IPI_IRQ and records a failure in bad_irq,
and check_ipi_sender() compares the sender with IPI_SENDER and writes to
bad_sender when they don't match.

Let's move all the checks to check_acked() by renaming
bad_sender->irq_sender and bad_irq->irq_number and changing their semantics
so they record the interrupt sender, respectively the irq number.
check_acked() now takes two new parameters: the expected interrupt number
and sender.

This has two distinct advantages:

1. check_acked() and ipi_handler() can now be used for interrupts other
   than IPIs.
2. Correctness checks are consolidated in one function.

CC: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 68 +++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a9ef1a5def56..fb91861900b7 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -35,7 +35,7 @@ struct gic {
 
 static struct gic *gic;
 static int acked[NR_CPUS], spurious[NR_CPUS];
-static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
+static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
 static cpumask_t ready;
 
 static void nr_cpu_check(int nr)
@@ -57,8 +57,8 @@ static void stats_reset(void)
 
 	for (i = 0; i < nr_cpus; ++i) {
 		acked[i] = 0;
-		bad_sender[i] = -1;
-		bad_irq[i] = -1;
+		irq_sender[i] = -1;
+		irq_number[i] = -1;
 	}
 }
 
@@ -92,9 +92,10 @@ static void wait_for_interrupts(cpumask_t *mask)
 	report_info("interrupts timed-out (5s)");
 }
 
-static bool check_acked(cpumask_t *mask)
+static bool check_acked(cpumask_t *mask, int sender, int irqnum)
 {
 	int missing = 0, extra = 0, unexpected = 0;
+	bool has_gicv2 = (gic_version() == 2);
 	bool pass = true;
 	int cpu;
 
@@ -108,17 +109,19 @@ static bool check_acked(cpumask_t *mask)
 			if (acked[cpu])
 				++unexpected;
 		}
+		if (!acked[cpu])
+			continue;
 		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
 
-		if (bad_sender[cpu] != -1) {
+		if (has_gicv2 && irq_sender[cpu] != sender) {
 			report_info("cpu%d received IPI from wrong sender %d",
-					cpu, bad_sender[cpu]);
+					cpu, irq_sender[cpu]);
 			pass = false;
 		}
 
-		if (bad_irq[cpu] != -1) {
+		if (irq_number[cpu] != irqnum) {
 			report_info("cpu%d received wrong irq %d",
-					cpu, bad_irq[cpu]);
+					cpu, irq_number[cpu]);
 			pass = false;
 		}
 	}
@@ -143,26 +146,18 @@ static void check_spurious(void)
 	}
 }
 
-static void check_ipi_sender(u32 irqstat, int sender)
+static int gic_get_sender(int irqstat)
 {
-	if (gic_version() == 2) {
-		int src = (irqstat >> 10) & 7;
-
-		if (src != sender)
-			bad_sender[smp_processor_id()] = src;
-	}
-}
-
-static void check_irqnr(u32 irqnr)
-{
-	if (irqnr != IPI_IRQ)
-		bad_irq[smp_processor_id()] = irqnr;
+	if (gic_version() == 2)
+		return (irqstat >> 10) & 7;
+	return -1;
 }
 
 static void ipi_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
+	int this_cpu = smp_processor_id();
 
 	if (irqnr != GICC_INT_SPURIOUS) {
 		gic_write_eoir(irqstat);
@@ -173,12 +168,12 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		check_ipi_sender(irqstat, IPI_SENDER);
-		check_irqnr(irqnr);
+		irq_sender[this_cpu] = gic_get_sender(irqstat);
+		irq_number[this_cpu] = irqnr;
 		smp_wmb(); /* pairs with smp_rmb in check_acked */
-		++acked[smp_processor_id()];
+		++acked[this_cpu];
 	} else {
-		++spurious[smp_processor_id()];
+		++spurious[this_cpu];
 	}
 
 	/* Wait for writes to acked/spurious to complete */
@@ -314,40 +309,42 @@ static void gicv3_ipi_send_broadcast(void)
 
 static void ipi_test_self(void)
 {
+	int this_cpu = smp_processor_id();
 	cpumask_t mask;
 
 	report_prefix_push("self");
 	stats_reset();
 	cpumask_clear(&mask);
-	cpumask_set_cpu(smp_processor_id(), &mask);
+	cpumask_set_cpu(this_cpu, &mask);
 	gic->ipi.send_self();
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 }
 
 static void ipi_test_smp(void)
 {
+	int this_cpu = smp_processor_id();
 	cpumask_t mask;
 	int i;
 
 	report_prefix_push("target-list");
 	stats_reset();
 	cpumask_copy(&mask, &cpu_present_mask);
-	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
+	for (i = this_cpu & 1; i < nr_cpus; i += 2)
 		cpumask_clear_cpu(i, &mask);
 	gic_ipi_send_mask(IPI_IRQ, &mask);
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 
 	report_prefix_push("broadcast");
 	stats_reset();
 	cpumask_copy(&mask, &cpu_present_mask);
-	cpumask_clear_cpu(smp_processor_id(), &mask);
+	cpumask_clear_cpu(this_cpu, &mask);
 	gic->ipi.send_broadcast();
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 }
 
@@ -396,6 +393,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
+	int this_cpu = smp_processor_id();
 
 	if (irqnr != GICC_INT_SPURIOUS) {
 		void *base;
@@ -408,11 +406,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
-		check_ipi_sender(irqstat, smp_processor_id());
-		check_irqnr(irqnr);
-		++acked[smp_processor_id()];
+		irq_sender[this_cpu] = gic_get_sender(irqstat);
+		irq_number[this_cpu] = irqnr;
+		++acked[this_cpu];
 	} else {
-		++spurious[smp_processor_id()];
+		++spurious[this_cpu];
 	}
 }
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 09/12] arm/arm64: gic: Make check_acked() more generic

[Alexandru Elisei]

Testing that an interrupt is received as expected is done in three places:
in check_ipi_sender(), check_irqnr() and check_acked(). check_irqnr()
compares the interrupt ID with IPI_IRQ and records a failure in bad_irq,
and check_ipi_sender() compares the sender with IPI_SENDER and writes to
bad_sender when they don't match.

Let's move all the checks to check_acked() by renaming
bad_sender->irq_sender and bad_irq->irq_number and changing their semantics
so they record the interrupt sender, respectively the irq number.
check_acked() now takes two new parameters: the expected interrupt number
and sender.

This has two distinct advantages:

1. check_acked() and ipi_handler() can now be used for interrupts other
   than IPIs.
2. Correctness checks are consolidated in one function.

CC: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 68 +++++++++++++++++++++++++++----------------------------
 1 file changed, 33 insertions(+), 35 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index a9ef1a5def56..fb91861900b7 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -35,7 +35,7 @@ struct gic {
 
 static struct gic *gic;
 static int acked[NR_CPUS], spurious[NR_CPUS];
-static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
+static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
 static cpumask_t ready;
 
 static void nr_cpu_check(int nr)
@@ -57,8 +57,8 @@ static void stats_reset(void)
 
 	for (i = 0; i < nr_cpus; ++i) {
 		acked[i] = 0;
-		bad_sender[i] = -1;
-		bad_irq[i] = -1;
+		irq_sender[i] = -1;
+		irq_number[i] = -1;
 	}
 }
 
@@ -92,9 +92,10 @@ static void wait_for_interrupts(cpumask_t *mask)
 	report_info("interrupts timed-out (5s)");
 }
 
-static bool check_acked(cpumask_t *mask)
+static bool check_acked(cpumask_t *mask, int sender, int irqnum)
 {
 	int missing = 0, extra = 0, unexpected = 0;
+	bool has_gicv2 = (gic_version() == 2);
 	bool pass = true;
 	int cpu;
 
@@ -108,17 +109,19 @@ static bool check_acked(cpumask_t *mask)
 			if (acked[cpu])
 				++unexpected;
 		}
+		if (!acked[cpu])
+			continue;
 		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
 
-		if (bad_sender[cpu] != -1) {
+		if (has_gicv2 && irq_sender[cpu] != sender) {
 			report_info("cpu%d received IPI from wrong sender %d",
-					cpu, bad_sender[cpu]);
+					cpu, irq_sender[cpu]);
 			pass = false;
 		}
 
-		if (bad_irq[cpu] != -1) {
+		if (irq_number[cpu] != irqnum) {
 			report_info("cpu%d received wrong irq %d",
-					cpu, bad_irq[cpu]);
+					cpu, irq_number[cpu]);
 			pass = false;
 		}
 	}
@@ -143,26 +146,18 @@ static void check_spurious(void)
 	}
 }
 
-static void check_ipi_sender(u32 irqstat, int sender)
+static int gic_get_sender(int irqstat)
 {
-	if (gic_version() == 2) {
-		int src = (irqstat >> 10) & 7;
-
-		if (src != sender)
-			bad_sender[smp_processor_id()] = src;
-	}
-}
-
-static void check_irqnr(u32 irqnr)
-{
-	if (irqnr != IPI_IRQ)
-		bad_irq[smp_processor_id()] = irqnr;
+	if (gic_version() == 2)
+		return (irqstat >> 10) & 7;
+	return -1;
 }
 
 static void ipi_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
+	int this_cpu = smp_processor_id();
 
 	if (irqnr != GICC_INT_SPURIOUS) {
 		gic_write_eoir(irqstat);
@@ -173,12 +168,12 @@ static void ipi_handler(struct pt_regs *regs __unused)
 		 */
 		if (gic_version() == 2)
 			smp_rmb();
-		check_ipi_sender(irqstat, IPI_SENDER);
-		check_irqnr(irqnr);
+		irq_sender[this_cpu] = gic_get_sender(irqstat);
+		irq_number[this_cpu] = irqnr;
 		smp_wmb(); /* pairs with smp_rmb in check_acked */
-		++acked[smp_processor_id()];
+		++acked[this_cpu];
 	} else {
-		++spurious[smp_processor_id()];
+		++spurious[this_cpu];
 	}
 
 	/* Wait for writes to acked/spurious to complete */
@@ -314,40 +309,42 @@ static void gicv3_ipi_send_broadcast(void)
 
 static void ipi_test_self(void)
 {
+	int this_cpu = smp_processor_id();
 	cpumask_t mask;
 
 	report_prefix_push("self");
 	stats_reset();
 	cpumask_clear(&mask);
-	cpumask_set_cpu(smp_processor_id(), &mask);
+	cpumask_set_cpu(this_cpu, &mask);
 	gic->ipi.send_self();
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 }
 
 static void ipi_test_smp(void)
 {
+	int this_cpu = smp_processor_id();
 	cpumask_t mask;
 	int i;
 
 	report_prefix_push("target-list");
 	stats_reset();
 	cpumask_copy(&mask, &cpu_present_mask);
-	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
+	for (i = this_cpu & 1; i < nr_cpus; i += 2)
 		cpumask_clear_cpu(i, &mask);
 	gic_ipi_send_mask(IPI_IRQ, &mask);
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 
 	report_prefix_push("broadcast");
 	stats_reset();
 	cpumask_copy(&mask, &cpu_present_mask);
-	cpumask_clear_cpu(smp_processor_id(), &mask);
+	cpumask_clear_cpu(this_cpu, &mask);
 	gic->ipi.send_broadcast();
 	wait_for_interrupts(&mask);
-	report(check_acked(&mask), "Interrupts received");
+	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
 	report_prefix_pop();
 }
 
@@ -396,6 +393,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
+	int this_cpu = smp_processor_id();
 
 	if (irqnr != GICC_INT_SPURIOUS) {
 		void *base;
@@ -408,11 +406,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 
 		writel(val, base + GICD_ICACTIVER);
 
-		check_ipi_sender(irqstat, smp_processor_id());
-		check_irqnr(irqnr);
-		++acked[smp_processor_id()];
+		irq_sender[this_cpu] = gic_get_sender(irqstat);
+		irq_number[this_cpu] = irqnr;
+		++acked[this_cpu];
 	} else {
-		++spurious[smp_processor_id()];
+		++spurious[this_cpu];
 	}
 }
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 09/12] arm/arm64: gic: Make check_acked() more generic

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> Testing that an interrupt is received as expected is done in three places:
> in check_ipi_sender(), check_irqnr() and check_acked(). check_irqnr()
> compares the interrupt ID with IPI_IRQ and records a failure in bad_irq,
> and check_ipi_sender() compares the sender with IPI_SENDER and writes to
> bad_sender when they don't match.
> 
> Let's move all the checks to check_acked() by renaming
> bad_sender->irq_sender and bad_irq->irq_number and changing their semantics
> so they record the interrupt sender, respectively the irq number.
> check_acked() now takes two new parameters: the expected interrupt number
> and sender.
> 
> This has two distinct advantages:
> 
> 1. check_acked() and ipi_handler() can now be used for interrupts other
>    than IPIs.
> 2. Correctness checks are consolidated in one function.
> 
> CC: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Look all like valid transformations to me, and indeed it's more reusable
now:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/gic.c | 68 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index a9ef1a5def56..fb91861900b7 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -35,7 +35,7 @@ struct gic {
>  
>  static struct gic *gic;
>  static int acked[NR_CPUS], spurious[NR_CPUS];
> -static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> +static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
>  static cpumask_t ready;
>  
>  static void nr_cpu_check(int nr)
> @@ -57,8 +57,8 @@ static void stats_reset(void)
>  
>  	for (i = 0; i < nr_cpus; ++i) {
>  		acked[i] = 0;
> -		bad_sender[i] = -1;
> -		bad_irq[i] = -1;
> +		irq_sender[i] = -1;
> +		irq_number[i] = -1;
>  	}
>  }
>  
> @@ -92,9 +92,10 @@ static void wait_for_interrupts(cpumask_t *mask)
>  	report_info("interrupts timed-out (5s)");
>  }
>  
> -static bool check_acked(cpumask_t *mask)
> +static bool check_acked(cpumask_t *mask, int sender, int irqnum)
>  {
>  	int missing = 0, extra = 0, unexpected = 0;
> +	bool has_gicv2 = (gic_version() == 2);
>  	bool pass = true;
>  	int cpu;
>  
> @@ -108,17 +109,19 @@ static bool check_acked(cpumask_t *mask)
>  			if (acked[cpu])
>  				++unexpected;
>  		}
> +		if (!acked[cpu])
> +			continue;
>  		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>  
> -		if (bad_sender[cpu] != -1) {
> +		if (has_gicv2 && irq_sender[cpu] != sender) {
>  			report_info("cpu%d received IPI from wrong sender %d",
> -					cpu, bad_sender[cpu]);
> +					cpu, irq_sender[cpu]);
>  			pass = false;
>  		}
>  
> -		if (bad_irq[cpu] != -1) {
> +		if (irq_number[cpu] != irqnum) {
>  			report_info("cpu%d received wrong irq %d",
> -					cpu, bad_irq[cpu]);
> +					cpu, irq_number[cpu]);
>  			pass = false;
>  		}
>  	}
> @@ -143,26 +146,18 @@ static void check_spurious(void)
>  	}
>  }
>  
> -static void check_ipi_sender(u32 irqstat, int sender)
> +static int gic_get_sender(int irqstat)
>  {
> -	if (gic_version() == 2) {
> -		int src = (irqstat >> 10) & 7;
> -
> -		if (src != sender)
> -			bad_sender[smp_processor_id()] = src;
> -	}
> -}
> -
> -static void check_irqnr(u32 irqnr)
> -{
> -	if (irqnr != IPI_IRQ)
> -		bad_irq[smp_processor_id()] = irqnr;
> +	if (gic_version() == 2)
> +		return (irqstat >> 10) & 7;
> +	return -1;
>  }
>  
>  static void ipi_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
> +	int this_cpu = smp_processor_id();
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		gic_write_eoir(irqstat);
> @@ -173,12 +168,12 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  		 */
>  		if (gic_version() == 2)
>  			smp_rmb();
> -		check_ipi_sender(irqstat, IPI_SENDER);
> -		check_irqnr(irqnr);
> +		irq_sender[this_cpu] = gic_get_sender(irqstat);
> +		irq_number[this_cpu] = irqnr;
>  		smp_wmb(); /* pairs with smp_rmb in check_acked */
> -		++acked[smp_processor_id()];
> +		++acked[this_cpu];
>  	} else {
> -		++spurious[smp_processor_id()];
> +		++spurious[this_cpu];
>  	}
>  
>  	/* Wait for writes to acked/spurious to complete */
> @@ -314,40 +309,42 @@ static void gicv3_ipi_send_broadcast(void)
>  
>  static void ipi_test_self(void)
>  {
> +	int this_cpu = smp_processor_id();
>  	cpumask_t mask;
>  
>  	report_prefix_push("self");
>  	stats_reset();
>  	cpumask_clear(&mask);
> -	cpumask_set_cpu(smp_processor_id(), &mask);
> +	cpumask_set_cpu(this_cpu, &mask);
>  	gic->ipi.send_self();
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
>  static void ipi_test_smp(void)
>  {
> +	int this_cpu = smp_processor_id();
>  	cpumask_t mask;
>  	int i;
>  
>  	report_prefix_push("target-list");
>  	stats_reset();
>  	cpumask_copy(&mask, &cpu_present_mask);
> -	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> +	for (i = this_cpu & 1; i < nr_cpus; i += 2)
>  		cpumask_clear_cpu(i, &mask);
>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  
>  	report_prefix_push("broadcast");
>  	stats_reset();
>  	cpumask_copy(&mask, &cpu_present_mask);
> -	cpumask_clear_cpu(smp_processor_id(), &mask);
> +	cpumask_clear_cpu(this_cpu, &mask);
>  	gic->ipi.send_broadcast();
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> @@ -396,6 +393,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
> +	int this_cpu = smp_processor_id();
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		void *base;
> @@ -408,11 +406,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -		check_ipi_sender(irqstat, smp_processor_id());
> -		check_irqnr(irqnr);
> -		++acked[smp_processor_id()];
> +		irq_sender[this_cpu] = gic_get_sender(irqstat);
> +		irq_number[this_cpu] = irqnr;
> +		++acked[this_cpu];
>  	} else {
> -		++spurious[smp_processor_id()];
> +		++spurious[this_cpu];
>  	}
>  }
>  
> 

Re: [kvm-unit-tests PATCH v2 09/12] arm/arm64: gic: Make check_acked() more generic

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> Testing that an interrupt is received as expected is done in three places:
> in check_ipi_sender(), check_irqnr() and check_acked(). check_irqnr()
> compares the interrupt ID with IPI_IRQ and records a failure in bad_irq,
> and check_ipi_sender() compares the sender with IPI_SENDER and writes to
> bad_sender when they don't match.
> 
> Let's move all the checks to check_acked() by renaming
> bad_sender->irq_sender and bad_irq->irq_number and changing their semantics
> so they record the interrupt sender, respectively the irq number.
> check_acked() now takes two new parameters: the expected interrupt number
> and sender.
> 
> This has two distinct advantages:
> 
> 1. check_acked() and ipi_handler() can now be used for interrupts other
>    than IPIs.
> 2. Correctness checks are consolidated in one function.
> 
> CC: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Eric Auger <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Look all like valid transformations to me, and indeed it's more reusable
now:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/gic.c | 68 +++++++++++++++++++++++++++----------------------------
>  1 file changed, 33 insertions(+), 35 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index a9ef1a5def56..fb91861900b7 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -35,7 +35,7 @@ struct gic {
>  
>  static struct gic *gic;
>  static int acked[NR_CPUS], spurious[NR_CPUS];
> -static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> +static int irq_sender[NR_CPUS], irq_number[NR_CPUS];
>  static cpumask_t ready;
>  
>  static void nr_cpu_check(int nr)
> @@ -57,8 +57,8 @@ static void stats_reset(void)
>  
>  	for (i = 0; i < nr_cpus; ++i) {
>  		acked[i] = 0;
> -		bad_sender[i] = -1;
> -		bad_irq[i] = -1;
> +		irq_sender[i] = -1;
> +		irq_number[i] = -1;
>  	}
>  }
>  
> @@ -92,9 +92,10 @@ static void wait_for_interrupts(cpumask_t *mask)
>  	report_info("interrupts timed-out (5s)");
>  }
>  
> -static bool check_acked(cpumask_t *mask)
> +static bool check_acked(cpumask_t *mask, int sender, int irqnum)
>  {
>  	int missing = 0, extra = 0, unexpected = 0;
> +	bool has_gicv2 = (gic_version() == 2);
>  	bool pass = true;
>  	int cpu;
>  
> @@ -108,17 +109,19 @@ static bool check_acked(cpumask_t *mask)
>  			if (acked[cpu])
>  				++unexpected;
>  		}
> +		if (!acked[cpu])
> +			continue;
>  		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
>  
> -		if (bad_sender[cpu] != -1) {
> +		if (has_gicv2 && irq_sender[cpu] != sender) {
>  			report_info("cpu%d received IPI from wrong sender %d",
> -					cpu, bad_sender[cpu]);
> +					cpu, irq_sender[cpu]);
>  			pass = false;
>  		}
>  
> -		if (bad_irq[cpu] != -1) {
> +		if (irq_number[cpu] != irqnum) {
>  			report_info("cpu%d received wrong irq %d",
> -					cpu, bad_irq[cpu]);
> +					cpu, irq_number[cpu]);
>  			pass = false;
>  		}
>  	}
> @@ -143,26 +146,18 @@ static void check_spurious(void)
>  	}
>  }
>  
> -static void check_ipi_sender(u32 irqstat, int sender)
> +static int gic_get_sender(int irqstat)
>  {
> -	if (gic_version() == 2) {
> -		int src = (irqstat >> 10) & 7;
> -
> -		if (src != sender)
> -			bad_sender[smp_processor_id()] = src;
> -	}
> -}
> -
> -static void check_irqnr(u32 irqnr)
> -{
> -	if (irqnr != IPI_IRQ)
> -		bad_irq[smp_processor_id()] = irqnr;
> +	if (gic_version() == 2)
> +		return (irqstat >> 10) & 7;
> +	return -1;
>  }
>  
>  static void ipi_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
> +	int this_cpu = smp_processor_id();
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		gic_write_eoir(irqstat);
> @@ -173,12 +168,12 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  		 */
>  		if (gic_version() == 2)
>  			smp_rmb();
> -		check_ipi_sender(irqstat, IPI_SENDER);
> -		check_irqnr(irqnr);
> +		irq_sender[this_cpu] = gic_get_sender(irqstat);
> +		irq_number[this_cpu] = irqnr;
>  		smp_wmb(); /* pairs with smp_rmb in check_acked */
> -		++acked[smp_processor_id()];
> +		++acked[this_cpu];
>  	} else {
> -		++spurious[smp_processor_id()];
> +		++spurious[this_cpu];
>  	}
>  
>  	/* Wait for writes to acked/spurious to complete */
> @@ -314,40 +309,42 @@ static void gicv3_ipi_send_broadcast(void)
>  
>  static void ipi_test_self(void)
>  {
> +	int this_cpu = smp_processor_id();
>  	cpumask_t mask;
>  
>  	report_prefix_push("self");
>  	stats_reset();
>  	cpumask_clear(&mask);
> -	cpumask_set_cpu(smp_processor_id(), &mask);
> +	cpumask_set_cpu(this_cpu, &mask);
>  	gic->ipi.send_self();
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
>  static void ipi_test_smp(void)
>  {
> +	int this_cpu = smp_processor_id();
>  	cpumask_t mask;
>  	int i;
>  
>  	report_prefix_push("target-list");
>  	stats_reset();
>  	cpumask_copy(&mask, &cpu_present_mask);
> -	for (i = smp_processor_id() & 1; i < nr_cpus; i += 2)
> +	for (i = this_cpu & 1; i < nr_cpus; i += 2)
>  		cpumask_clear_cpu(i, &mask);
>  	gic_ipi_send_mask(IPI_IRQ, &mask);
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  
>  	report_prefix_push("broadcast");
>  	stats_reset();
>  	cpumask_copy(&mask, &cpu_present_mask);
> -	cpumask_clear_cpu(smp_processor_id(), &mask);
> +	cpumask_clear_cpu(this_cpu, &mask);
>  	gic->ipi.send_broadcast();
>  	wait_for_interrupts(&mask);
> -	report(check_acked(&mask), "Interrupts received");
> +	report(check_acked(&mask, this_cpu, IPI_IRQ), "Interrupts received");
>  	report_prefix_pop();
>  }
>  
> @@ -396,6 +393,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  {
>  	u32 irqstat = gic_read_iar();
>  	u32 irqnr = gic_iar_irqnr(irqstat);
> +	int this_cpu = smp_processor_id();
>  
>  	if (irqnr != GICC_INT_SPURIOUS) {
>  		void *base;
> @@ -408,11 +406,11 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -		check_ipi_sender(irqstat, smp_processor_id());
> -		check_irqnr(irqnr);
> -		++acked[smp_processor_id()];
> +		irq_sender[this_cpu] = gic_get_sender(irqstat);
> +		irq_number[this_cpu] = irqnr;
> +		++acked[this_cpu];
>  	} else {
> -		++spurious[smp_processor_id()];
> +		++spurious[this_cpu];
>  	}
>  }
>  
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[Alexandru Elisei]

The its-trigger test checks that LPI 8195 is not delivered to the CPU while
it is disabled at the ITS level. After that it is re-enabled and the test
checks that the interrupt is properly asserted. After it's re-enabled and
before the stats are examined, the test triggers the interrupt again, which
can lead to the same interrupt being delivered twice: once after the
configuration invalidation and before the INT command, and once after the
INT command.

Get rid of the INT command after the interrupt is re-enabled to prevent the
LPI from being asserted twice and add a separate check to test that the INT
command still works for the now re-enabled LPI 8195.

CC: Auger Eric <eric.auger@redhat.com>
Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index fb91861900b7..aa3aa1763984 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -805,6 +805,9 @@ static void test_its_trigger(void)
 
 	/* Now call the invall and check the LPI hits */
 	its_send_invall(col3);
+	lpi_stats_expect(3, 8195);
+	check_lpi_stats("dev2/eventid=20 pending LPI is received");
+
 	lpi_stats_expect(3, 8195);
 	its_send_int(dev2, 20);
 	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
-- 
2.29.2

[kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[Alexandru Elisei]

The its-trigger test checks that LPI 8195 is not delivered to the CPU while
it is disabled at the ITS level. After that it is re-enabled and the test
checks that the interrupt is properly asserted. After it's re-enabled and
before the stats are examined, the test triggers the interrupt again, which
can lead to the same interrupt being delivered twice: once after the
configuration invalidation and before the INT command, and once after the
INT command.

Get rid of the INT command after the interrupt is re-enabled to prevent the
LPI from being asserted twice and add a separate check to test that the INT
command still works for the now re-enabled LPI 8195.

CC: Auger Eric <eric.auger@redhat.com>
Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arm/gic.c b/arm/gic.c
index fb91861900b7..aa3aa1763984 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -805,6 +805,9 @@ static void test_its_trigger(void)
 
 	/* Now call the invall and check the LPI hits */
 	its_send_invall(col3);
+	lpi_stats_expect(3, 8195);
+	check_lpi_stats("dev2/eventid=20 pending LPI is received");
+
 	lpi_stats_expect(3, 8195);
 	its_send_int(dev2, 20);
 	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The its-trigger test checks that LPI 8195 is not delivered to the CPU while
> it is disabled at the ITS level. After that it is re-enabled and the test
> checks that the interrupt is properly asserted. After it's re-enabled and
> before the stats are examined, the test triggers the interrupt again, which
> can lead to the same interrupt being delivered twice: once after the
> configuration invalidation and before the INT command, and once after the
> INT command.
> 
> Get rid of the INT command after the interrupt is re-enabled to prevent the

This is confusing to read, since you don't remove anything in the patch.
Can you reword this? Something like "Before explicitly triggering the
interrupt, check that the unmasking worked, ..."

> LPI from being asserted twice and add a separate check to test that the INT
> command still works for the now re-enabled LPI 8195.
> 
> CC: Auger Eric <eric.auger@redhat.com>
> Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Otherwise this looks fine, but I think there is another flaw: There is
no requirement that an INV(ALL) is *needed* to update the status, it
could also update anytime (think: "cache invalidate").

The KVM ITS emulation *only* bothers to read the memory on an INV(ALL)
command, so that matches the test. But that's not how unit-tests should
work ;-)

But that's a separate issue, just mentioning this to not forget about it.

For this patch, with the message fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/gic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index fb91861900b7..aa3aa1763984 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -805,6 +805,9 @@ static void test_its_trigger(void)
>  
>  	/* Now call the invall and check the LPI hits */
>  	its_send_invall(col3);
> +	lpi_stats_expect(3, 8195);
> +	check_lpi_stats("dev2/eventid=20 pending LPI is received");
> +
>  	lpi_stats_expect(3, 8195);
>  	its_send_int(dev2, 20);
>  	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
> 

Re: [kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The its-trigger test checks that LPI 8195 is not delivered to the CPU while
> it is disabled at the ITS level. After that it is re-enabled and the test
> checks that the interrupt is properly asserted. After it's re-enabled and
> before the stats are examined, the test triggers the interrupt again, which
> can lead to the same interrupt being delivered twice: once after the
> configuration invalidation and before the INT command, and once after the
> INT command.
> 
> Get rid of the INT command after the interrupt is re-enabled to prevent the

This is confusing to read, since you don't remove anything in the patch.
Can you reword this? Something like "Before explicitly triggering the
interrupt, check that the unmasking worked, ..."

> LPI from being asserted twice and add a separate check to test that the INT
> command still works for the now re-enabled LPI 8195.
> 
> CC: Auger Eric <eric.auger@redhat.com>
> Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>

Otherwise this looks fine, but I think there is another flaw: There is
no requirement that an INV(ALL) is *needed* to update the status, it
could also update anytime (think: "cache invalidate").

The KVM ITS emulation *only* bothers to read the memory on an INV(ALL)
command, so that matches the test. But that's not how unit-tests should
work ;-)

But that's a separate issue, just mentioning this to not forget about it.

For this patch, with the message fixed:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre

> ---
>  arm/gic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index fb91861900b7..aa3aa1763984 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -805,6 +805,9 @@ static void test_its_trigger(void)
>  
>  	/* Now call the invall and check the LPI hits */
>  	its_send_invall(col3);
> +	lpi_stats_expect(3, 8195);
> +	check_lpi_stats("dev2/eventid=20 pending LPI is received");
> +
>  	lpi_stats_expect(3, 8195);
>  	its_send_int(dev2, 20);
>  	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[Alexandru Elisei]

Hi Andre,

On 12/18/20 6:15 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> The its-trigger test checks that LPI 8195 is not delivered to the CPU while
>> it is disabled at the ITS level. After that it is re-enabled and the test
>> checks that the interrupt is properly asserted. After it's re-enabled and
>> before the stats are examined, the test triggers the interrupt again, which
>> can lead to the same interrupt being delivered twice: once after the
>> configuration invalidation and before the INT command, and once after the
>> INT command.
>>
>> Get rid of the INT command after the interrupt is re-enabled to prevent the
> This is confusing to read, since you don't remove anything in the patch.
> Can you reword this? Something like "Before explicitly triggering the
> interrupt, check that the unmasking worked, ..."

That's a good point, I'll reword it.

>
>> LPI from being asserted twice and add a separate check to test that the INT
>> command still works for the now re-enabled LPI 8195.
>>
>> CC: Auger Eric <eric.auger@redhat.com>
>> Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Otherwise this looks fine, but I think there is another flaw: There is
> no requirement that an INV(ALL) is *needed* to update the status, it
> could also update anytime (think: "cache invalidate").
>
> The KVM ITS emulation *only* bothers to read the memory on an INV(ALL)
> command, so that matches the test. But that's not how unit-tests should
> work ;-)
>
> But that's a separate issue, just mentioning this to not forget about it.

That's a good point, I must admit that I didn't check how caching is defined by
the architecture. I would prefer creating a patch independent of this series to
change what test_its_trigger() checks for, to get input from Eric just for that
particular change.

Thanks,
Alex
>
> For this patch, with the message fixed:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
>> ---
>>  arm/gic.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index fb91861900b7..aa3aa1763984 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -805,6 +805,9 @@ static void test_its_trigger(void)
>>  
>>  	/* Now call the invall and check the LPI hits */
>>  	its_send_invall(col3);
>> +	lpi_stats_expect(3, 8195);
>> +	check_lpi_stats("dev2/eventid=20 pending LPI is received");
>> +
>>  	lpi_stats_expect(3, 8195);
>>  	its_send_int(dev2, 20);
>>  	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
>>

Re: [kvm-unit-tests PATCH v2 10/12] arm64: gic: its-trigger: Don't trigger the LPI while it is pending

[Alexandru Elisei]

Hi Andre,

On 12/18/20 6:15 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> The its-trigger test checks that LPI 8195 is not delivered to the CPU while
>> it is disabled at the ITS level. After that it is re-enabled and the test
>> checks that the interrupt is properly asserted. After it's re-enabled and
>> before the stats are examined, the test triggers the interrupt again, which
>> can lead to the same interrupt being delivered twice: once after the
>> configuration invalidation and before the INT command, and once after the
>> INT command.
>>
>> Get rid of the INT command after the interrupt is re-enabled to prevent the
> This is confusing to read, since you don't remove anything in the patch.
> Can you reword this? Something like "Before explicitly triggering the
> interrupt, check that the unmasking worked, ..."

That's a good point, I'll reword it.

>
>> LPI from being asserted twice and add a separate check to test that the INT
>> command still works for the now re-enabled LPI 8195.
>>
>> CC: Auger Eric <eric.auger@redhat.com>
>> Suggested-by: Zenghui Yu <yuzenghui@huawei.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> Otherwise this looks fine, but I think there is another flaw: There is
> no requirement that an INV(ALL) is *needed* to update the status, it
> could also update anytime (think: "cache invalidate").
>
> The KVM ITS emulation *only* bothers to read the memory on an INV(ALL)
> command, so that matches the test. But that's not how unit-tests should
> work ;-)
>
> But that's a separate issue, just mentioning this to not forget about it.

That's a good point, I must admit that I didn't check how caching is defined by
the architecture. I would prefer creating a patch independent of this series to
change what test_its_trigger() checks for, to get input from Eric just for that
particular change.

Thanks,
Alex
>
> For this patch, with the message fixed:
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>
> Cheers,
> Andre
>
>> ---
>>  arm/gic.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/arm/gic.c b/arm/gic.c
>> index fb91861900b7..aa3aa1763984 100644
>> --- a/arm/gic.c
>> +++ b/arm/gic.c
>> @@ -805,6 +805,9 @@ static void test_its_trigger(void)
>>  
>>  	/* Now call the invall and check the LPI hits */
>>  	its_send_invall(col3);
>> +	lpi_stats_expect(3, 8195);
>> +	check_lpi_stats("dev2/eventid=20 pending LPI is received");
>> +
>>  	lpi_stats_expect(3, 8195);
>>  	its_send_int(dev2, 20);
>>  	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
>>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[Alexandru Elisei]

The ITS tests use the INT command like an SGI. The its_send_int() function
kicks a CPU and then the test checks that the interrupt was observed as
expected in check_lpi_stats(). This is done by using lpi_stats.observed and
lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
and the source CPU reads it and compares the values with
lpi_stats.expected.

The fact that the target CPU doesn't read data written by the source CPU
means that we don't need to do inter-processor memory synchronization
for that between the two at the moment.

The acked array is used by its-pending-migration test, but the reset value
for acked (zero) is the same as the initialization value for static
variables, so memory synchronization is again not needed.

However, that is all about to change when we modify all ITS tests to use
the same functions as the IPI tests. Add a write memory barrier to
its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
semantics.

Suggested-by: Auger Eric <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm64/gic-v3-its-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
index 34574f71d171..32703147ee85 100644
--- a/lib/arm64/gic-v3-its-cmd.c
+++ b/lib/arm64/gic-v3-its-cmd.c
@@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
 {
 	struct its_cmd_desc desc;
 
+	/*
+	 * The INT command is used by tests as an IPI. Ensure stores to Normal
+	 * memory are visible to other CPUs before sending the LPI.
+	 */
+	wmb();
+
 	desc.its_int_cmd.dev = dev;
 	desc.its_int_cmd.event_id = event_id;
 	desc.verbose = verbose;
-- 
2.29.2

[kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[Alexandru Elisei]

The ITS tests use the INT command like an SGI. The its_send_int() function
kicks a CPU and then the test checks that the interrupt was observed as
expected in check_lpi_stats(). This is done by using lpi_stats.observed and
lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
and the source CPU reads it and compares the values with
lpi_stats.expected.

The fact that the target CPU doesn't read data written by the source CPU
means that we don't need to do inter-processor memory synchronization
for that between the two at the moment.

The acked array is used by its-pending-migration test, but the reset value
for acked (zero) is the same as the initialization value for static
variables, so memory synchronization is again not needed.

However, that is all about to change when we modify all ITS tests to use
the same functions as the IPI tests. Add a write memory barrier to
its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
semantics.

Suggested-by: Auger Eric <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 lib/arm64/gic-v3-its-cmd.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
index 34574f71d171..32703147ee85 100644
--- a/lib/arm64/gic-v3-its-cmd.c
+++ b/lib/arm64/gic-v3-its-cmd.c
@@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
 {
 	struct its_cmd_desc desc;
 
+	/*
+	 * The INT command is used by tests as an IPI. Ensure stores to Normal
+	 * memory are visible to other CPUs before sending the LPI.
+	 */
+	wmb();
+
 	desc.its_int_cmd.dev = dev;
 	desc.its_int_cmd.event_id = event_id;
 	desc.verbose = verbose;
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The ITS tests use the INT command like an SGI. The its_send_int() function
> kicks a CPU and then the test checks that the interrupt was observed as
> expected in check_lpi_stats(). This is done by using lpi_stats.observed and
> lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
> and the source CPU reads it and compares the values with
> lpi_stats.expected.
> 
> The fact that the target CPU doesn't read data written by the source CPU
> means that we don't need to do inter-processor memory synchronization
> for that between the two at the moment.
> 
> The acked array is used by its-pending-migration test, but the reset value
> for acked (zero) is the same as the initialization value for static
> variables, so memory synchronization is again not needed.
> 
> However, that is all about to change when we modify all ITS tests to use
> the same functions as the IPI tests. Add a write memory barrier to
> its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
> semantics.

I agree to the requirement for having the barrier, but am not sure this
is the right place. Wouldn't it be better to have the barrier in the
callers?

Besides: This command is written to the command queue (in normal
memory), then we notify the ITS via an MMIO writeq. And this one has a
"wmb" barrier already (though for other reasons).

Cheers,
Andre


> 
> Suggested-by: Auger Eric <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm64/gic-v3-its-cmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
> index 34574f71d171..32703147ee85 100644
> --- a/lib/arm64/gic-v3-its-cmd.c
> +++ b/lib/arm64/gic-v3-its-cmd.c
> @@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
>  {
>  	struct its_cmd_desc desc;
>  
> +	/*
> +	 * The INT command is used by tests as an IPI. Ensure stores to Normal
> +	 * memory are visible to other CPUs before sending the LPI.
> +	 */
> +	wmb();
> +
>  	desc.its_int_cmd.dev = dev;
>  	desc.its_int_cmd.event_id = event_id;
>  	desc.verbose = verbose;
> 

Re: [kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[André Przywara]

On 17/12/2020 14:13, Alexandru Elisei wrote:
> The ITS tests use the INT command like an SGI. The its_send_int() function
> kicks a CPU and then the test checks that the interrupt was observed as
> expected in check_lpi_stats(). This is done by using lpi_stats.observed and
> lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
> and the source CPU reads it and compares the values with
> lpi_stats.expected.
> 
> The fact that the target CPU doesn't read data written by the source CPU
> means that we don't need to do inter-processor memory synchronization
> for that between the two at the moment.
> 
> The acked array is used by its-pending-migration test, but the reset value
> for acked (zero) is the same as the initialization value for static
> variables, so memory synchronization is again not needed.
> 
> However, that is all about to change when we modify all ITS tests to use
> the same functions as the IPI tests. Add a write memory barrier to
> its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
> semantics.

I agree to the requirement for having the barrier, but am not sure this
is the right place. Wouldn't it be better to have the barrier in the
callers?

Besides: This command is written to the command queue (in normal
memory), then we notify the ITS via an MMIO writeq. And this one has a
"wmb" barrier already (though for other reasons).

Cheers,
Andre


> 
> Suggested-by: Auger Eric <eric.auger@redhat.com>
> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
> ---
>  lib/arm64/gic-v3-its-cmd.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
> index 34574f71d171..32703147ee85 100644
> --- a/lib/arm64/gic-v3-its-cmd.c
> +++ b/lib/arm64/gic-v3-its-cmd.c
> @@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
>  {
>  	struct its_cmd_desc desc;
>  
> +	/*
> +	 * The INT command is used by tests as an IPI. Ensure stores to Normal
> +	 * memory are visible to other CPUs before sending the LPI.
> +	 */
> +	wmb();
> +
>  	desc.its_int_cmd.dev = dev;
>  	desc.its_int_cmd.event_id = event_id;
>  	desc.verbose = verbose;
> 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[Alexandru Elisei]

Hi Andre,

On 12/18/20 6:36 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> The ITS tests use the INT command like an SGI. The its_send_int() function
>> kicks a CPU and then the test checks that the interrupt was observed as
>> expected in check_lpi_stats(). This is done by using lpi_stats.observed and
>> lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
>> and the source CPU reads it and compares the values with
>> lpi_stats.expected.
>>
>> The fact that the target CPU doesn't read data written by the source CPU
>> means that we don't need to do inter-processor memory synchronization
>> for that between the two at the moment.
>>
>> The acked array is used by its-pending-migration test, but the reset value
>> for acked (zero) is the same as the initialization value for static
>> variables, so memory synchronization is again not needed.
>>
>> However, that is all about to change when we modify all ITS tests to use
>> the same functions as the IPI tests. Add a write memory barrier to
>> its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
>> semantics.
> I agree to the requirement for having the barrier, but am not sure this
> is the right place. Wouldn't it be better to have the barrier in the
> callers?
>
> Besides: This command is written to the command queue (in normal
> memory), then we notify the ITS via an MMIO writeq. And this one has a
> "wmb" barrier already (though for other reasons).

Had another look, and you're totally right: its_post_commands() already has a
wmb() in writeq(), thank you for pointing it out. This makes the wmb() which I've
added pointless, I'll drop the patch since it doesn't add useful.

Thanks,
Alex
>
> Cheers,
> Andre
>
>
>> Suggested-by: Auger Eric <eric.auger@redhat.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  lib/arm64/gic-v3-its-cmd.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
>> index 34574f71d171..32703147ee85 100644
>> --- a/lib/arm64/gic-v3-its-cmd.c
>> +++ b/lib/arm64/gic-v3-its-cmd.c
>> @@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
>>  {
>>  	struct its_cmd_desc desc;
>>  
>> +	/*
>> +	 * The INT command is used by tests as an IPI. Ensure stores to Normal
>> +	 * memory are visible to other CPUs before sending the LPI.
>> +	 */
>> +	wmb();
>> +
>>  	desc.its_int_cmd.dev = dev;
>>  	desc.its_int_cmd.event_id = event_id;
>>  	desc.verbose = verbose;
>>

Re: [kvm-unit-tests PATCH v2 11/12] lib: arm64: gic-v3-its: Add wmb() barrier before INT command

[Alexandru Elisei]

Hi Andre,

On 12/18/20 6:36 PM, André Przywara wrote:
> On 17/12/2020 14:13, Alexandru Elisei wrote:
>> The ITS tests use the INT command like an SGI. The its_send_int() function
>> kicks a CPU and then the test checks that the interrupt was observed as
>> expected in check_lpi_stats(). This is done by using lpi_stats.observed and
>> lpi_stats.expected, where the target CPU only writes to lpi_stats.observed,
>> and the source CPU reads it and compares the values with
>> lpi_stats.expected.
>>
>> The fact that the target CPU doesn't read data written by the source CPU
>> means that we don't need to do inter-processor memory synchronization
>> for that between the two at the moment.
>>
>> The acked array is used by its-pending-migration test, but the reset value
>> for acked (zero) is the same as the initialization value for static
>> variables, so memory synchronization is again not needed.
>>
>> However, that is all about to change when we modify all ITS tests to use
>> the same functions as the IPI tests. Add a write memory barrier to
>> its_send_int(), similar to the gicv3_ipi_send_mask(), which has similar
>> semantics.
> I agree to the requirement for having the barrier, but am not sure this
> is the right place. Wouldn't it be better to have the barrier in the
> callers?
>
> Besides: This command is written to the command queue (in normal
> memory), then we notify the ITS via an MMIO writeq. And this one has a
> "wmb" barrier already (though for other reasons).

Had another look, and you're totally right: its_post_commands() already has a
wmb() in writeq(), thank you for pointing it out. This makes the wmb() which I've
added pointless, I'll drop the patch since it doesn't add useful.

Thanks,
Alex
>
> Cheers,
> Andre
>
>
>> Suggested-by: Auger Eric <eric.auger@redhat.com>
>> Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
>> ---
>>  lib/arm64/gic-v3-its-cmd.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/lib/arm64/gic-v3-its-cmd.c b/lib/arm64/gic-v3-its-cmd.c
>> index 34574f71d171..32703147ee85 100644
>> --- a/lib/arm64/gic-v3-its-cmd.c
>> +++ b/lib/arm64/gic-v3-its-cmd.c
>> @@ -385,6 +385,12 @@ void __its_send_int(struct its_device *dev, u32 event_id, bool verbose)
>>  {
>>  	struct its_cmd_desc desc;
>>  
>> +	/*
>> +	 * The INT command is used by tests as an IPI. Ensure stores to Normal
>> +	 * memory are visible to other CPUs before sending the LPI.
>> +	 */
>> +	wmb();
>> +
>>  	desc.its_int_cmd.dev = dev;
>>  	desc.its_int_cmd.event_id = event_id;
>>  	desc.verbose = verbose;
>>
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH v2 12/12] arm64: gic: Use IPI test checking for the LPI tests

[Alexandru Elisei]

The LPI code validates a result similarly to the IPI tests, by checking if
the target CPU received the interrupt with the expected interrupt number.
However, the LPI tests invent their own way of checking the test results by
creating a global struct (lpi_stats), using a separate interrupt handler
(lpi_handler) and test function (check_lpi_stats).

There are several areas that can be improved in the LPI code, which are
already covered by the IPI tests:

- check_lpi_stats() doesn't take into account that the target CPU can
  receive the correct interrupt multiple times.
- check_lpi_stats() doesn't take into the account the scenarios where all
  online CPUs can receive the interrupt, but the target CPU is the last CPU
  that touches lpi_stats.observed.
- Insufficient or missing memory synchronization.

Instead of duplicating code, let's convert the LPI tests to use
check_acked() and the same interrupt handler as the IPI tests, which has
been renamed to irq_handler() to avoid any confusion.

check_lpi_stats() has been replaced with check_acked() which, together with
using irq_handler(), instantly gives us more correctness checks and proper
memory synchronization between threads. lpi_stats.expected has been
replaced by the CPU mask and the expected interrupt number arguments to
check_acked(), with no change in semantics.

lpi_handler() aborted the test if the interrupt number was not an LPI. This
was changed in favor of allowing the test to continue, as it will fail in
check_acked(), but possibly print information useful for debugging. If the
test receives spurious interrupts, those are reported via report_info() at
the end of the test for consistency with the IPI tests, which don't treat
spurious interrupts as critical errors.

In the spirit of code reuse, secondary_lpi_tests() has been replaced with
ipi_recv() because the two are now identical; ipi_recv() has been renamed
to irq_recv(), similarly to irq_handler(), to avoid confusion.

CC: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 190 +++++++++++++++++++++++++-----------------------------
 1 file changed, 87 insertions(+), 103 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index aa3aa1763984..53944753db63 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -105,13 +105,12 @@ static bool check_acked(cpumask_t *mask, int sender, int irqnum)
 				++missing;
 			else if (acked[cpu] > 1)
 				++extra;
-		} else {
-			if (acked[cpu])
+		} else if (acked[cpu]) {
 				++unexpected;
 		}
 		if (!acked[cpu])
 			continue;
-		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
+		smp_rmb(); /* pairs with smp_wmb in irq_handler */
 
 		if (has_gicv2 && irq_sender[cpu] != sender) {
 			report_info("cpu%d received IPI from wrong sender %d",
@@ -149,11 +148,12 @@ static void check_spurious(void)
 static int gic_get_sender(int irqstat)
 {
 	if (gic_version() == 2)
+		/* GICC_IAR.CPUID is RAZ for non-SGIs */
 		return (irqstat >> 10) & 7;
 	return -1;
 }
 
-static void ipi_handler(struct pt_regs *regs __unused)
+static void irq_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
@@ -192,75 +192,6 @@ static void setup_irq(irq_handler_fn handler)
 }
 
 #if defined(__aarch64__)
-struct its_event {
-	int cpu_id;
-	int lpi_id;
-};
-
-struct its_stats {
-	struct its_event expected;
-	struct its_event observed;
-};
-
-static struct its_stats lpi_stats;
-
-static void lpi_handler(struct pt_regs *regs __unused)
-{
-	u32 irqstat = gic_read_iar();
-	int irqnr = gic_iar_irqnr(irqstat);
-
-	gic_write_eoir(irqstat);
-	assert(irqnr >= 8192);
-	smp_rmb(); /* pairs with wmb in lpi_stats_expect */
-	lpi_stats.observed.cpu_id = smp_processor_id();
-	lpi_stats.observed.lpi_id = irqnr;
-	acked[lpi_stats.observed.cpu_id]++;
-	smp_wmb(); /* pairs with rmb in check_lpi_stats */
-}
-
-static void lpi_stats_expect(int exp_cpu_id, int exp_lpi_id)
-{
-	lpi_stats.expected.cpu_id = exp_cpu_id;
-	lpi_stats.expected.lpi_id = exp_lpi_id;
-	lpi_stats.observed.cpu_id = -1;
-	lpi_stats.observed.lpi_id = -1;
-	smp_wmb(); /* pairs with rmb in handler */
-}
-
-static void check_lpi_stats(const char *msg)
-{
-	int i;
-
-	for (i = 0; i < 50; i++) {
-		mdelay(100);
-		smp_rmb(); /* pairs with wmb in lpi_handler */
-		if (lpi_stats.observed.cpu_id == lpi_stats.expected.cpu_id &&
-		    lpi_stats.observed.lpi_id == lpi_stats.expected.lpi_id) {
-			report(true, "%s", msg);
-			return;
-		}
-	}
-
-	if (lpi_stats.observed.cpu_id == -1 && lpi_stats.observed.lpi_id == -1) {
-		report_info("No LPI received whereas (cpuid=%d, intid=%d) "
-			    "was expected", lpi_stats.expected.cpu_id,
-			    lpi_stats.expected.lpi_id);
-	} else {
-		report_info("Unexpected LPI (cpuid=%d, intid=%d)",
-			    lpi_stats.observed.cpu_id,
-			    lpi_stats.observed.lpi_id);
-	}
-	report(false, "%s", msg);
-}
-
-static void secondary_lpi_test(void)
-{
-	setup_irq(lpi_handler);
-	cpumask_set_cpu(smp_processor_id(), &ready);
-	while (1)
-		wfi();
-}
-
 static void check_lpi_hits(int *expected, const char *msg)
 {
 	bool pass = true;
@@ -350,7 +281,7 @@ static void ipi_test_smp(void)
 
 static void ipi_send(void)
 {
-	setup_irq(ipi_handler);
+	setup_irq(irq_handler);
 	wait_on_ready();
 	ipi_test_self();
 	ipi_test_smp();
@@ -358,9 +289,9 @@ static void ipi_send(void)
 	exit(report_summary());
 }
 
-static void ipi_recv(void)
+static void irq_recv(void)
 {
-	setup_irq(ipi_handler);
+	setup_irq(irq_handler);
 	cpumask_set_cpu(smp_processor_id(), &ready);
 	while (1)
 		wfi();
@@ -371,7 +302,7 @@ static void ipi_test(void *data __unused)
 	if (smp_processor_id() == IPI_SENDER)
 		ipi_send();
 	else
-		ipi_recv();
+		irq_recv();
 }
 
 static struct gic gicv2 = {
@@ -699,14 +630,12 @@ static int its_prerequisites(int nb_cpus)
 		return -1;
 	}
 
-	stats_reset();
-
-	setup_irq(lpi_handler);
+	setup_irq(irq_handler);
 
 	for_each_present_cpu(cpu) {
 		if (cpu == 0)
 			continue;
-		smp_boot_secondary(cpu, secondary_lpi_test);
+		smp_boot_secondary(cpu, irq_recv);
 	}
 	wait_on_ready();
 
@@ -760,6 +689,7 @@ static void test_its_trigger(void)
 {
 	struct its_collection *col3;
 	struct its_device *dev2, *dev7;
+	cpumask_t mask;
 
 	if (its_setup1())
 		return;
@@ -770,13 +700,21 @@ static void test_its_trigger(void)
 
 	report_prefix_push("int");
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev=2, eventid=20  -> lpi= 8195, col=3");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev=2, eventid=20  -> lpi= 8195, col=3");
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev=7, eventid=255 -> lpi= 8196, col=2");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev=7, eventid=255 -> lpi= 8196, col=2");
 
 	report_prefix_pop();
 
@@ -789,9 +727,12 @@ static void test_its_trigger(void)
 	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT & ~LPI_PROP_ENABLED);
 	its_send_inv(dev2, 20);
 
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 does not trigger any LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1),
+			"dev2/eventid=20 does not trigger any LPI");
 
 	/*
 	 * re-enable the LPI but willingly do not call invall
@@ -799,18 +740,31 @@ static void test_its_trigger(void)
 	 * The LPI should not hit
 	 */
 	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1),
+			"dev2/eventid=20 still does not trigger any LPI");
 
 	/* Now call the invall and check the LPI hits */
+	stats_reset();
+	/* The barrier is from its_send_int() */
+	wmb();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_invall(col3);
-	lpi_stats_expect(3, 8195);
-	check_lpi_stats("dev2/eventid=20 pending LPI is received");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 pending LPI is received");
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 now triggers an LPI");
 
 	report_prefix_pop();
 
@@ -821,9 +775,13 @@ static void test_its_trigger(void)
 	 */
 
 	its_send_mapd(dev2, false);
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("no LPI after device unmap");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1), "no LPI after device unmap");
+
+	check_spurious();
 	report_prefix_pop();
 }
 
@@ -831,6 +789,7 @@ static void test_its_migration(void)
 {
 	struct its_device *dev2, *dev7;
 	bool test_skipped = false;
+	cpumask_t mask;
 
 	if (its_setup1()) {
 		test_skipped = true;
@@ -847,13 +806,23 @@ do_migrate:
 	if (test_skipped)
 		return;
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
+
+	check_spurious();
 }
 
 #define ERRATA_UNMAPPED_COLLECTIONS "ERRATA_8c58be34494b"
@@ -863,6 +832,7 @@ static void test_migrate_unmapped_collection(void)
 	struct its_collection *col = NULL;
 	struct its_device *dev2 = NULL, *dev7 = NULL;
 	bool test_skipped = false;
+	cpumask_t mask;
 	int pe0 = 0;
 	u8 config;
 
@@ -897,17 +867,27 @@ do_migrate:
 	its_send_mapc(col, true);
 	its_send_invall(col);
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev7/eventid= 255 triggered LPI 8196 on PE #2");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev7/eventid= 255 triggered LPI 8196 on PE #2");
 
 	config = gicv3_lpi_get_config(8192);
 	report(config == LPI_PROP_DEFAULT,
 	       "Config of LPI 8192 was properly migrated");
 
-	lpi_stats_expect(pe0, 8192);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(pe0, &mask);
 	its_send_int(dev2, 0);
-	check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8192),
+			"dev2/eventid = 0 triggered LPI 8192 on PE0");
+
+	check_spurious();
 }
 
 static void test_its_pending_migration(void)
@@ -964,6 +944,10 @@ static void test_its_pending_migration(void)
 	pendbaser = readq(ptr);
 	writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
 
+	/*
+	 * Reset and initialization values for acked are the same, so we don't
+	 * need to explicitely call stats_reset().
+	 */
 	gicv3_lpi_rdist_enable(pe0);
 	gicv3_lpi_rdist_enable(pe1);
 
-- 
2.29.2

[kvm-unit-tests PATCH v2 12/12] arm64: gic: Use IPI test checking for the LPI tests

[Alexandru Elisei]

The LPI code validates a result similarly to the IPI tests, by checking if
the target CPU received the interrupt with the expected interrupt number.
However, the LPI tests invent their own way of checking the test results by
creating a global struct (lpi_stats), using a separate interrupt handler
(lpi_handler) and test function (check_lpi_stats).

There are several areas that can be improved in the LPI code, which are
already covered by the IPI tests:

- check_lpi_stats() doesn't take into account that the target CPU can
  receive the correct interrupt multiple times.
- check_lpi_stats() doesn't take into the account the scenarios where all
  online CPUs can receive the interrupt, but the target CPU is the last CPU
  that touches lpi_stats.observed.
- Insufficient or missing memory synchronization.

Instead of duplicating code, let's convert the LPI tests to use
check_acked() and the same interrupt handler as the IPI tests, which has
been renamed to irq_handler() to avoid any confusion.

check_lpi_stats() has been replaced with check_acked() which, together with
using irq_handler(), instantly gives us more correctness checks and proper
memory synchronization between threads. lpi_stats.expected has been
replaced by the CPU mask and the expected interrupt number arguments to
check_acked(), with no change in semantics.

lpi_handler() aborted the test if the interrupt number was not an LPI. This
was changed in favor of allowing the test to continue, as it will fail in
check_acked(), but possibly print information useful for debugging. If the
test receives spurious interrupts, those are reported via report_info() at
the end of the test for consistency with the IPI tests, which don't treat
spurious interrupts as critical errors.

In the spirit of code reuse, secondary_lpi_tests() has been replaced with
ipi_recv() because the two are now identical; ipi_recv() has been renamed
to irq_recv(), similarly to irq_handler(), to avoid confusion.

CC: Eric Auger <eric.auger@redhat.com>
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
 arm/gic.c | 190 +++++++++++++++++++++++++-----------------------------
 1 file changed, 87 insertions(+), 103 deletions(-)

diff --git a/arm/gic.c b/arm/gic.c
index aa3aa1763984..53944753db63 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -105,13 +105,12 @@ static bool check_acked(cpumask_t *mask, int sender, int irqnum)
 				++missing;
 			else if (acked[cpu] > 1)
 				++extra;
-		} else {
-			if (acked[cpu])
+		} else if (acked[cpu]) {
 				++unexpected;
 		}
 		if (!acked[cpu])
 			continue;
-		smp_rmb(); /* pairs with smp_wmb in ipi_handler */
+		smp_rmb(); /* pairs with smp_wmb in irq_handler */
 
 		if (has_gicv2 && irq_sender[cpu] != sender) {
 			report_info("cpu%d received IPI from wrong sender %d",
@@ -149,11 +148,12 @@ static void check_spurious(void)
 static int gic_get_sender(int irqstat)
 {
 	if (gic_version() == 2)
+		/* GICC_IAR.CPUID is RAZ for non-SGIs */
 		return (irqstat >> 10) & 7;
 	return -1;
 }
 
-static void ipi_handler(struct pt_regs *regs __unused)
+static void irq_handler(struct pt_regs *regs __unused)
 {
 	u32 irqstat = gic_read_iar();
 	u32 irqnr = gic_iar_irqnr(irqstat);
@@ -192,75 +192,6 @@ static void setup_irq(irq_handler_fn handler)
 }
 
 #if defined(__aarch64__)
-struct its_event {
-	int cpu_id;
-	int lpi_id;
-};
-
-struct its_stats {
-	struct its_event expected;
-	struct its_event observed;
-};
-
-static struct its_stats lpi_stats;
-
-static void lpi_handler(struct pt_regs *regs __unused)
-{
-	u32 irqstat = gic_read_iar();
-	int irqnr = gic_iar_irqnr(irqstat);
-
-	gic_write_eoir(irqstat);
-	assert(irqnr >= 8192);
-	smp_rmb(); /* pairs with wmb in lpi_stats_expect */
-	lpi_stats.observed.cpu_id = smp_processor_id();
-	lpi_stats.observed.lpi_id = irqnr;
-	acked[lpi_stats.observed.cpu_id]++;
-	smp_wmb(); /* pairs with rmb in check_lpi_stats */
-}
-
-static void lpi_stats_expect(int exp_cpu_id, int exp_lpi_id)
-{
-	lpi_stats.expected.cpu_id = exp_cpu_id;
-	lpi_stats.expected.lpi_id = exp_lpi_id;
-	lpi_stats.observed.cpu_id = -1;
-	lpi_stats.observed.lpi_id = -1;
-	smp_wmb(); /* pairs with rmb in handler */
-}
-
-static void check_lpi_stats(const char *msg)
-{
-	int i;
-
-	for (i = 0; i < 50; i++) {
-		mdelay(100);
-		smp_rmb(); /* pairs with wmb in lpi_handler */
-		if (lpi_stats.observed.cpu_id == lpi_stats.expected.cpu_id &&
-		    lpi_stats.observed.lpi_id == lpi_stats.expected.lpi_id) {
-			report(true, "%s", msg);
-			return;
-		}
-	}
-
-	if (lpi_stats.observed.cpu_id == -1 && lpi_stats.observed.lpi_id == -1) {
-		report_info("No LPI received whereas (cpuid=%d, intid=%d) "
-			    "was expected", lpi_stats.expected.cpu_id,
-			    lpi_stats.expected.lpi_id);
-	} else {
-		report_info("Unexpected LPI (cpuid=%d, intid=%d)",
-			    lpi_stats.observed.cpu_id,
-			    lpi_stats.observed.lpi_id);
-	}
-	report(false, "%s", msg);
-}
-
-static void secondary_lpi_test(void)
-{
-	setup_irq(lpi_handler);
-	cpumask_set_cpu(smp_processor_id(), &ready);
-	while (1)
-		wfi();
-}
-
 static void check_lpi_hits(int *expected, const char *msg)
 {
 	bool pass = true;
@@ -350,7 +281,7 @@ static void ipi_test_smp(void)
 
 static void ipi_send(void)
 {
-	setup_irq(ipi_handler);
+	setup_irq(irq_handler);
 	wait_on_ready();
 	ipi_test_self();
 	ipi_test_smp();
@@ -358,9 +289,9 @@ static void ipi_send(void)
 	exit(report_summary());
 }
 
-static void ipi_recv(void)
+static void irq_recv(void)
 {
-	setup_irq(ipi_handler);
+	setup_irq(irq_handler);
 	cpumask_set_cpu(smp_processor_id(), &ready);
 	while (1)
 		wfi();
@@ -371,7 +302,7 @@ static void ipi_test(void *data __unused)
 	if (smp_processor_id() == IPI_SENDER)
 		ipi_send();
 	else
-		ipi_recv();
+		irq_recv();
 }
 
 static struct gic gicv2 = {
@@ -699,14 +630,12 @@ static int its_prerequisites(int nb_cpus)
 		return -1;
 	}
 
-	stats_reset();
-
-	setup_irq(lpi_handler);
+	setup_irq(irq_handler);
 
 	for_each_present_cpu(cpu) {
 		if (cpu == 0)
 			continue;
-		smp_boot_secondary(cpu, secondary_lpi_test);
+		smp_boot_secondary(cpu, irq_recv);
 	}
 	wait_on_ready();
 
@@ -760,6 +689,7 @@ static void test_its_trigger(void)
 {
 	struct its_collection *col3;
 	struct its_device *dev2, *dev7;
+	cpumask_t mask;
 
 	if (its_setup1())
 		return;
@@ -770,13 +700,21 @@ static void test_its_trigger(void)
 
 	report_prefix_push("int");
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev=2, eventid=20  -> lpi= 8195, col=3");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev=2, eventid=20  -> lpi= 8195, col=3");
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev=7, eventid=255 -> lpi= 8196, col=2");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev=7, eventid=255 -> lpi= 8196, col=2");
 
 	report_prefix_pop();
 
@@ -789,9 +727,12 @@ static void test_its_trigger(void)
 	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT & ~LPI_PROP_ENABLED);
 	its_send_inv(dev2, 20);
 
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 does not trigger any LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1),
+			"dev2/eventid=20 does not trigger any LPI");
 
 	/*
 	 * re-enable the LPI but willingly do not call invall
@@ -799,18 +740,31 @@ static void test_its_trigger(void)
 	 * The LPI should not hit
 	 */
 	gicv3_lpi_set_config(8195, LPI_PROP_DEFAULT);
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 still does not trigger any LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1),
+			"dev2/eventid=20 still does not trigger any LPI");
 
 	/* Now call the invall and check the LPI hits */
+	stats_reset();
+	/* The barrier is from its_send_int() */
+	wmb();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_invall(col3);
-	lpi_stats_expect(3, 8195);
-	check_lpi_stats("dev2/eventid=20 pending LPI is received");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 pending LPI is received");
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 now triggers an LPI");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 now triggers an LPI");
 
 	report_prefix_pop();
 
@@ -821,9 +775,13 @@ static void test_its_trigger(void)
 	 */
 
 	its_send_mapd(dev2, false);
-	lpi_stats_expect(-1, -1);
+	stats_reset();
+	cpumask_clear(&mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("no LPI after device unmap");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, -1, -1), "no LPI after device unmap");
+
+	check_spurious();
 	report_prefix_pop();
 }
 
@@ -831,6 +789,7 @@ static void test_its_migration(void)
 {
 	struct its_device *dev2, *dev7;
 	bool test_skipped = false;
+	cpumask_t mask;
 
 	if (its_setup1()) {
 		test_skipped = true;
@@ -847,13 +806,23 @@ do_migrate:
 	if (test_skipped)
 		return;
 
-	lpi_stats_expect(3, 8195);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(3, &mask);
 	its_send_int(dev2, 20);
-	check_lpi_stats("dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8195),
+			"dev2/eventid=20 triggers LPI 8195 on PE #3 after migration");
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev7/eventid=255 triggers LPI 8196 on PE #2 after migration");
+
+	check_spurious();
 }
 
 #define ERRATA_UNMAPPED_COLLECTIONS "ERRATA_8c58be34494b"
@@ -863,6 +832,7 @@ static void test_migrate_unmapped_collection(void)
 	struct its_collection *col = NULL;
 	struct its_device *dev2 = NULL, *dev7 = NULL;
 	bool test_skipped = false;
+	cpumask_t mask;
 	int pe0 = 0;
 	u8 config;
 
@@ -897,17 +867,27 @@ do_migrate:
 	its_send_mapc(col, true);
 	its_send_invall(col);
 
-	lpi_stats_expect(2, 8196);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(2, &mask);
 	its_send_int(dev7, 255);
-	check_lpi_stats("dev7/eventid= 255 triggered LPI 8196 on PE #2");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8196),
+			"dev7/eventid= 255 triggered LPI 8196 on PE #2");
 
 	config = gicv3_lpi_get_config(8192);
 	report(config == LPI_PROP_DEFAULT,
 	       "Config of LPI 8192 was properly migrated");
 
-	lpi_stats_expect(pe0, 8192);
+	stats_reset();
+	cpumask_clear(&mask);
+	cpumask_set_cpu(pe0, &mask);
 	its_send_int(dev2, 0);
-	check_lpi_stats("dev2/eventid = 0 triggered LPI 8192 on PE0");
+	wait_for_interrupts(&mask);
+	report(check_acked(&mask, 0, 8192),
+			"dev2/eventid = 0 triggered LPI 8192 on PE0");
+
+	check_spurious();
 }
 
 static void test_its_pending_migration(void)
@@ -964,6 +944,10 @@ static void test_its_pending_migration(void)
 	pendbaser = readq(ptr);
 	writeq(pendbaser & ~GICR_PENDBASER_PTZ, ptr);
 
+	/*
+	 * Reset and initialization values for acked are the same, so we don't
+	 * need to explicitely call stats_reset().
+	 */
 	gicv3_lpi_rdist_enable(pe0);
 	gicv3_lpi_rdist_enable(pe1);
 
-- 
2.29.2

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm