Re: [Qemu-devel] [RFC PATCH] target-arm/psci.c: wake up sleeping CPUs (MTTCG)

[Andrew Jones <drjones@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2188 bytes --]

On Wed, Jun 24, 2015 at 08:12:52PM +0100, Peter Maydell wrote:
> On 24 June 2015 at 18:18, Alex Bennée <alex.bennee@linaro.org> wrote:
> >
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> >
> >> On 24/06/2015 17:34, Alex Bennée wrote:
> >>> Testing with Alexander's bare metal syncronisation tests fails in MTTCG
> >>> leaving one CPU spinning forever waiting for the second CPU to wake up.
> >>> We simply need to poke the halt_cond once we have processed the PSCI
> >>> power on call.
> >>>
> >>> Tested-by: Alex Bennée <alex.bennee@linaro.org>
> >>> CC: Alexander Spyridakis <a.spyridakis@virtualopensystems.com>
> >>>
> >>> ---
> >>> TODO
> >>>   - exactly how does the vexpress wake up it's sleeping CPUs?
> >>> ---
> >>>  target-arm/psci.c | 2 ++
> >>>  1 file changed, 2 insertions(+)
> >>>
> >>> diff --git a/target-arm/psci.c b/target-arm/psci.c
> >>> index d8fafab..661ff28 100644
> >>> --- a/target-arm/psci.c
> >>> +++ b/target-arm/psci.c
> >>> @@ -196,6 +196,8 @@ void arm_handle_psci_call(ARMCPU *cpu)
> >>>          }
> >>>          target_cpu_class->set_pc(target_cpu_state, entry);
> >>>
> >>> +        qemu_cond_signal(target_cpu_state->halt_cond);
> >>
> >> That's called qemu_cpu_kick(target_cpu_state). :)  The patch should be
> >> acceptable now upstream, I think.
> >
> > Oh so this might well fail in KVM too?
> 
> In KVM we won't use target-arm/psci.c because PSCI calls
> are handled in the kernel.
>

It's also not valid to use Alexander's test on KVM, as the test
framework doesn't enable the mmu, and thus {ldr,str}ex won't work
as expected.

I guess I need to do a better job at advertising/documenting
kvm-unit-tests/arm, as that framework would suit this test just
fine. I've attached a patch porting the test over to k-u-t[1].
After applying the patch, do

./configure --arch=arm64 --cross-prefix=aarch64-linux-gnu-
OR
./configure --arch=arm --cross-prefix=arm-linux-gnu-

make

arm/run arm/vos-spinlock-test.flat -smp 4 # non-atomic locks
OR
arm/run arm/vos-spinlock-test.flat -smp 4 -append atomic # atomic locks

Thanks,
drew

[1] git://git.kernel.org/pub/scm/virt/kvm/kvm-unit-tests.git

[-- Attachment #2: vos-spinlock-test.patch --]
[-- Type: text/plain, Size: 3054 bytes --]

>From dea26966a92cc65f23c0ac7bf2620982e0a9c57d Mon Sep 17 00:00:00 2001
From: Andrew Jones <drjones@redhat.com>
Date: Thu, 25 Jun 2015 17:28:53 +0200
Subject: [PATCH] arm/arm64: add spinlock torture test

This is port of the test in virtualopensystems tcg_baremetal_tests.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/vos-spinlock-test.c      | 92 ++++++++++++++++++++++++++++++++++++++++++++
 config/config-arm-common.mak |  4 +-
 2 files changed, 95 insertions(+), 1 deletion(-)
 create mode 100644 arm/vos-spinlock-test.c

diff --git a/arm/vos-spinlock-test.c b/arm/vos-spinlock-test.c
new file mode 100644
index 0000000000000..6cc62c6f49dfb
--- /dev/null
+++ b/arm/vos-spinlock-test.c
@@ -0,0 +1,92 @@
+#include <libcflat.h>
+#include <asm/smp.h>
+#include <asm/cpumask.h>
+#include <asm/barrier.h>
+
+#define LOOP_SIZE 10000000
+
+struct lock_ops {
+	void (*lock)(int *v);
+	void (*unlock)(int *v);
+};
+static struct lock_ops lock_ops;
+
+static void gcc_builtin_lock(int *lock_var)
+{
+	while (__sync_lock_test_and_set(lock_var, 1));
+}
+static void gcc_builtin_unlock(int *lock_var)
+{
+	__sync_lock_release(lock_var);
+}
+static void none_lock(int *lock_var)
+{
+	while (*lock_var != 0);
+	*lock_var = 1;
+}
+static void none_unlock(int *lock_var)
+{
+	*lock_var = 0;
+}
+
+static int global_a, global_b;
+static int global_lock;
+
+static cpumask_t smp_test_complete;
+
+static void test_spinlock(void)
+{
+	int i, errors = 0;
+	int cpu = smp_processor_id();
+
+	printf("CPU%d online\n", cpu);
+
+	for (i = 0; i < LOOP_SIZE; i++) {
+
+		lock_ops.lock(&global_lock);
+
+		if (global_a == (cpu + 1) % 2) {
+			global_a = 1;
+			global_b = 0;
+		} else {
+			global_a = 0;
+			global_b = 1;
+		}
+
+		if (global_a == global_b)
+			errors++;
+
+		lock_ops.unlock(&global_lock);
+	}
+	report("CPU%d: Done - Errors: %d\n", errors == 0, cpu, errors);
+
+	cpumask_set_cpu(cpu, &smp_test_complete);
+	if (cpu != 0)
+		halt();
+}
+
+int main(int argc, char **argv)
+{
+	int cpu;
+
+	if (argc && strcmp(argv[0], "atomic") == 0) {
+		lock_ops.lock = gcc_builtin_lock;
+		lock_ops.unlock = gcc_builtin_unlock;
+	} else {
+		lock_ops.lock = none_lock;
+		lock_ops.unlock = none_unlock;
+	}
+
+	for_each_present_cpu(cpu) {
+		if (cpu == 0)
+			continue;
+		smp_boot_secondary(cpu, test_spinlock);
+	}
+
+	test_spinlock();
+
+	while (!cpumask_full(&smp_test_complete))
+		cpu_relax();
+
+	return report_summary();
+}
diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
index 314261ef60cf7..6f4c8ee163c3d 100644
--- a/config/config-arm-common.mak
+++ b/config/config-arm-common.mak
@@ -10,7 +10,8 @@ ifeq ($(LOADADDR),)
 endif
 
 tests-common = \
-	$(TEST_DIR)/selftest.flat
+	$(TEST_DIR)/selftest.flat \
+	$(TEST_DIR)/vos-spinlock-test.flat
 
 all: test_cases
 
@@ -70,3 +71,4 @@ generated_files = $(asm-offsets)
 test_cases: $(generated_files) $(tests-common) $(tests)
 
 $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
+$(TEST_DIR)/vos-spinlock-test.elf: $(cstart.o) $(TEST_DIR)/vos-spinlock-test.o
-- 
2.4.3