[kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

Invoking run_tests.sh without the -g parameter will by default run all of
the tests for a given architecture. This patch series will add a test which
has the ability to bring down the host and thus it might be nice if we
double check that the user actually wants to run that test instead of
them unknowingly bringing down a machine they might not want to.

In order to do this add the option for a tests' group parameter in
unittests.cfg to include "nodefault" on order to indicate that it shouldn't
be run be default.

When tests are invoked via run_tests.sh those with the nodefault group
parameter will be skipped unless explicitly specified by the "-g" command
line option. When tests with the nodefault group parameter are built and
run standalone the user will be prompted on invocation to confirm that
they actually want to run the test.

This allows a developer to mark a test as having potentially adverse
effects and thus requires an extra level of confirmation from the user
before they are invoked. Existing functionality will be preserved and new
tests can choose any group other than "nodefault" if they want to be run
by default.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Move checking on standalone invokation into a function
	  "skip_nodefault" in scripts/runtime.bash

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arm/unittests.cfg       |  3 +++
 powerpc/unittests.cfg   |  3 +++
 scripts/mkstandalone.sh |  4 ++++
 scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
 x86/unittests.cfg       |  3 +++
 5 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ffd12e5..3f6fa45 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index ed4fdbe..0098cb6 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index d2bae19..b921416 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -74,6 +74,10 @@ generate_test ()
 
 	cat scripts/runtime.bash
 
+	if grep -qw "nodefault" <<<${args[1]}; then
+		echo "export STANDALONE=yes"
+	fi
+
 	echo "run ${args[@]}"
 }
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 0503cf0..383ebd4 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,6 +32,26 @@ get_cmdline()
     echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
 }
 
+skip_nodefault()
+{
+    [ "$STANDALONE" != yes ] && return 0
+
+    while true; do
+        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn
+        case $yn in
+            "Y" | "y" | "Yes" | "yes")
+                return 1
+                ;;
+            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
+                exit
+                ;;
+            *)
+                echo Please select Y or N
+                ;;
+        esac
+    done
+}
+
 function run()
 {
     local testname="$1"
@@ -48,10 +68,16 @@ function run()
         return
     fi
 
-    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
+    if [ -n "$only_group" ] && ! grep -qw "$only_group" <<<$groups; then
         return
     fi
 
+    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
+            skip_nodefault; then
+        echo -e "`SKIP` $testname (test marked as manual run only)"
+        return;
+    fi
+
     if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
         echo "`SKIP` $1 ($arch only)"
         return 2
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 60747cf..4a1f74e 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
-- 
2.5.5

[kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

Invoking run_tests.sh without the -g parameter will by default run all of
the tests for a given architecture. This patch series will add a test which
has the ability to bring down the host and thus it might be nice if we
double check that the user actually wants to run that test instead of
them unknowingly bringing down a machine they might not want to.

In order to do this add the option for a tests' group parameter in
unittests.cfg to include "nodefault" on order to indicate that it shouldn't
be run be default.

When tests are invoked via run_tests.sh those with the nodefault group
parameter will be skipped unless explicitly specified by the "-g" command
line option. When tests with the nodefault group parameter are built and
run standalone the user will be prompted on invocation to confirm that
they actually want to run the test.

This allows a developer to mark a test as having potentially adverse
effects and thus requires an extra level of confirmation from the user
before they are invoked. Existing functionality will be preserved and new
tests can choose any group other than "nodefault" if they want to be run
by default.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Move checking on standalone invokation into a function
	  "skip_nodefault" in scripts/runtime.bash

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 arm/unittests.cfg       |  3 +++
 powerpc/unittests.cfg   |  3 +++
 scripts/mkstandalone.sh |  4 ++++
 scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
 x86/unittests.cfg       |  3 +++
 5 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/arm/unittests.cfg b/arm/unittests.cfg
index ffd12e5..3f6fa45 100644
--- a/arm/unittests.cfg
+++ b/arm/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index ed4fdbe..0098cb6 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
index d2bae19..b921416 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -74,6 +74,10 @@ generate_test ()
 
 	cat scripts/runtime.bash
 
+	if grep -qw "nodefault" <<<${args[1]}; then
+		echo "export STANDALONE=yes"
+	fi
+
 	echo "run ${args[@]}"
 }
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 0503cf0..383ebd4 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -32,6 +32,26 @@ get_cmdline()
     echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
 }
 
+skip_nodefault()
+{
+    [ "$STANDALONE" != yes ] && return 0
+
+    while true; do
+        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn
+        case $yn in
+            "Y" | "y" | "Yes" | "yes")
+                return 1
+                ;;
+            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
+                exit
+                ;;
+            *)
+                echo Please select Y or N
+                ;;
+        esac
+    done
+}
+
 function run()
 {
     local testname="$1"
@@ -48,10 +68,16 @@ function run()
         return
     fi
 
-    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
+    if [ -n "$only_group" ] && ! grep -qw "$only_group" <<<$groups; then
         return
     fi
 
+    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
+            skip_nodefault; then
+        echo -e "`SKIP` $testname (test marked as manual run only)"
+        return;
+    fi
+
     if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
         echo "`SKIP` $1 ($arch only)"
         return 2
diff --git a/x86/unittests.cfg b/x86/unittests.cfg
index 60747cf..4a1f74e 100644
--- a/x86/unittests.cfg
+++ b/x86/unittests.cfg
@@ -12,6 +12,9 @@
 #					# specific to only one.
 # groups = <group_name1> <group_name2> ...	# Used to identify test cases
 #						# with run_tests -g ...
+#						# Specify group_name=nodefault
+#						# to have test not run by
+#						# default
 # accel = kvm|tcg		# Optionally specify if test must run with
 #				# kvm or tcg. If not specified, then kvm will
 #				# be used when available.
-- 
2.5.5

[kvm-unit-tests PATCH V3 2/5] lib/powerpc: Add generic decrementer exception handler

[Suraj Jitindar Singh]

Add the lib/powerpc/handlers.c file and associated header files as a place
to implement generic exception handler functions for inclusion in tests.

Add a generic exception handler for a decrementer (0x900) interrupt which
will reset the decrementer to its maximum value.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---

Change Log:

V2 -> V3:
	- Whitespace changes

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/handlers.h |  8 ++++++++
 lib/powerpc/handlers.c     | 22 ++++++++++++++++++++++
 lib/ppc64/asm/handlers.h   |  1 +
 powerpc/Makefile.common    |  1 +
 4 files changed, 32 insertions(+)
 create mode 100644 lib/powerpc/asm/handlers.h
 create mode 100644 lib/powerpc/handlers.c
 create mode 100644 lib/ppc64/asm/handlers.h

diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
new file mode 100644
index 0000000..64ba727
--- /dev/null
+++ b/lib/powerpc/asm/handlers.h
@@ -0,0 +1,8 @@
+#ifndef _ASMPOWERPC_HANDLERS_H_
+#define _ASMPOWERPC_HANDLERS_H_
+
+#include <asm/ptrace.h>
+
+void dec_except_handler(struct pt_regs *regs, void *data);
+
+#endif /* _ASMPOWERPC_HANDLERS_H_ */
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
new file mode 100644
index 0000000..be8226a
--- /dev/null
+++ b/lib/powerpc/handlers.c
@@ -0,0 +1,22 @@
+/*
+ * Generic exception handlers for registration and use in tests
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/handlers.h>
+#include <asm/ptrace.h>
+
+/*
+ * Generic handler for decrementer exceptions (0x900)
+ * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
+ */
+void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
+{
+	uint32_t dec = 0x7FFFFFFF;
+
+	asm volatile ("mtdec %0" : : "r" (dec));
+}
diff --git a/lib/ppc64/asm/handlers.h b/lib/ppc64/asm/handlers.h
new file mode 100644
index 0000000..92e6fb2
--- /dev/null
+++ b/lib/ppc64/asm/handlers.h
@@ -0,0 +1 @@
+#include "../../powerpc/asm/handlers.h"
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 3f8887d..404194b 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -36,6 +36,7 @@ cflatobjs += lib/powerpc/hcall.o
 cflatobjs += lib/powerpc/setup.o
 cflatobjs += lib/powerpc/rtas.o
 cflatobjs += lib/powerpc/processor.o
+cflatobjs += lib/powerpc/handlers.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-- 
2.5.5

[kvm-unit-tests PATCH V3 2/5] lib/powerpc: Add generic decrementer exception handler

[Suraj Jitindar Singh]

Add the lib/powerpc/handlers.c file and associated header files as a place
to implement generic exception handler functions for inclusion in tests.

Add a generic exception handler for a decrementer (0x900) interrupt which
will reset the decrementer to its maximum value.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
Reviewed-by: Thomas Huth <thuth@redhat.com>
---

Change Log:

V2 -> V3:
	- Whitespace changes

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/handlers.h |  8 ++++++++
 lib/powerpc/handlers.c     | 22 ++++++++++++++++++++++
 lib/ppc64/asm/handlers.h   |  1 +
 powerpc/Makefile.common    |  1 +
 4 files changed, 32 insertions(+)
 create mode 100644 lib/powerpc/asm/handlers.h
 create mode 100644 lib/powerpc/handlers.c
 create mode 100644 lib/ppc64/asm/handlers.h

diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
new file mode 100644
index 0000000..64ba727
--- /dev/null
+++ b/lib/powerpc/asm/handlers.h
@@ -0,0 +1,8 @@
+#ifndef _ASMPOWERPC_HANDLERS_H_
+#define _ASMPOWERPC_HANDLERS_H_
+
+#include <asm/ptrace.h>
+
+void dec_except_handler(struct pt_regs *regs, void *data);
+
+#endif /* _ASMPOWERPC_HANDLERS_H_ */
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
new file mode 100644
index 0000000..be8226a
--- /dev/null
+++ b/lib/powerpc/handlers.c
@@ -0,0 +1,22 @@
+/*
+ * Generic exception handlers for registration and use in tests
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#include <libcflat.h>
+#include <asm/handlers.h>
+#include <asm/ptrace.h>
+
+/*
+ * Generic handler for decrementer exceptions (0x900)
+ * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
+ */
+void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
+{
+	uint32_t dec = 0x7FFFFFFF;
+
+	asm volatile ("mtdec %0" : : "r" (dec));
+}
diff --git a/lib/ppc64/asm/handlers.h b/lib/ppc64/asm/handlers.h
new file mode 100644
index 0000000..92e6fb2
--- /dev/null
+++ b/lib/ppc64/asm/handlers.h
@@ -0,0 +1 @@
+#include "../../powerpc/asm/handlers.h"
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 3f8887d..404194b 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -36,6 +36,7 @@ cflatobjs += lib/powerpc/hcall.o
 cflatobjs += lib/powerpc/setup.o
 cflatobjs += lib/powerpc/rtas.o
 cflatobjs += lib/powerpc/processor.o
+cflatobjs += lib/powerpc/handlers.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-- 
2.5.5

[kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Suraj Jitindar Singh]

Add the lib/powerpc/smp.c file and associated header files as a place
to implement generic smp functionality for inclusion in tests.

Add functions start_all_cpus(), start_cpu() and start_thread() to start
all stopped threads of all cpus, all stopped threads of a single cpu or a
single stopped thread of a guest at a given execution location,
respectively.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- start_thread now returns int to reflect error, success or failure to
	  start thread
	- start_cpu returns number of threads on cpu and number successfully
	  started
	- start_all_cpus checks if number of threads started == total number of
	  threads - 1

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/smp.h   |  22 ++++++++++
 lib/powerpc/smp.c       | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ppc64/asm/smp.h     |   1 +
 powerpc/Makefile.common |   1 +
 4 files changed, 130 insertions(+)
 create mode 100644 lib/powerpc/asm/smp.h
 create mode 100644 lib/powerpc/smp.c
 create mode 100644 lib/ppc64/asm/smp.h

diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
new file mode 100644
index 0000000..21940b4
--- /dev/null
+++ b/lib/powerpc/asm/smp.h
@@ -0,0 +1,22 @@
+#ifndef _ASMPOWERPC_SMP_H_
+#define _ASMPOWERPC_SMP_H_
+
+#include <libcflat.h>
+
+extern int nr_threads;
+
+struct start_threads {
+	int nr_threads;
+	int nr_started;
+};
+
+typedef void (*secondary_entry_fn)(void);
+
+extern void halt(void);
+
+extern int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
+extern struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
+				      uint32_t r3);
+extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
+
+#endif /* _ASMPOWERPC_SMP_H_ */
diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
new file mode 100644
index 0000000..48c636e
--- /dev/null
+++ b/lib/powerpc/smp.c
@@ -0,0 +1,106 @@
+/*
+ * Secondary cpu support
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#include <devicetree.h>
+#include <asm/setup.h>
+#include <asm/rtas.h>
+#include <asm/smp.h>
+
+int nr_threads;
+
+struct secondary_entry_data {
+	secondary_entry_fn entry;
+	uint64_t r3;
+	int nr_started;
+};
+
+/*
+ * Start stopped thread cpu_id at entry
+ * Returns:	<0 on failure to start stopped cpu
+ *		0  on success
+ *		>0 on cpu not in stopped state
+ */
+int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3)
+{
+	int query_token, start_token, outputs[1], ret;
+
+	query_token = rtas_token("query-cpu-stopped-state");
+	start_token = rtas_token("start-cpu");
+	assert(query_token != RTAS_UNKNOWN_SERVICE &&
+			start_token != RTAS_UNKNOWN_SERVICE);
+
+	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
+	if (ret) /* rtas query call failed */
+		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
+	else if (!outputs[0]) { /* cpu in stopped state */
+		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
+		if (ret) /* rtas start-cpu call failed */
+			printf("failed to start cpu %d\n", cpu_id);
+	} else /* cpu not in stopped state */
+		ret = outputs[0];
+
+	return ret;
+}
+
+/*
+ * Start all stopped threads (vcpus) on cpu_node
+ * Returns: Number of stopped cpus which were successfully started
+ */
+struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
+			       uint32_t r3)
+{
+	int len, cpu, nr_threads, nr_started = 0;
+	const struct fdt_property *prop;
+	u32 *cpus;
+
+	/* Get the id array of threads on this cpu_node */
+	prop = fdt_get_property(dt_fdt(), cpu_node,
+			"ibm,ppc-interrupt-server#s", &len);
+	assert(prop);
+
+	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */
+	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
+
+	for (cpu = 0; cpu < nr_threads; cpu++) {
+		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3))
+			nr_started++;
+	}
+
+	return (struct start_threads) {nr_threads, nr_started};
+}
+
+static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
+{
+	struct secondary_entry_data *datap = info;
+	struct start_threads ret = start_cpu(fdtnode, datap->entry, datap->r3);
+
+	nr_threads += ret.nr_threads;
+	datap->nr_started += ret.nr_started;
+}
+
+/*
+ * Start all stopped cpus on the guest at entry with register 3 set to r3
+ * We expect that we come in with only one thread currently started
+ * Returns:	TRUE on success
+ *		FALSE on failure
+ */
+bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
+{
+	struct secondary_entry_data data = {
+		entry,
+		r3,
+		0
+	};
+	int ret;
+
+	ret = dt_for_each_cpu_node(&start_each_secondary, &data);
+	assert(ret == 0);
+
+	/* We expect that we come in with one thread already started */
+	return data.nr_started == (nr_threads - 1);
+}
diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
new file mode 100644
index 0000000..67ced75
--- /dev/null
+++ b/lib/ppc64/asm/smp.h
@@ -0,0 +1 @@
+#include "../../powerpc/asm/smp.h"
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 404194b..677030a 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
 cflatobjs += lib/powerpc/rtas.o
 cflatobjs += lib/powerpc/processor.o
 cflatobjs += lib/powerpc/handlers.o
+cflatobjs += lib/powerpc/smp.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-- 
2.5.5

[kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Suraj Jitindar Singh]

Add the lib/powerpc/smp.c file and associated header files as a place
to implement generic smp functionality for inclusion in tests.

Add functions start_all_cpus(), start_cpu() and start_thread() to start
all stopped threads of all cpus, all stopped threads of a single cpu or a
single stopped thread of a guest at a given execution location,
respectively.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- start_thread now returns int to reflect error, success or failure to
	  start thread
	- start_cpu returns number of threads on cpu and number successfully
	  started
	- start_all_cpus checks if number of threads started = total number of
	  threads - 1

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/smp.h   |  22 ++++++++++
 lib/powerpc/smp.c       | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ppc64/asm/smp.h     |   1 +
 powerpc/Makefile.common |   1 +
 4 files changed, 130 insertions(+)
 create mode 100644 lib/powerpc/asm/smp.h
 create mode 100644 lib/powerpc/smp.c
 create mode 100644 lib/ppc64/asm/smp.h

diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
new file mode 100644
index 0000000..21940b4
--- /dev/null
+++ b/lib/powerpc/asm/smp.h
@@ -0,0 +1,22 @@
+#ifndef _ASMPOWERPC_SMP_H_
+#define _ASMPOWERPC_SMP_H_
+
+#include <libcflat.h>
+
+extern int nr_threads;
+
+struct start_threads {
+	int nr_threads;
+	int nr_started;
+};
+
+typedef void (*secondary_entry_fn)(void);
+
+extern void halt(void);
+
+extern int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
+extern struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
+				      uint32_t r3);
+extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
+
+#endif /* _ASMPOWERPC_SMP_H_ */
diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
new file mode 100644
index 0000000..48c636e
--- /dev/null
+++ b/lib/powerpc/smp.c
@@ -0,0 +1,106 @@
+/*
+ * Secondary cpu support
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#include <devicetree.h>
+#include <asm/setup.h>
+#include <asm/rtas.h>
+#include <asm/smp.h>
+
+int nr_threads;
+
+struct secondary_entry_data {
+	secondary_entry_fn entry;
+	uint64_t r3;
+	int nr_started;
+};
+
+/*
+ * Start stopped thread cpu_id at entry
+ * Returns:	<0 on failure to start stopped cpu
+ *		0  on success
+ *		>0 on cpu not in stopped state
+ */
+int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3)
+{
+	int query_token, start_token, outputs[1], ret;
+
+	query_token = rtas_token("query-cpu-stopped-state");
+	start_token = rtas_token("start-cpu");
+	assert(query_token != RTAS_UNKNOWN_SERVICE &&
+			start_token != RTAS_UNKNOWN_SERVICE);
+
+	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
+	if (ret) /* rtas query call failed */
+		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
+	else if (!outputs[0]) { /* cpu in stopped state */
+		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
+		if (ret) /* rtas start-cpu call failed */
+			printf("failed to start cpu %d\n", cpu_id);
+	} else /* cpu not in stopped state */
+		ret = outputs[0];
+
+	return ret;
+}
+
+/*
+ * Start all stopped threads (vcpus) on cpu_node
+ * Returns: Number of stopped cpus which were successfully started
+ */
+struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
+			       uint32_t r3)
+{
+	int len, cpu, nr_threads, nr_started = 0;
+	const struct fdt_property *prop;
+	u32 *cpus;
+
+	/* Get the id array of threads on this cpu_node */
+	prop = fdt_get_property(dt_fdt(), cpu_node,
+			"ibm,ppc-interrupt-server#s", &len);
+	assert(prop);
+
+	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */
+	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
+
+	for (cpu = 0; cpu < nr_threads; cpu++) {
+		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3))
+			nr_started++;
+	}
+
+	return (struct start_threads) {nr_threads, nr_started};
+}
+
+static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
+{
+	struct secondary_entry_data *datap = info;
+	struct start_threads ret = start_cpu(fdtnode, datap->entry, datap->r3);
+
+	nr_threads += ret.nr_threads;
+	datap->nr_started += ret.nr_started;
+}
+
+/*
+ * Start all stopped cpus on the guest at entry with register 3 set to r3
+ * We expect that we come in with only one thread currently started
+ * Returns:	TRUE on success
+ *		FALSE on failure
+ */
+bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
+{
+	struct secondary_entry_data data = {
+		entry,
+		r3,
+		0
+	};
+	int ret;
+
+	ret = dt_for_each_cpu_node(&start_each_secondary, &data);
+	assert(ret = 0);
+
+	/* We expect that we come in with one thread already started */
+	return data.nr_started = (nr_threads - 1);
+}
diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
new file mode 100644
index 0000000..67ced75
--- /dev/null
+++ b/lib/ppc64/asm/smp.h
@@ -0,0 +1 @@
+#include "../../powerpc/asm/smp.h"
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 404194b..677030a 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
 cflatobjs += lib/powerpc/rtas.o
 cflatobjs += lib/powerpc/processor.o
 cflatobjs += lib/powerpc/handlers.o
+cflatobjs += lib/powerpc/smp.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-- 
2.5.5

[kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Suraj Jitindar Singh]

It would be nice if we had a generic sleep function which could be used
in unit tests, add one.

Add the variable tb_hz used to store the time base frequency which is read
from the device tree on setup.

Add functions msleep, usleep and sleep in processor.c to sleep for a given
number of milliseconds, microseconds and time base ticks respectively.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Add patch to series

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
 lib/powerpc/asm/setup.h     |  2 ++
 lib/powerpc/processor.c     | 13 +++++++++++++
 lib/powerpc/setup.c         | 12 ++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
index 09692bd..9554e08 100644
--- a/lib/powerpc/asm/processor.h
+++ b/lib/powerpc/asm/processor.h
@@ -1,11 +1,30 @@
 #ifndef _ASMPOWERPC_PROCESSOR_H_
 #define _ASMPOWERPC_PROCESSOR_H_
 
+#include <libcflat.h>
 #include <asm/ptrace.h>
 
+#define cpu_relax() asm volatile ("" : : : "memory")
+
 #ifndef __ASSEMBLY__
 void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
 void do_handle_exception(struct pt_regs *regs);
 #endif /* __ASSEMBLY__ */
 
+static inline uint64_t get_tb(void)
+{
+	uint64_t tb;
+	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
+	return tb;
+}
+
+extern void delay(uint64_t cycles);
+extern void udelay(uint64_t us);
+
+static inline void mdelay(uint64_t ms)
+{
+	while (ms--)
+		udelay(1000);
+}
+
 #endif /* _ASMPOWERPC_PROCESSOR_H_ */
diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
index b1e1e5a..23b4156 100644
--- a/lib/powerpc/asm/setup.h
+++ b/lib/powerpc/asm/setup.h
@@ -11,6 +11,8 @@
 extern u32 cpus[NR_CPUS];
 extern int nr_cpus;
 
+extern uint64_t tb_hz;
+
 #define NR_MEM_REGIONS		8
 #define MR_F_PRIMARY		(1U << 0)
 struct mem_region {
diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index a78bc3c..c84bae5 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -5,6 +5,7 @@
 #include <libcflat.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
+#include <asm/setup.h>
 
 static struct {
 	void (*func)(struct pt_regs *, void *data);
@@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
 	printf("unhandled cpu exception 0x%lx\n", regs->trap);
 	abort();
 }
+
+void delay(uint64_t cycles)
+{
+	uint64_t start = get_tb();
+	while((get_tb() - start) < cycles)
+		cpu_relax();
+}
+
+void udelay(uint64_t us)
+{
+	delay((us * tb_hz) / 1000000);
+}
diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
index e3d2afa..e7fa525 100644
--- a/lib/powerpc/setup.c
+++ b/lib/powerpc/setup.c
@@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
 
 u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
 int nr_cpus;
+uint64_t tb_hz;
 
 struct mem_region mem_regions[NR_MEM_REGIONS];
 phys_addr_t __physical_start, __physical_end;
@@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
 	}
 	cpus[cpu] = regval;
 
+	/* set timebase frequency */
+	if (!tb_hz) {
+		const struct fdt_property *prop;
+		u32 *data;
+		prop = fdt_get_property(dt_fdt(), fdtnode,
+					"timebase-frequency", NULL);
+		assert(prop != NULL);
+		data = (u32 *)prop->data;
+		tb_hz = fdt32_to_cpu(*data);
+	}
+
 	/* set exception stack address for this CPU (in SPGR0) */
 
 	asm volatile ("mtsprg0 %[addr]" ::
-- 
2.5.5

[kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Suraj Jitindar Singh]

It would be nice if we had a generic sleep function which could be used
in unit tests, add one.

Add the variable tb_hz used to store the time base frequency which is read
from the device tree on setup.

Add functions msleep, usleep and sleep in processor.c to sleep for a given
number of milliseconds, microseconds and time base ticks respectively.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Add patch to series

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
 lib/powerpc/asm/setup.h     |  2 ++
 lib/powerpc/processor.c     | 13 +++++++++++++
 lib/powerpc/setup.c         | 12 ++++++++++++
 4 files changed, 46 insertions(+)

diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
index 09692bd..9554e08 100644
--- a/lib/powerpc/asm/processor.h
+++ b/lib/powerpc/asm/processor.h
@@ -1,11 +1,30 @@
 #ifndef _ASMPOWERPC_PROCESSOR_H_
 #define _ASMPOWERPC_PROCESSOR_H_
 
+#include <libcflat.h>
 #include <asm/ptrace.h>
 
+#define cpu_relax() asm volatile ("" : : : "memory")
+
 #ifndef __ASSEMBLY__
 void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
 void do_handle_exception(struct pt_regs *regs);
 #endif /* __ASSEMBLY__ */
 
+static inline uint64_t get_tb(void)
+{
+	uint64_t tb;
+	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
+	return tb;
+}
+
+extern void delay(uint64_t cycles);
+extern void udelay(uint64_t us);
+
+static inline void mdelay(uint64_t ms)
+{
+	while (ms--)
+		udelay(1000);
+}
+
 #endif /* _ASMPOWERPC_PROCESSOR_H_ */
diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
index b1e1e5a..23b4156 100644
--- a/lib/powerpc/asm/setup.h
+++ b/lib/powerpc/asm/setup.h
@@ -11,6 +11,8 @@
 extern u32 cpus[NR_CPUS];
 extern int nr_cpus;
 
+extern uint64_t tb_hz;
+
 #define NR_MEM_REGIONS		8
 #define MR_F_PRIMARY		(1U << 0)
 struct mem_region {
diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
index a78bc3c..c84bae5 100644
--- a/lib/powerpc/processor.c
+++ b/lib/powerpc/processor.c
@@ -5,6 +5,7 @@
 #include <libcflat.h>
 #include <asm/processor.h>
 #include <asm/ptrace.h>
+#include <asm/setup.h>
 
 static struct {
 	void (*func)(struct pt_regs *, void *data);
@@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
 	printf("unhandled cpu exception 0x%lx\n", regs->trap);
 	abort();
 }
+
+void delay(uint64_t cycles)
+{
+	uint64_t start = get_tb();
+	while((get_tb() - start) < cycles)
+		cpu_relax();
+}
+
+void udelay(uint64_t us)
+{
+	delay((us * tb_hz) / 1000000);
+}
diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
index e3d2afa..e7fa525 100644
--- a/lib/powerpc/setup.c
+++ b/lib/powerpc/setup.c
@@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
 
 u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
 int nr_cpus;
+uint64_t tb_hz;
 
 struct mem_region mem_regions[NR_MEM_REGIONS];
 phys_addr_t __physical_start, __physical_end;
@@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
 	}
 	cpus[cpu] = regval;
 
+	/* set timebase frequency */
+	if (!tb_hz) {
+		const struct fdt_property *prop;
+		u32 *data;
+		prop = fdt_get_property(dt_fdt(), fdtnode,
+					"timebase-frequency", NULL);
+		assert(prop != NULL);
+		data = (u32 *)prop->data;
+		tb_hz = fdt32_to_cpu(*data);
+	}
+
 	/* set exception stack address for this CPU (in SPGR0) */
 
 	asm volatile ("mtsprg0 %[addr]" ::
-- 
2.5.5

[kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Suraj Jitindar Singh]

On Power machines if a guest cedes while a tm transaction is in the
suspended state then the checkpointed state of the vcpu may be lost and we
lose the cpu in the host.

Add a file for tm tests "powerpc/tm.c" and add a test to check if the fix
has been applied to the host kernel. If this fix hasn't been applied then
the test will never complete and the cpu will be lost. Otherwise the test
should succeed. Since this has the ability to mess things up in the host
mark this test as don't run by default.

This vulnerability has been assigned the ID CVE-2016-5412.

Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Remove get_dec() and sleep() functions as sleep functionality is
	  now provided by a generic implementation in processor.c.
	- Replace TM instructions with raw machine code to support older
	  compilers

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/hcall.h |   1 +
 powerpc/Makefile.common |   3 +-
 powerpc/tm.c            | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 +++
 4 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 powerpc/tm.c

diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
index 99bce79..80aa3e3 100644
--- a/lib/powerpc/asm/hcall.h
+++ b/lib/powerpc/asm/hcall.h
@@ -18,6 +18,7 @@
 #define H_SET_SPRG0		0x24
 #define H_SET_DABR		0x28
 #define H_PAGE_INIT		0x2c
+#define H_CEDE			0xE0
 #define H_PUT_TERM_CHAR		0x58
 #define H_RANDOM		0x300
 #define H_SET_MODE		0x31C
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 677030a..93e4f66 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -8,7 +8,8 @@ tests-common = \
 	$(TEST_DIR)/selftest.elf \
 	$(TEST_DIR)/spapr_hcall.elf \
 	$(TEST_DIR)/rtas.elf \
-	$(TEST_DIR)/emulator.elf
+	$(TEST_DIR)/emulator.elf \
+	$(TEST_DIR)/tm.elf
 
 all: $(TEST_DIR)/boot_rom.bin test_cases
 
diff --git a/powerpc/tm.c b/powerpc/tm.c
new file mode 100644
index 0000000..ba01f95
--- /dev/null
+++ b/powerpc/tm.c
@@ -0,0 +1,121 @@
+/*
+ * Transactional Memory Unit Tests
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <asm/hcall.h>
+#include <asm/processor.h>
+#include <asm/handlers.h>
+#include <asm/smp.h>
+
+static int h_cede(void)
+{
+	register uint64_t r3 asm("r3") = H_CEDE;
+
+	asm volatile ("sc 1" : "+r"(r3) :
+			     : "r0", "r4", "r5", "r6", "r7", "r8", "r9",
+			       "r10", "r11", "r12", "xer", "ctr", "cc");
+
+	return r3;
+}
+
+/*
+ * Enable transactional memory
+ * Returns:	FALSE - Failure
+ *		TRUE - Success
+ */
+static bool enable_tm(void)
+{
+	uint64_t msr = 0;
+
+	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
+
+	msr |= (((uint64_t) 1) << 32);
+
+	asm volatile ("mtmsrd %[msr]\n\t"
+		      "mfmsr %[msr]" : [msr] "+r" (msr));
+
+	return !!(msr & (((uint64_t) 1) << 32));
+}
+
+/*
+ * Test H_CEDE call while transactional memory transaction is suspended
+ *
+ * WARNING: This tests for a known vulnerability in which the host may go down.
+ * Probably best not to run this if your host going down is going to cause
+ * problems.
+ *
+ * If the test passes then your kernel probably has the necessary patch.
+ * If the test fails then the H_CEDE call was unsuccessful and the
+ * vulnerability wasn't tested.
+ * If the test hits the vulnerability then it will never complete or report and
+ * the qemu process will block indefinitely. RCU stalls will be detected on the
+ * cpu and any process scheduled on the lost cpu will also block indefinitely.
+ */
+static void test_h_cede_tm(int argc, char **argv)
+{
+	bool pass = true;
+	int i;
+
+	if (argc > 2)
+		report_abort("Unsupported argument: '%s'", argv[2]);
+
+	handle_exception(0x900, &dec_except_handler, NULL);
+
+	if (!start_all_cpus(&halt, 0))
+		report_abort("Failed to start secondary cpus");
+
+	if (!enable_tm())
+		report_abort("Failed to enable tm");
+
+	/*
+	 * Begin a transaction and guarantee we are in the suspend state
+	 * before continuing
+	 */
+	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin. */
+		      "beq 2f\n\t"
+		      ".long 0x7c0005dd\n\t"	/* tsuspend. */
+		      "2: .long 0x7c00059c\n\t"	/* tcheck cr0 */
+		      "bf 2,1b" : : : "cr0");
+
+	for (i = 0; i < 500 && pass; i++) {
+		uint64_t rval = h_cede();
+
+		if (rval != H_SUCCESS)
+			pass = false;
+		mdelay(5);
+	}
+
+	report("H_CEDE TM", pass);
+}
+
+struct {
+	const char *name;
+	void (*func)(int argc, char **argv);
+} hctests[] = {
+	{ "h_cede_tm", test_h_cede_tm },
+	{ NULL, NULL }
+};
+
+int main(int argc, char **argv)
+{
+	bool all;
+	int i;
+
+	report_prefix_push("tm");
+
+	all = argc == 1 || (argc == 2 && !strcmp(argv[1], "all"));
+
+	for (i = 0; hctests[i].name != NULL; i++) {
+		if (all || strcmp(argv[1], hctests[i].name) == 0) {
+			report_prefix_push(hctests[i].name);
+			hctests[i].func(argc, argv);
+			report_prefix_pop();
+		}
+	}
+
+	return report_summary();
+}
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 0098cb6..20dbde6 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -53,3 +53,9 @@ groups = rtas
 
 [emulator]
 file = emulator.elf
+
+[h_cede_tm]
+file = tm.elf
+smp = 2,threads=2
+extra_params = -append "h_cede_tm"
+groups = nodefault,h_cede_tm
-- 
2.5.5

[kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Suraj Jitindar Singh]

On Power machines if a guest cedes while a tm transaction is in the
suspended state then the checkpointed state of the vcpu may be lost and we
lose the cpu in the host.

Add a file for tm tests "powerpc/tm.c" and add a test to check if the fix
has been applied to the host kernel. If this fix hasn't been applied then
the test will never complete and the cpu will be lost. Otherwise the test
should succeed. Since this has the ability to mess things up in the host
mark this test as don't run by default.

This vulnerability has been assigned the ID CVE-2016-5412.

Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---

Change Log:

V2 -> V3:
	- Remove get_dec() and sleep() functions as sleep functionality is
	  now provided by a generic implementation in processor.c.
	- Replace TM instructions with raw machine code to support older
	  compilers

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/hcall.h |   1 +
 powerpc/Makefile.common |   3 +-
 powerpc/tm.c            | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 +++
 4 files changed, 130 insertions(+), 1 deletion(-)
 create mode 100644 powerpc/tm.c

diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
index 99bce79..80aa3e3 100644
--- a/lib/powerpc/asm/hcall.h
+++ b/lib/powerpc/asm/hcall.h
@@ -18,6 +18,7 @@
 #define H_SET_SPRG0		0x24
 #define H_SET_DABR		0x28
 #define H_PAGE_INIT		0x2c
+#define H_CEDE			0xE0
 #define H_PUT_TERM_CHAR		0x58
 #define H_RANDOM		0x300
 #define H_SET_MODE		0x31C
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 677030a..93e4f66 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -8,7 +8,8 @@ tests-common = \
 	$(TEST_DIR)/selftest.elf \
 	$(TEST_DIR)/spapr_hcall.elf \
 	$(TEST_DIR)/rtas.elf \
-	$(TEST_DIR)/emulator.elf
+	$(TEST_DIR)/emulator.elf \
+	$(TEST_DIR)/tm.elf
 
 all: $(TEST_DIR)/boot_rom.bin test_cases
 
diff --git a/powerpc/tm.c b/powerpc/tm.c
new file mode 100644
index 0000000..ba01f95
--- /dev/null
+++ b/powerpc/tm.c
@@ -0,0 +1,121 @@
+/*
+ * Transactional Memory Unit Tests
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+#include <libcflat.h>
+#include <asm/hcall.h>
+#include <asm/processor.h>
+#include <asm/handlers.h>
+#include <asm/smp.h>
+
+static int h_cede(void)
+{
+	register uint64_t r3 asm("r3") = H_CEDE;
+
+	asm volatile ("sc 1" : "+r"(r3) :
+			     : "r0", "r4", "r5", "r6", "r7", "r8", "r9",
+			       "r10", "r11", "r12", "xer", "ctr", "cc");
+
+	return r3;
+}
+
+/*
+ * Enable transactional memory
+ * Returns:	FALSE - Failure
+ *		TRUE - Success
+ */
+static bool enable_tm(void)
+{
+	uint64_t msr = 0;
+
+	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
+
+	msr |= (((uint64_t) 1) << 32);
+
+	asm volatile ("mtmsrd %[msr]\n\t"
+		      "mfmsr %[msr]" : [msr] "+r" (msr));
+
+	return !!(msr & (((uint64_t) 1) << 32));
+}
+
+/*
+ * Test H_CEDE call while transactional memory transaction is suspended
+ *
+ * WARNING: This tests for a known vulnerability in which the host may go down.
+ * Probably best not to run this if your host going down is going to cause
+ * problems.
+ *
+ * If the test passes then your kernel probably has the necessary patch.
+ * If the test fails then the H_CEDE call was unsuccessful and the
+ * vulnerability wasn't tested.
+ * If the test hits the vulnerability then it will never complete or report and
+ * the qemu process will block indefinitely. RCU stalls will be detected on the
+ * cpu and any process scheduled on the lost cpu will also block indefinitely.
+ */
+static void test_h_cede_tm(int argc, char **argv)
+{
+	bool pass = true;
+	int i;
+
+	if (argc > 2)
+		report_abort("Unsupported argument: '%s'", argv[2]);
+
+	handle_exception(0x900, &dec_except_handler, NULL);
+
+	if (!start_all_cpus(&halt, 0))
+		report_abort("Failed to start secondary cpus");
+
+	if (!enable_tm())
+		report_abort("Failed to enable tm");
+
+	/*
+	 * Begin a transaction and guarantee we are in the suspend state
+	 * before continuing
+	 */
+	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin. */
+		      "beq 2f\n\t"
+		      ".long 0x7c0005dd\n\t"	/* tsuspend. */
+		      "2: .long 0x7c00059c\n\t"	/* tcheck cr0 */
+		      "bf 2,1b" : : : "cr0");
+
+	for (i = 0; i < 500 && pass; i++) {
+		uint64_t rval = h_cede();
+
+		if (rval != H_SUCCESS)
+			pass = false;
+		mdelay(5);
+	}
+
+	report("H_CEDE TM", pass);
+}
+
+struct {
+	const char *name;
+	void (*func)(int argc, char **argv);
+} hctests[] = {
+	{ "h_cede_tm", test_h_cede_tm },
+	{ NULL, NULL }
+};
+
+int main(int argc, char **argv)
+{
+	bool all;
+	int i;
+
+	report_prefix_push("tm");
+
+	all = argc = 1 || (argc = 2 && !strcmp(argv[1], "all"));
+
+	for (i = 0; hctests[i].name != NULL; i++) {
+		if (all || strcmp(argv[1], hctests[i].name) = 0) {
+			report_prefix_push(hctests[i].name);
+			hctests[i].func(argc, argv);
+			report_prefix_pop();
+		}
+	}
+
+	return report_summary();
+}
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 0098cb6..20dbde6 100644
--- a/powerpc/unittests.cfg
+++ b/powerpc/unittests.cfg
@@ -53,3 +53,9 @@ groups = rtas
 
 [emulator]
 file = emulator.elf
+
+[h_cede_tm]
+file = tm.elf
+smp = 2,threads=2
+extra_params = -append "h_cede_tm"
+groups = nodefault,h_cede_tm
-- 
2.5.5

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
> Invoking run_tests.sh without the -g parameter will by default run all of
> the tests for a given architecture. This patch series will add a test which
> has the ability to bring down the host and thus it might be nice if we
> double check that the user actually wants to run that test instead of
> them unknowingly bringing down a machine they might not want to.
> 
> In order to do this add the option for a tests' group parameter in
> unittests.cfg to include "nodefault" on order to indicate that it shouldn't
> be run be default.
> 
> When tests are invoked via run_tests.sh those with the nodefault group
> parameter will be skipped unless explicitly specified by the "-g" command
> line option. When tests with the nodefault group parameter are built and
> run standalone the user will be prompted on invocation to confirm that
> they actually want to run the test.
> 
> This allows a developer to mark a test as having potentially adverse
> effects and thus requires an extra level of confirmation from the user
> before they are invoked. Existing functionality will be preserved and new
> tests can choose any group other than "nodefault" if they want to be run
> by default.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Move checking on standalone invokation into a function
> 	  "skip_nodefault" in scripts/runtime.bash
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arm/unittests.cfg       |  3 +++
>  powerpc/unittests.cfg   |  3 +++
>  scripts/mkstandalone.sh |  4 ++++
>  scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
>  x86/unittests.cfg       |  3 +++
>  5 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index ffd12e5..3f6fa45 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index ed4fdbe..0098cb6 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index d2bae19..b921416 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -74,6 +74,10 @@ generate_test ()
>  
>  	cat scripts/runtime.bash
>  
> +	if grep -qw "nodefault" <<<${args[1]}; then
> +		echo "export STANDALONE=yes"
> +	fi
> +

The answer is 42. Or, rather, we already unconditionally do this
on line 42 of this file, so this hunk isn't needed.

>  	echo "run ${args[@]}"
>  }
>  
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 0503cf0..383ebd4 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -32,6 +32,26 @@ get_cmdline()
>      echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
>  }
>  
> +skip_nodefault()
> +{
> +    [ "$STANDALONE" != yes ] && return 0

For consistency throughout our scripts, please quote all strings, like "yes"

> +
> +    while true; do
> +        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn
> +        case $yn in
> +            "Y" | "y" | "Yes" | "yes")

What about "YES" :-)

Actually, I'd just accept 'Y' for yes, and nothing else, like the prompt says.
And, instead of looping for valid input, all other input can just mean no.

> +                return 1
> +                ;;
> +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")

We should output something when the answer is 'no' like "User aborted",
or whatever.

> +                exit
> +                ;;
> +            *)
> +                echo Please select Y or N
> +                ;;
> +        esac
> +    done
> +}
> +
>  function run()
>  {
>      local testname="$1"
> @@ -48,10 +68,16 @@ function run()
>          return
>      fi
>  
> -    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
> +    if [ -n "$only_group" ] && ! grep -qw "$only_group" <<<$groups; then
>          return
>      fi
>  
> +    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> +            skip_nodefault; then
> +        echo -e "`SKIP` $testname (test marked as manual run only)"
> +        return;
> +    fi
> +
>      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
>          echo "`SKIP` $1 ($arch only)"
>          return 2
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 60747cf..4a1f74e 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> -- 
> 2.5.5
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
> Invoking run_tests.sh without the -g parameter will by default run all of
> the tests for a given architecture. This patch series will add a test which
> has the ability to bring down the host and thus it might be nice if we
> double check that the user actually wants to run that test instead of
> them unknowingly bringing down a machine they might not want to.
> 
> In order to do this add the option for a tests' group parameter in
> unittests.cfg to include "nodefault" on order to indicate that it shouldn't
> be run be default.
> 
> When tests are invoked via run_tests.sh those with the nodefault group
> parameter will be skipped unless explicitly specified by the "-g" command
> line option. When tests with the nodefault group parameter are built and
> run standalone the user will be prompted on invocation to confirm that
> they actually want to run the test.
> 
> This allows a developer to mark a test as having potentially adverse
> effects and thus requires an extra level of confirmation from the user
> before they are invoked. Existing functionality will be preserved and new
> tests can choose any group other than "nodefault" if they want to be run
> by default.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Move checking on standalone invokation into a function
> 	  "skip_nodefault" in scripts/runtime.bash
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  arm/unittests.cfg       |  3 +++
>  powerpc/unittests.cfg   |  3 +++
>  scripts/mkstandalone.sh |  4 ++++
>  scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
>  x86/unittests.cfg       |  3 +++
>  5 files changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> index ffd12e5..3f6fa45 100644
> --- a/arm/unittests.cfg
> +++ b/arm/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index ed4fdbe..0098cb6 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> index d2bae19..b921416 100755
> --- a/scripts/mkstandalone.sh
> +++ b/scripts/mkstandalone.sh
> @@ -74,6 +74,10 @@ generate_test ()
>  
>  	cat scripts/runtime.bash
>  
> +	if grep -qw "nodefault" <<<${args[1]}; then
> +		echo "export STANDALONE=yes"
> +	fi
> +

The answer is 42. Or, rather, we already unconditionally do this
on line 42 of this file, so this hunk isn't needed.

>  	echo "run ${args[@]}"
>  }
>  
> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> index 0503cf0..383ebd4 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -32,6 +32,26 @@ get_cmdline()
>      echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel $RUNTIME_arch_run $kernel -smp $smp $opts"
>  }
>  
> +skip_nodefault()
> +{
> +    [ "$STANDALONE" != yes ] && return 0

For consistency throughout our scripts, please quote all strings, like "yes"

> +
> +    while true; do
> +        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn
> +        case $yn in
> +            "Y" | "y" | "Yes" | "yes")

What about "YES" :-)

Actually, I'd just accept 'Y' for yes, and nothing else, like the prompt says.
And, instead of looping for valid input, all other input can just mean no.

> +                return 1
> +                ;;
> +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")

We should output something when the answer is 'no' like "User aborted",
or whatever.

> +                exit
> +                ;;
> +            *)
> +                echo Please select Y or N
> +                ;;
> +        esac
> +    done
> +}
> +
>  function run()
>  {
>      local testname="$1"
> @@ -48,10 +68,16 @@ function run()
>          return
>      fi
>  
> -    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups; then
> +    if [ -n "$only_group" ] && ! grep -qw "$only_group" <<<$groups; then
>          return
>      fi
>  
> +    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> +            skip_nodefault; then
> +        echo -e "`SKIP` $testname (test marked as manual run only)"
> +        return;
> +    fi
> +
>      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
>          echo "`SKIP` $1 ($arch only)"
>          return 2
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 60747cf..4a1f74e 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> -- 
> 2.5.5
>

Thanks,
drew 

Re: [kvm-unit-tests PATCH V3 2/5] lib/powerpc: Add generic decrementer exception handler

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:12PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/handlers.c file and associated header files as a place
> to implement generic exception handler functions for inclusion in tests.
> 
> Add a generic exception handler for a decrementer (0x900) interrupt which
> will reset the decrementer to its maximum value.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Whitespace changes
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/handlers.h |  8 ++++++++
>  lib/powerpc/handlers.c     | 22 ++++++++++++++++++++++
>  lib/ppc64/asm/handlers.h   |  1 +
>  powerpc/Makefile.common    |  1 +
>  4 files changed, 32 insertions(+)
>  create mode 100644 lib/powerpc/asm/handlers.h
>  create mode 100644 lib/powerpc/handlers.c
>  create mode 100644 lib/ppc64/asm/handlers.h

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
> new file mode 100644
> index 0000000..64ba727
> --- /dev/null
> +++ b/lib/powerpc/asm/handlers.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASMPOWERPC_HANDLERS_H_
> +#define _ASMPOWERPC_HANDLERS_H_
> +
> +#include <asm/ptrace.h>
> +
> +void dec_except_handler(struct pt_regs *regs, void *data);
> +
> +#endif /* _ASMPOWERPC_HANDLERS_H_ */
> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> new file mode 100644
> index 0000000..be8226a
> --- /dev/null
> +++ b/lib/powerpc/handlers.c
> @@ -0,0 +1,22 @@
> +/*
> + * Generic exception handlers for registration and use in tests
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/handlers.h>
> +#include <asm/ptrace.h>
> +
> +/*
> + * Generic handler for decrementer exceptions (0x900)
> + * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
> + */
> +void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
> +{
> +	uint32_t dec = 0x7FFFFFFF;
> +
> +	asm volatile ("mtdec %0" : : "r" (dec));
> +}
> diff --git a/lib/ppc64/asm/handlers.h b/lib/ppc64/asm/handlers.h
> new file mode 100644
> index 0000000..92e6fb2
> --- /dev/null
> +++ b/lib/ppc64/asm/handlers.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/handlers.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 3f8887d..404194b 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -36,6 +36,7 @@ cflatobjs += lib/powerpc/hcall.o
>  cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
> +cflatobjs += lib/powerpc/handlers.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH V3 2/5] lib/powerpc: Add generic decrementer exception handler

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:12PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/handlers.c file and associated header files as a place
> to implement generic exception handler functions for inclusion in tests.
> 
> Add a generic exception handler for a decrementer (0x900) interrupt which
> will reset the decrementer to its maximum value.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Whitespace changes
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/handlers.h |  8 ++++++++
>  lib/powerpc/handlers.c     | 22 ++++++++++++++++++++++
>  lib/ppc64/asm/handlers.h   |  1 +
>  powerpc/Makefile.common    |  1 +
>  4 files changed, 32 insertions(+)
>  create mode 100644 lib/powerpc/asm/handlers.h
>  create mode 100644 lib/powerpc/handlers.c
>  create mode 100644 lib/ppc64/asm/handlers.h

Reviewed-by: Andrew Jones <drjones@redhat.com>

> 
> diff --git a/lib/powerpc/asm/handlers.h b/lib/powerpc/asm/handlers.h
> new file mode 100644
> index 0000000..64ba727
> --- /dev/null
> +++ b/lib/powerpc/asm/handlers.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASMPOWERPC_HANDLERS_H_
> +#define _ASMPOWERPC_HANDLERS_H_
> +
> +#include <asm/ptrace.h>
> +
> +void dec_except_handler(struct pt_regs *regs, void *data);
> +
> +#endif /* _ASMPOWERPC_HANDLERS_H_ */
> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c
> new file mode 100644
> index 0000000..be8226a
> --- /dev/null
> +++ b/lib/powerpc/handlers.c
> @@ -0,0 +1,22 @@
> +/*
> + * Generic exception handlers for registration and use in tests
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <libcflat.h>
> +#include <asm/handlers.h>
> +#include <asm/ptrace.h>
> +
> +/*
> + * Generic handler for decrementer exceptions (0x900)
> + * Just reset the decrementer back to its maximum value (0x7FFFFFFF)
> + */
> +void dec_except_handler(struct pt_regs *regs __unused, void *data __unused)
> +{
> +	uint32_t dec = 0x7FFFFFFF;
> +
> +	asm volatile ("mtdec %0" : : "r" (dec));
> +}
> diff --git a/lib/ppc64/asm/handlers.h b/lib/ppc64/asm/handlers.h
> new file mode 100644
> index 0000000..92e6fb2
> --- /dev/null
> +++ b/lib/ppc64/asm/handlers.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/handlers.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 3f8887d..404194b 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -36,6 +36,7 @@ cflatobjs += lib/powerpc/hcall.o
>  cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
> +cflatobjs += lib/powerpc/handlers.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:13PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/smp.c file and associated header files as a place
> to implement generic smp functionality for inclusion in tests.
> 
> Add functions start_all_cpus(), start_cpu() and start_thread() to start
> all stopped threads of all cpus, all stopped threads of a single cpu or a
> single stopped thread of a guest at a given execution location,
> respectively.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- start_thread now returns int to reflect error, success or failure to
> 	  start thread
> 	- start_cpu returns number of threads on cpu and number successfully
> 	  started
> 	- start_all_cpus checks if number of threads started == total number of
> 	  threads - 1
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/smp.h   |  22 ++++++++++
>  lib/powerpc/smp.c       | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ppc64/asm/smp.h     |   1 +
>  powerpc/Makefile.common |   1 +
>  4 files changed, 130 insertions(+)
>  create mode 100644 lib/powerpc/asm/smp.h
>  create mode 100644 lib/powerpc/smp.c
>  create mode 100644 lib/ppc64/asm/smp.h
> 
> diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
> new file mode 100644
> index 0000000..21940b4
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,22 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +#include <libcflat.h>
> +
> +extern int nr_threads;
> +
> +struct start_threads {
> +	int nr_threads;
> +	int nr_started;
> +};
> +
> +typedef void (*secondary_entry_fn)(void);
> +
> +extern void halt(void);
> +
> +extern int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
> +extern struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> +				      uint32_t r3);
> +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..48c636e
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,106 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <devicetree.h>
> +#include <asm/setup.h>
> +#include <asm/rtas.h>
> +#include <asm/smp.h>
> +
> +int nr_threads;
> +
> +struct secondary_entry_data {
> +	secondary_entry_fn entry;
> +	uint64_t r3;
> +	int nr_started;
> +};
> +
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns:	<0 on failure to start stopped cpu
> + *		0  on success
> + *		>0 on cpu not in stopped state
> + */
> +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3)
> +{
> +	int query_token, start_token, outputs[1], ret;
> +
> +	query_token = rtas_token("query-cpu-stopped-state");
> +	start_token = rtas_token("start-cpu");
> +	assert(query_token != RTAS_UNKNOWN_SERVICE &&
> +			start_token != RTAS_UNKNOWN_SERVICE);
> +
> +	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> +	if (ret) /* rtas query call failed */
> +		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
> +	else if (!outputs[0]) { /* cpu in stopped state */
> +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
> +		if (ret) /* rtas start-cpu call failed */
> +			printf("failed to start cpu %d\n", cpu_id);
> +	} else /* cpu not in stopped state */
> +		ret = outputs[0];
> +
> +	return ret;
> +}
> +
> +/*
> + * Start all stopped threads (vcpus) on cpu_node
> + * Returns: Number of stopped cpus which were successfully started
> + */
> +struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> +			       uint32_t r3)
> +{
> +	int len, cpu, nr_threads, nr_started = 0;
> +	const struct fdt_property *prop;
> +	u32 *cpus;

The mixing of the terms cpus and threads in the function is confusing.
Let's change the variables like this

/cpus/threads/
/cpu/i/

> +
> +	/* Get the id array of threads on this cpu_node */
> +	prop = fdt_get_property(dt_fdt(), cpu_node,
> +			"ibm,ppc-interrupt-server#s", &len);
> +	assert(prop);
> +
> +	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */

/cpu/thread/ in comment

> +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */

/cpu/thread/ in comment

> +
> +	for (cpu = 0; cpu < nr_threads; cpu++) {
> +		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3))
> +			nr_started++;
> +	}
> +
> +	return (struct start_threads) {nr_threads, nr_started};
> +}
> +
> +static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
> +{
> +	struct secondary_entry_data *datap = info;
> +	struct start_threads ret = start_cpu(fdtnode, datap->entry, datap->r3);
> +
> +	nr_threads += ret.nr_threads;
> +	datap->nr_started += ret.nr_started;
> +}
> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * We expect that we come in with only one thread currently started
> + * Returns:	TRUE on success
> + *		FALSE on failure
> + */
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> +{
> +	struct secondary_entry_data data = {
> +		entry,
> +		r3,
> +		0
> +	};

nit: no need for all the white space, can just do = { entry, r3, 0 };

> +	int ret;
> +
> +	ret = dt_for_each_cpu_node(&start_each_secondary, &data);

nit: no need for '&' on function pointer.

> +	assert(ret == 0);
> +
> +	/* We expect that we come in with one thread already started */
> +	return data.nr_started == (nr_threads - 1);

nit: no need for the ()

You could also init data.nr_started to 1 above, and then avoid
the -1 here, but whatever.

> +}
> diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
> new file mode 100644
> index 0000000..67ced75
> --- /dev/null
> +++ b/lib/ppc64/asm/smp.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/smp.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 404194b..677030a 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
>  cflatobjs += lib/powerpc/handlers.o
> +cflatobjs += lib/powerpc/smp.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
>

My comments are just nits of varying degrees, so

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

Re: [kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:13PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/smp.c file and associated header files as a place
> to implement generic smp functionality for inclusion in tests.
> 
> Add functions start_all_cpus(), start_cpu() and start_thread() to start
> all stopped threads of all cpus, all stopped threads of a single cpu or a
> single stopped thread of a guest at a given execution location,
> respectively.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- start_thread now returns int to reflect error, success or failure to
> 	  start thread
> 	- start_cpu returns number of threads on cpu and number successfully
> 	  started
> 	- start_all_cpus checks if number of threads started = total number of
> 	  threads - 1
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/smp.h   |  22 ++++++++++
>  lib/powerpc/smp.c       | 106 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ppc64/asm/smp.h     |   1 +
>  powerpc/Makefile.common |   1 +
>  4 files changed, 130 insertions(+)
>  create mode 100644 lib/powerpc/asm/smp.h
>  create mode 100644 lib/powerpc/smp.c
>  create mode 100644 lib/ppc64/asm/smp.h
> 
> diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
> new file mode 100644
> index 0000000..21940b4
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,22 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +#include <libcflat.h>
> +
> +extern int nr_threads;
> +
> +struct start_threads {
> +	int nr_threads;
> +	int nr_started;
> +};
> +
> +typedef void (*secondary_entry_fn)(void);
> +
> +extern void halt(void);
> +
> +extern int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
> +extern struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> +				      uint32_t r3);
> +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..48c636e
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,106 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <devicetree.h>
> +#include <asm/setup.h>
> +#include <asm/rtas.h>
> +#include <asm/smp.h>
> +
> +int nr_threads;
> +
> +struct secondary_entry_data {
> +	secondary_entry_fn entry;
> +	uint64_t r3;
> +	int nr_started;
> +};
> +
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns:	<0 on failure to start stopped cpu
> + *		0  on success
> + *		>0 on cpu not in stopped state
> + */
> +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3)
> +{
> +	int query_token, start_token, outputs[1], ret;
> +
> +	query_token = rtas_token("query-cpu-stopped-state");
> +	start_token = rtas_token("start-cpu");
> +	assert(query_token != RTAS_UNKNOWN_SERVICE &&
> +			start_token != RTAS_UNKNOWN_SERVICE);
> +
> +	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> +	if (ret) /* rtas query call failed */
> +		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
> +	else if (!outputs[0]) { /* cpu in stopped state */
> +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
> +		if (ret) /* rtas start-cpu call failed */
> +			printf("failed to start cpu %d\n", cpu_id);
> +	} else /* cpu not in stopped state */
> +		ret = outputs[0];
> +
> +	return ret;
> +}
> +
> +/*
> + * Start all stopped threads (vcpus) on cpu_node
> + * Returns: Number of stopped cpus which were successfully started
> + */
> +struct start_threads start_cpu(int cpu_node, secondary_entry_fn entry,
> +			       uint32_t r3)
> +{
> +	int len, cpu, nr_threads, nr_started = 0;
> +	const struct fdt_property *prop;
> +	u32 *cpus;

The mixing of the terms cpus and threads in the function is confusing.
Let's change the variables like this

/cpus/threads/
/cpu/i/

> +
> +	/* Get the id array of threads on this cpu_node */
> +	prop = fdt_get_property(dt_fdt(), cpu_node,
> +			"ibm,ppc-interrupt-server#s", &len);
> +	assert(prop);
> +
> +	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per cpu */

/cpu/thread/ in comment

> +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */

/cpu/thread/ in comment

> +
> +	for (cpu = 0; cpu < nr_threads; cpu++) {
> +		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3))
> +			nr_started++;
> +	}
> +
> +	return (struct start_threads) {nr_threads, nr_started};
> +}
> +
> +static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
> +{
> +	struct secondary_entry_data *datap = info;
> +	struct start_threads ret = start_cpu(fdtnode, datap->entry, datap->r3);
> +
> +	nr_threads += ret.nr_threads;
> +	datap->nr_started += ret.nr_started;
> +}
> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * We expect that we come in with only one thread currently started
> + * Returns:	TRUE on success
> + *		FALSE on failure
> + */
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> +{
> +	struct secondary_entry_data data = {
> +		entry,
> +		r3,
> +		0
> +	};

nit: no need for all the white space, can just do = { entry, r3, 0 };

> +	int ret;
> +
> +	ret = dt_for_each_cpu_node(&start_each_secondary, &data);

nit: no need for '&' on function pointer.

> +	assert(ret = 0);
> +
> +	/* We expect that we come in with one thread already started */
> +	return data.nr_started = (nr_threads - 1);

nit: no need for the ()

You could also init data.nr_started to 1 above, and then avoid
the -1 here, but whatever.

> +}
> diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
> new file mode 100644
> index 0000000..67ced75
> --- /dev/null
> +++ b/lib/ppc64/asm/smp.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/smp.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 404194b..677030a 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
>  cflatobjs += lib/powerpc/handlers.o
> +cflatobjs += lib/powerpc/smp.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
>

My comments are just nits of varying degrees, so

Reviewed-by: Andrew Jones <drjones@redhat.com>

Thanks,
drew

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:14PM +1000, Suraj Jitindar Singh wrote:
> It would be nice if we had a generic sleep function which could be used
> in unit tests, add one.
> 
> Add the variable tb_hz used to store the time base frequency which is read
> from the device tree on setup.
> 
> Add functions msleep, usleep and sleep in processor.c to sleep for a given
> number of milliseconds, microseconds and time base ticks respectively.

Above you talk about adding *sleep functions, but below you add *delay
functions.

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Add patch to series
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
>  lib/powerpc/asm/setup.h     |  2 ++
>  lib/powerpc/processor.c     | 13 +++++++++++++
>  lib/powerpc/setup.c         | 12 ++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> index 09692bd..9554e08 100644
> --- a/lib/powerpc/asm/processor.h
> +++ b/lib/powerpc/asm/processor.h
> @@ -1,11 +1,30 @@
>  #ifndef _ASMPOWERPC_PROCESSOR_H_
>  #define _ASMPOWERPC_PROCESSOR_H_
>  
> +#include <libcflat.h>
>  #include <asm/ptrace.h>
>  
> +#define cpu_relax() asm volatile ("" : : : "memory")

Please add this to lib/asm-generic/barrier.h, then lib/ppc64/asm/barrier.h
will pick it up from there and you'll just need to make sure you include
asm/barrier.h in processor.c

> +
>  #ifndef __ASSEMBLY__
>  void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
>  void do_handle_exception(struct pt_regs *regs);
>  #endif /* __ASSEMBLY__ */
>  
> +static inline uint64_t get_tb(void)
> +{
> +	uint64_t tb;
> +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> +	return tb;
> +}
> +
> +extern void delay(uint64_t cycles);
> +extern void udelay(uint64_t us);
> +
> +static inline void mdelay(uint64_t ms)
> +{
> +	while (ms--)
> +		udelay(1000);
> +}
> +
>  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> index b1e1e5a..23b4156 100644
> --- a/lib/powerpc/asm/setup.h
> +++ b/lib/powerpc/asm/setup.h
> @@ -11,6 +11,8 @@
>  extern u32 cpus[NR_CPUS];
>  extern int nr_cpus;
>  
> +extern uint64_t tb_hz;
> +
>  #define NR_MEM_REGIONS		8
>  #define MR_F_PRIMARY		(1U << 0)
>  struct mem_region {
> diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> index a78bc3c..c84bae5 100644
> --- a/lib/powerpc/processor.c
> +++ b/lib/powerpc/processor.c
> @@ -5,6 +5,7 @@
>  #include <libcflat.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
> +#include <asm/setup.h>
>  
>  static struct {
>  	void (*func)(struct pt_regs *, void *data);
> @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
>  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
>  	abort();
>  }
> +
> +void delay(uint64_t cycles)
> +{
> +	uint64_t start = get_tb();
> +	while((get_tb() - start) < cycles)
> +		cpu_relax();
> +}
> +
> +void udelay(uint64_t us)
> +{
> +	delay((us * tb_hz) / 1000000);
> +}
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index e3d2afa..e7fa525 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
>  
>  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
>  int nr_cpus;
> +uint64_t tb_hz;
>  
>  struct mem_region mem_regions[NR_MEM_REGIONS];
>  phys_addr_t __physical_start, __physical_end;
> @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
>  	}
>  	cpus[cpu] = regval;
>  
> +	/* set timebase frequency */
> +	if (!tb_hz) {

Instead of this condition, can't we tuck this DT parsing into the
if (!read_common_info) block we already have?

> +		const struct fdt_property *prop;
> +		u32 *data;
> +		prop = fdt_get_property(dt_fdt(), fdtnode,
> +					"timebase-frequency", NULL);
> +		assert(prop != NULL);
> +		data = (u32 *)prop->data;
> +		tb_hz = fdt32_to_cpu(*data);
> +	}
> +
>  	/* set exception stack address for this CPU (in SPGR0) */
>  
>  	asm volatile ("mtsprg0 %[addr]" ::
> -- 
> 2.5.5
>

Otherwise looks good to me.

Thanks,
drew

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit test

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:14PM +1000, Suraj Jitindar Singh wrote:
> It would be nice if we had a generic sleep function which could be used
> in unit tests, add one.
> 
> Add the variable tb_hz used to store the time base frequency which is read
> from the device tree on setup.
> 
> Add functions msleep, usleep and sleep in processor.c to sleep for a given
> number of milliseconds, microseconds and time base ticks respectively.

Above you talk about adding *sleep functions, but below you add *delay
functions.

> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Add patch to series
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
>  lib/powerpc/asm/setup.h     |  2 ++
>  lib/powerpc/processor.c     | 13 +++++++++++++
>  lib/powerpc/setup.c         | 12 ++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> index 09692bd..9554e08 100644
> --- a/lib/powerpc/asm/processor.h
> +++ b/lib/powerpc/asm/processor.h
> @@ -1,11 +1,30 @@
>  #ifndef _ASMPOWERPC_PROCESSOR_H_
>  #define _ASMPOWERPC_PROCESSOR_H_
>  
> +#include <libcflat.h>
>  #include <asm/ptrace.h>
>  
> +#define cpu_relax() asm volatile ("" : : : "memory")

Please add this to lib/asm-generic/barrier.h, then lib/ppc64/asm/barrier.h
will pick it up from there and you'll just need to make sure you include
asm/barrier.h in processor.c

> +
>  #ifndef __ASSEMBLY__
>  void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
>  void do_handle_exception(struct pt_regs *regs);
>  #endif /* __ASSEMBLY__ */
>  
> +static inline uint64_t get_tb(void)
> +{
> +	uint64_t tb;
> +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> +	return tb;
> +}
> +
> +extern void delay(uint64_t cycles);
> +extern void udelay(uint64_t us);
> +
> +static inline void mdelay(uint64_t ms)
> +{
> +	while (ms--)
> +		udelay(1000);
> +}
> +
>  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> index b1e1e5a..23b4156 100644
> --- a/lib/powerpc/asm/setup.h
> +++ b/lib/powerpc/asm/setup.h
> @@ -11,6 +11,8 @@
>  extern u32 cpus[NR_CPUS];
>  extern int nr_cpus;
>  
> +extern uint64_t tb_hz;
> +
>  #define NR_MEM_REGIONS		8
>  #define MR_F_PRIMARY		(1U << 0)
>  struct mem_region {
> diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> index a78bc3c..c84bae5 100644
> --- a/lib/powerpc/processor.c
> +++ b/lib/powerpc/processor.c
> @@ -5,6 +5,7 @@
>  #include <libcflat.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
> +#include <asm/setup.h>
>  
>  static struct {
>  	void (*func)(struct pt_regs *, void *data);
> @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
>  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
>  	abort();
>  }
> +
> +void delay(uint64_t cycles)
> +{
> +	uint64_t start = get_tb();
> +	while((get_tb() - start) < cycles)
> +		cpu_relax();
> +}
> +
> +void udelay(uint64_t us)
> +{
> +	delay((us * tb_hz) / 1000000);
> +}
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index e3d2afa..e7fa525 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
>  
>  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
>  int nr_cpus;
> +uint64_t tb_hz;
>  
>  struct mem_region mem_regions[NR_MEM_REGIONS];
>  phys_addr_t __physical_start, __physical_end;
> @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
>  	}
>  	cpus[cpu] = regval;
>  
> +	/* set timebase frequency */
> +	if (!tb_hz) {

Instead of this condition, can't we tuck this DT parsing into the
if (!read_common_info) block we already have?

> +		const struct fdt_property *prop;
> +		u32 *data;
> +		prop = fdt_get_property(dt_fdt(), fdtnode,
> +					"timebase-frequency", NULL);
> +		assert(prop != NULL);
> +		data = (u32 *)prop->data;
> +		tb_hz = fdt32_to_cpu(*data);
> +	}
> +
>  	/* set exception stack address for this CPU (in SPGR0) */
>  
>  	asm volatile ("mtsprg0 %[addr]" ::
> -- 
> 2.5.5
>

Otherwise looks good to me.

Thanks,
drew

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Thomas Huth]

On 16.08.2016 09:18, Suraj Jitindar Singh wrote:
> It would be nice if we had a generic sleep function which could be used
> in unit tests, add one.
> 
> Add the variable tb_hz used to store the time base frequency which is read
> from the device tree on setup.
> 
> Add functions msleep, usleep and sleep in processor.c to sleep for a given
> number of milliseconds, microseconds and time base ticks respectively.

You finally called them mdelay, udelay and delay instead, so in case you
respin, please adjust the commit message.

> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Add patch to series
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
>  lib/powerpc/asm/setup.h     |  2 ++
>  lib/powerpc/processor.c     | 13 +++++++++++++
>  lib/powerpc/setup.c         | 12 ++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> index 09692bd..9554e08 100644
> --- a/lib/powerpc/asm/processor.h
> +++ b/lib/powerpc/asm/processor.h
> @@ -1,11 +1,30 @@
>  #ifndef _ASMPOWERPC_PROCESSOR_H_
>  #define _ASMPOWERPC_PROCESSOR_H_
>  
> +#include <libcflat.h>
>  #include <asm/ptrace.h>
>  
> +#define cpu_relax() asm volatile ("" : : : "memory")

I'd maybe add a "nop" instruction in there ... just in case?

>  #ifndef __ASSEMBLY__
>  void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
>  void do_handle_exception(struct pt_regs *regs);
>  #endif /* __ASSEMBLY__ */
>  
> +static inline uint64_t get_tb(void)
> +{
> +	uint64_t tb;
> +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> +	return tb;
> +}
> +
> +extern void delay(uint64_t cycles);
> +extern void udelay(uint64_t us);
> +
> +static inline void mdelay(uint64_t ms)
> +{
> +	while (ms--)
> +		udelay(1000);
> +}
> +
>  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> index b1e1e5a..23b4156 100644
> --- a/lib/powerpc/asm/setup.h
> +++ b/lib/powerpc/asm/setup.h
> @@ -11,6 +11,8 @@
>  extern u32 cpus[NR_CPUS];
>  extern int nr_cpus;
>  
> +extern uint64_t tb_hz;
> +
>  #define NR_MEM_REGIONS		8
>  #define MR_F_PRIMARY		(1U << 0)
>  struct mem_region {
> diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> index a78bc3c..c84bae5 100644
> --- a/lib/powerpc/processor.c
> +++ b/lib/powerpc/processor.c
> @@ -5,6 +5,7 @@
>  #include <libcflat.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
> +#include <asm/setup.h>
>  
>  static struct {
>  	void (*func)(struct pt_regs *, void *data);
> @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
>  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
>  	abort();
>  }
> +
> +void delay(uint64_t cycles)
> +{
> +	uint64_t start = get_tb();
> +	while((get_tb() - start) < cycles)
> +		cpu_relax();

Shouldn't there be some kind of wrap-around detection, like you had it
in v2 for the decrementer based sleep function?

> +}
> +
> +void udelay(uint64_t us)
> +{
> +	delay((us * tb_hz) / 1000000);
> +}
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index e3d2afa..e7fa525 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
>  
>  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
>  int nr_cpus;
> +uint64_t tb_hz;
>  
>  struct mem_region mem_regions[NR_MEM_REGIONS];
>  phys_addr_t __physical_start, __physical_end;
> @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
>  	}
>  	cpus[cpu] = regval;
>  
> +	/* set timebase frequency */
> +	if (!tb_hz) {
> +		const struct fdt_property *prop;
> +		u32 *data;
> +		prop = fdt_get_property(dt_fdt(), fdtnode,
> +					"timebase-frequency", NULL);
> +		assert(prop != NULL);
> +		data = (u32 *)prop->data;
> +		tb_hz = fdt32_to_cpu(*data);

You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get rid
of the "data" variable.

> +	}
> +
>  	/* set exception stack address for this CPU (in SPGR0) */
>  
>  	asm volatile ("mtsprg0 %[addr]" ::
> 

 Thomas

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit test

[Thomas Huth]

On 16.08.2016 09:18, Suraj Jitindar Singh wrote:
> It would be nice if we had a generic sleep function which could be used
> in unit tests, add one.
> 
> Add the variable tb_hz used to store the time base frequency which is read
> from the device tree on setup.
> 
> Add functions msleep, usleep and sleep in processor.c to sleep for a given
> number of milliseconds, microseconds and time base ticks respectively.

You finally called them mdelay, udelay and delay instead, so in case you
respin, please adjust the commit message.

> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Add patch to series
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
>  lib/powerpc/asm/setup.h     |  2 ++
>  lib/powerpc/processor.c     | 13 +++++++++++++
>  lib/powerpc/setup.c         | 12 ++++++++++++
>  4 files changed, 46 insertions(+)
> 
> diff --git a/lib/powerpc/asm/processor.h b/lib/powerpc/asm/processor.h
> index 09692bd..9554e08 100644
> --- a/lib/powerpc/asm/processor.h
> +++ b/lib/powerpc/asm/processor.h
> @@ -1,11 +1,30 @@
>  #ifndef _ASMPOWERPC_PROCESSOR_H_
>  #define _ASMPOWERPC_PROCESSOR_H_
>  
> +#include <libcflat.h>
>  #include <asm/ptrace.h>
>  
> +#define cpu_relax() asm volatile ("" : : : "memory")

I'd maybe add a "nop" instruction in there ... just in case?

>  #ifndef __ASSEMBLY__
>  void handle_exception(int trap, void (*func)(struct pt_regs *, void *), void *);
>  void do_handle_exception(struct pt_regs *regs);
>  #endif /* __ASSEMBLY__ */
>  
> +static inline uint64_t get_tb(void)
> +{
> +	uint64_t tb;
> +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> +	return tb;
> +}
> +
> +extern void delay(uint64_t cycles);
> +extern void udelay(uint64_t us);
> +
> +static inline void mdelay(uint64_t ms)
> +{
> +	while (ms--)
> +		udelay(1000);
> +}
> +
>  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> index b1e1e5a..23b4156 100644
> --- a/lib/powerpc/asm/setup.h
> +++ b/lib/powerpc/asm/setup.h
> @@ -11,6 +11,8 @@
>  extern u32 cpus[NR_CPUS];
>  extern int nr_cpus;
>  
> +extern uint64_t tb_hz;
> +
>  #define NR_MEM_REGIONS		8
>  #define MR_F_PRIMARY		(1U << 0)
>  struct mem_region {
> diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> index a78bc3c..c84bae5 100644
> --- a/lib/powerpc/processor.c
> +++ b/lib/powerpc/processor.c
> @@ -5,6 +5,7 @@
>  #include <libcflat.h>
>  #include <asm/processor.h>
>  #include <asm/ptrace.h>
> +#include <asm/setup.h>
>  
>  static struct {
>  	void (*func)(struct pt_regs *, void *data);
> @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
>  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
>  	abort();
>  }
> +
> +void delay(uint64_t cycles)
> +{
> +	uint64_t start = get_tb();
> +	while((get_tb() - start) < cycles)
> +		cpu_relax();

Shouldn't there be some kind of wrap-around detection, like you had it
in v2 for the decrementer based sleep function?

> +}
> +
> +void udelay(uint64_t us)
> +{
> +	delay((us * tb_hz) / 1000000);
> +}
> diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> index e3d2afa..e7fa525 100644
> --- a/lib/powerpc/setup.c
> +++ b/lib/powerpc/setup.c
> @@ -24,6 +24,7 @@ extern void setup_args_progname(const char *args);
>  
>  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
>  int nr_cpus;
> +uint64_t tb_hz;
>  
>  struct mem_region mem_regions[NR_MEM_REGIONS];
>  phys_addr_t __physical_start, __physical_end;
> @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, void *info)
>  	}
>  	cpus[cpu] = regval;
>  
> +	/* set timebase frequency */
> +	if (!tb_hz) {
> +		const struct fdt_property *prop;
> +		u32 *data;
> +		prop = fdt_get_property(dt_fdt(), fdtnode,
> +					"timebase-frequency", NULL);
> +		assert(prop != NULL);
> +		data = (u32 *)prop->data;
> +		tb_hz = fdt32_to_cpu(*data);

You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get rid
of the "data" variable.

> +	}
> +
>  	/* set exception stack address for this CPU (in SPGR0) */
>  
>  	asm volatile ("mtsprg0 %[addr]" ::
> 

 Thomas

Re: [kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:15PM +1000, Suraj Jitindar Singh wrote:
> On Power machines if a guest cedes while a tm transaction is in the
> suspended state then the checkpointed state of the vcpu may be lost and we
> lose the cpu in the host.
> 
> Add a file for tm tests "powerpc/tm.c" and add a test to check if the fix
> has been applied to the host kernel. If this fix hasn't been applied then
> the test will never complete and the cpu will be lost. Otherwise the test
> should succeed. Since this has the ability to mess things up in the host
> mark this test as don't run by default.
> 
> This vulnerability has been assigned the ID CVE-2016-5412.
> 
> Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Remove get_dec() and sleep() functions as sleep functionality is
> 	  now provided by a generic implementation in processor.c.
> 	- Replace TM instructions with raw machine code to support older
> 	  compilers
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/hcall.h |   1 +
>  powerpc/Makefile.common |   3 +-
>  powerpc/tm.c            | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   6 +++
>  4 files changed, 130 insertions(+), 1 deletion(-)
>  create mode 100644 powerpc/tm.c
> 
> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> index 99bce79..80aa3e3 100644
> --- a/lib/powerpc/asm/hcall.h
> +++ b/lib/powerpc/asm/hcall.h
> @@ -18,6 +18,7 @@
>  #define H_SET_SPRG0		0x24
>  #define H_SET_DABR		0x28
>  #define H_PAGE_INIT		0x2c
> +#define H_CEDE			0xE0
>  #define H_PUT_TERM_CHAR		0x58
>  #define H_RANDOM		0x300
>  #define H_SET_MODE		0x31C
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 677030a..93e4f66 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -8,7 +8,8 @@ tests-common = \
>  	$(TEST_DIR)/selftest.elf \
>  	$(TEST_DIR)/spapr_hcall.elf \
>  	$(TEST_DIR)/rtas.elf \
> -	$(TEST_DIR)/emulator.elf
> +	$(TEST_DIR)/emulator.elf \
> +	$(TEST_DIR)/tm.elf
>  
>  all: $(TEST_DIR)/boot_rom.bin test_cases
>  
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> new file mode 100644
> index 0000000..ba01f95
> --- /dev/null
> +++ b/powerpc/tm.c
> @@ -0,0 +1,121 @@
> +/*
> + * Transactional Memory Unit Tests
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/hcall.h>
> +#include <asm/processor.h>
> +#include <asm/handlers.h>
> +#include <asm/smp.h>
> +
> +static int h_cede(void)
> +{
> +	register uint64_t r3 asm("r3") = H_CEDE;
> +
> +	asm volatile ("sc 1" : "+r"(r3) :
> +			     : "r0", "r4", "r5", "r6", "r7", "r8", "r9",
> +			       "r10", "r11", "r12", "xer", "ctr", "cc");
> +
> +	return r3;
> +}
> +
> +/*
> + * Enable transactional memory
> + * Returns:	FALSE - Failure
> + *		TRUE - Success
> + */
> +static bool enable_tm(void)
> +{
> +	uint64_t msr = 0;
> +
> +	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
> +
> +	msr |= (((uint64_t) 1) << 32);
> +
> +	asm volatile ("mtmsrd %[msr]\n\t"
> +		      "mfmsr %[msr]" : [msr] "+r" (msr));
> +
> +	return !!(msr & (((uint64_t) 1) << 32));
> +}
> +
> +/*
> + * Test H_CEDE call while transactional memory transaction is suspended
> + *
> + * WARNING: This tests for a known vulnerability in which the host may go down.
> + * Probably best not to run this if your host going down is going to cause
> + * problems.
> + *
> + * If the test passes then your kernel probably has the necessary patch.
> + * If the test fails then the H_CEDE call was unsuccessful and the
> + * vulnerability wasn't tested.
> + * If the test hits the vulnerability then it will never complete or report and
> + * the qemu process will block indefinitely. RCU stalls will be detected on the
> + * cpu and any process scheduled on the lost cpu will also block indefinitely.
> + */
> +static void test_h_cede_tm(int argc, char **argv)
> +{
> +	bool pass = true;
> +	int i;
> +
> +	if (argc > 2)
> +		report_abort("Unsupported argument: '%s'", argv[2]);
> +
> +	handle_exception(0x900, &dec_except_handler, NULL);
> +
> +	if (!start_all_cpus(&halt, 0))

nit: more unnecessary '&' on function pointers

> +		report_abort("Failed to start secondary cpus");
> +
> +	if (!enable_tm())
> +		report_abort("Failed to enable tm");
> +
> +	/*
> +	 * Begin a transaction and guarantee we are in the suspend state
> +	 * before continuing
> +	 */
> +	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin. */
> +		      "beq 2f\n\t"
> +		      ".long 0x7c0005dd\n\t"	/* tsuspend. */
> +		      "2: .long 0x7c00059c\n\t"	/* tcheck cr0 */
> +		      "bf 2,1b" : : : "cr0");
> +
> +	for (i = 0; i < 500 && pass; i++) {
> +		uint64_t rval = h_cede();
> +
> +		if (rval != H_SUCCESS)
> +			pass = false;

nit: you could drop the '&& pass' and just do a break here.

> +		mdelay(5);
> +	}
> +
> +	report("H_CEDE TM", pass);

nit: with a break above you could just test for i == 500 here.

> +}
> +
> +struct {
> +	const char *name;
> +	void (*func)(int argc, char **argv);
> +} hctests[] = {
> +	{ "h_cede_tm", test_h_cede_tm },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	bool all;
> +	int i;
> +
> +	report_prefix_push("tm");
> +
> +	all = argc == 1 || (argc == 2 && !strcmp(argv[1], "all"));

I'd drop the argc == 2 test. You already know argc >= 2 at this
point. And, if it's > 2, then you want to call the test function
so the "Unsupported argument" abort will fire.

> +
> +	for (i = 0; hctests[i].name != NULL; i++) {
> +		if (all || strcmp(argv[1], hctests[i].name) == 0) {
> +			report_prefix_push(hctests[i].name);
> +			hctests[i].func(argc, argv);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	return report_summary();
> +}
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index 0098cb6..20dbde6 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -53,3 +53,9 @@ groups = rtas
>  
>  [emulator]
>  file = emulator.elf
> +
> +[h_cede_tm]
> +file = tm.elf
> +smp = 2,threads=2
> +extra_params = -append "h_cede_tm"
> +groups = nodefault,h_cede_tm
> -- 
> 2.5.5
>

Not my area of expertise, but I only see nits, so

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Andrew Jones]

On Tue, Aug 16, 2016 at 05:18:15PM +1000, Suraj Jitindar Singh wrote:
> On Power machines if a guest cedes while a tm transaction is in the
> suspended state then the checkpointed state of the vcpu may be lost and we
> lose the cpu in the host.
> 
> Add a file for tm tests "powerpc/tm.c" and add a test to check if the fix
> has been applied to the host kernel. If this fix hasn't been applied then
> the test will never complete and the cpu will be lost. Otherwise the test
> should succeed. Since this has the ability to mess things up in the host
> mark this test as don't run by default.
> 
> This vulnerability has been assigned the ID CVE-2016-5412.
> 
> Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
> 
> Change Log:
> 
> V2 -> V3:
> 	- Remove get_dec() and sleep() functions as sleep functionality is
> 	  now provided by a generic implementation in processor.c.
> 	- Replace TM instructions with raw machine code to support older
> 	  compilers
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/hcall.h |   1 +
>  powerpc/Makefile.common |   3 +-
>  powerpc/tm.c            | 121 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   6 +++
>  4 files changed, 130 insertions(+), 1 deletion(-)
>  create mode 100644 powerpc/tm.c
> 
> diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> index 99bce79..80aa3e3 100644
> --- a/lib/powerpc/asm/hcall.h
> +++ b/lib/powerpc/asm/hcall.h
> @@ -18,6 +18,7 @@
>  #define H_SET_SPRG0		0x24
>  #define H_SET_DABR		0x28
>  #define H_PAGE_INIT		0x2c
> +#define H_CEDE			0xE0
>  #define H_PUT_TERM_CHAR		0x58
>  #define H_RANDOM		0x300
>  #define H_SET_MODE		0x31C
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 677030a..93e4f66 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -8,7 +8,8 @@ tests-common = \
>  	$(TEST_DIR)/selftest.elf \
>  	$(TEST_DIR)/spapr_hcall.elf \
>  	$(TEST_DIR)/rtas.elf \
> -	$(TEST_DIR)/emulator.elf
> +	$(TEST_DIR)/emulator.elf \
> +	$(TEST_DIR)/tm.elf
>  
>  all: $(TEST_DIR)/boot_rom.bin test_cases
>  
> diff --git a/powerpc/tm.c b/powerpc/tm.c
> new file mode 100644
> index 0000000..ba01f95
> --- /dev/null
> +++ b/powerpc/tm.c
> @@ -0,0 +1,121 @@
> +/*
> + * Transactional Memory Unit Tests
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <asm/hcall.h>
> +#include <asm/processor.h>
> +#include <asm/handlers.h>
> +#include <asm/smp.h>
> +
> +static int h_cede(void)
> +{
> +	register uint64_t r3 asm("r3") = H_CEDE;
> +
> +	asm volatile ("sc 1" : "+r"(r3) :
> +			     : "r0", "r4", "r5", "r6", "r7", "r8", "r9",
> +			       "r10", "r11", "r12", "xer", "ctr", "cc");
> +
> +	return r3;
> +}
> +
> +/*
> + * Enable transactional memory
> + * Returns:	FALSE - Failure
> + *		TRUE - Success
> + */
> +static bool enable_tm(void)
> +{
> +	uint64_t msr = 0;
> +
> +	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
> +
> +	msr |= (((uint64_t) 1) << 32);
> +
> +	asm volatile ("mtmsrd %[msr]\n\t"
> +		      "mfmsr %[msr]" : [msr] "+r" (msr));
> +
> +	return !!(msr & (((uint64_t) 1) << 32));
> +}
> +
> +/*
> + * Test H_CEDE call while transactional memory transaction is suspended
> + *
> + * WARNING: This tests for a known vulnerability in which the host may go down.
> + * Probably best not to run this if your host going down is going to cause
> + * problems.
> + *
> + * If the test passes then your kernel probably has the necessary patch.
> + * If the test fails then the H_CEDE call was unsuccessful and the
> + * vulnerability wasn't tested.
> + * If the test hits the vulnerability then it will never complete or report and
> + * the qemu process will block indefinitely. RCU stalls will be detected on the
> + * cpu and any process scheduled on the lost cpu will also block indefinitely.
> + */
> +static void test_h_cede_tm(int argc, char **argv)
> +{
> +	bool pass = true;
> +	int i;
> +
> +	if (argc > 2)
> +		report_abort("Unsupported argument: '%s'", argv[2]);
> +
> +	handle_exception(0x900, &dec_except_handler, NULL);
> +
> +	if (!start_all_cpus(&halt, 0))

nit: more unnecessary '&' on function pointers

> +		report_abort("Failed to start secondary cpus");
> +
> +	if (!enable_tm())
> +		report_abort("Failed to enable tm");
> +
> +	/*
> +	 * Begin a transaction and guarantee we are in the suspend state
> +	 * before continuing
> +	 */
> +	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin. */
> +		      "beq 2f\n\t"
> +		      ".long 0x7c0005dd\n\t"	/* tsuspend. */
> +		      "2: .long 0x7c00059c\n\t"	/* tcheck cr0 */
> +		      "bf 2,1b" : : : "cr0");
> +
> +	for (i = 0; i < 500 && pass; i++) {
> +		uint64_t rval = h_cede();
> +
> +		if (rval != H_SUCCESS)
> +			pass = false;

nit: you could drop the '&& pass' and just do a break here.

> +		mdelay(5);
> +	}
> +
> +	report("H_CEDE TM", pass);

nit: with a break above you could just test for i = 500 here.

> +}
> +
> +struct {
> +	const char *name;
> +	void (*func)(int argc, char **argv);
> +} hctests[] = {
> +	{ "h_cede_tm", test_h_cede_tm },
> +	{ NULL, NULL }
> +};
> +
> +int main(int argc, char **argv)
> +{
> +	bool all;
> +	int i;
> +
> +	report_prefix_push("tm");
> +
> +	all = argc = 1 || (argc = 2 && !strcmp(argv[1], "all"));

I'd drop the argc = 2 test. You already know argc >= 2 at this
point. And, if it's > 2, then you want to call the test function
so the "Unsupported argument" abort will fire.

> +
> +	for (i = 0; hctests[i].name != NULL; i++) {
> +		if (all || strcmp(argv[1], hctests[i].name) = 0) {
> +			report_prefix_push(hctests[i].name);
> +			hctests[i].func(argc, argv);
> +			report_prefix_pop();
> +		}
> +	}
> +
> +	return report_summary();
> +}
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index 0098cb6..20dbde6 100644
> --- a/powerpc/unittests.cfg
> +++ b/powerpc/unittests.cfg
> @@ -53,3 +53,9 @@ groups = rtas
>  
>  [emulator]
>  file = emulator.elf
> +
> +[h_cede_tm]
> +file = tm.elf
> +smp = 2,threads=2
> +extra_params = -append "h_cede_tm"
> +groups = nodefault,h_cede_tm
> -- 
> 2.5.5
>

Not my area of expertise, but I only see nits, so

Reviewed-by: Andrew Jones <drjones@redhat.com> 

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Radim Krčmář]

2016-08-16 14:00+0200, Andrew Jones:
> On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> @@ -32,6 +32,26 @@ get_cmdline()
>> +skip_nodefault()
>> +{
>> +    while true; do
>> +        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn

I'd write "run this test" instead of "are you sure", or something
similar for the question.  The user can be sure that the has is marked
with nodefault. ;)

>> +        case $yn in
>> +            "Y" | "y" | "Yes" | "yes")
> 
> What about "YES" :-)

And exclamation marks!

"YES!!!"

> Actually, I'd just accept 'Y' for yes, and nothing else, like the prompt says.

NO.  If it is only one value, then make it "y".
motto: saving the Earth, one shift at a time.

This kind of user interface usually accepts at least "[yY]|[yY]es" ...
users will already be pissed that they have to input something and
denying a perfectly logical "yes" (which is what "y" stands for) is
going too overboard, IMO.

> And, instead of looping for valid input, all other input can just mean no.

"y/N" is the convention for writing a bool question that defaults to no.
I'd accept "" (just enter) as the default and then, looping isn't
unexpected and user already typed some crap in that case, so they
probably want to answer the question without having to run the command
again.

>> +                return 1
>> +                ;;
>> +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> 
> We should output something when the answer is 'no' like "User aborted",
> or whatever.
> 
>> +                exit

Wouldn't "return 1" and the SKIP message be enough?

>> +                ;;
>> +            *)
>> +                echo Please select Y or N

Just asking the question again is more common -- it's not hard to figure
out that the answer was not accepted.

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Radim Krčmář]

2016-08-16 14:00+0200, Andrew Jones:
> On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
>> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> @@ -32,6 +32,26 @@ get_cmdline()
>> +skip_nodefault()
>> +{
>> +    while true; do
>> +        read -p "Test marked not to be run by default, are you sure (Y/N)? " yn

I'd write "run this test" instead of "are you sure", or something
similar for the question.  The user can be sure that the has is marked
with nodefault. ;)

>> +        case $yn in
>> +            "Y" | "y" | "Yes" | "yes")
> 
> What about "YES" :-)

And exclamation marks!

"YES!!!"

> Actually, I'd just accept 'Y' for yes, and nothing else, like the prompt says.

NO.  If it is only one value, then make it "y".
motto: saving the Earth, one shift at a time.

This kind of user interface usually accepts at least "[yY]|[yY]es" ...
users will already be pissed that they have to input something and
denying a perfectly logical "yes" (which is what "y" stands for) is
going too overboard, IMO.

> And, instead of looping for valid input, all other input can just mean no.

"y/N" is the convention for writing a bool question that defaults to no.
I'd accept "" (just enter) as the default and then, looping isn't
unexpected and user already typed some crap in that case, so they
probably want to answer the question without having to run the command
again.

>> +                return 1
>> +                ;;
>> +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> 
> We should output something when the answer is 'no' like "User aborted",
> or whatever.
> 
>> +                exit

Wouldn't "return 1" and the SKIP message be enough?

>> +                ;;
>> +            *)
>> +                echo Please select Y or N

Just asking the question again is more common -- it's not hard to figure
out that the answer was not accepted.

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:00 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Invoking run_tests.sh without the -g parameter will by default run
> > all of
> > the tests for a given architecture. This patch series will add a
> > test which
> > has the ability to bring down the host and thus it might be nice if
> > we
> > double check that the user actually wants to run that test instead
> > of
> > them unknowingly bringing down a machine they might not want to.
> > 
> > In order to do this add the option for a tests' group parameter in
> > unittests.cfg to include "nodefault" on order to indicate that it
> > shouldn't
> > be run be default.
> > 
> > When tests are invoked via run_tests.sh those with the nodefault
> > group
> > parameter will be skipped unless explicitly specified by the "-g"
> > command
> > line option. When tests with the nodefault group parameter are
> > built and
> > run standalone the user will be prompted on invocation to confirm
> > that
> > they actually want to run the test.
> > 
> > This allows a developer to mark a test as having potentially
> > adverse
> > effects and thus requires an extra level of confirmation from the
> > user
> > before they are invoked. Existing functionality will be preserved
> > and new
> > tests can choose any group other than "nodefault" if they want to
> > be run
> > by default.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Move checking on standalone invokation into a function
> > 	  "skip_nodefault" in scripts/runtime.bash
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  arm/unittests.cfg       |  3 +++
> >  powerpc/unittests.cfg   |  3 +++
> >  scripts/mkstandalone.sh |  4 ++++
> >  scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
> >  x86/unittests.cfg       |  3 +++
> >  5 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index ffd12e5..3f6fa45 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index ed4fdbe..0098cb6 100644
> > --- a/powerpc/unittests.cfg
> > +++ b/powerpc/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > index d2bae19..b921416 100755
> > --- a/scripts/mkstandalone.sh
> > +++ b/scripts/mkstandalone.sh
> > @@ -74,6 +74,10 @@ generate_test ()
> >  
> >  	cat scripts/runtime.bash
> >  
> > +	if grep -qw "nodefault" <<<${args[1]}; then
> > +		echo "export STANDALONE=yes"
> > +	fi
> > +
> The answer is 42. Or, rather, we already unconditionally do this
> on line 42 of this file, so this hunk isn't needed.
Woops, missed that.
> 
> > 
> >  	echo "run ${args[@]}"
> >  }
> >  
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > index 0503cf0..383ebd4 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -32,6 +32,26 @@ get_cmdline()
> >      echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel
> > $RUNTIME_arch_run $kernel -smp $smp $opts"
> >  }
> >  
> > +skip_nodefault()
> > +{
> > +    [ "$STANDALONE" != yes ] && return 0
> For consistency throughout our scripts, please quote all strings,
> like "yes"
> 
> > 
> > +
> > +    while true; do
> > +        read -p "Test marked not to be run by default, are you
> > sure (Y/N)? " yn
> > +        case $yn in
> > +            "Y" | "y" | "Yes" | "yes")
> What about "YES" :-)
> 
> Actually, I'd just accept 'Y' for yes, and nothing else, like the
> prompt says.
> And, instead of looping for valid input, all other input can just
> mean no.
> 
> > 
> > +                return 1
> > +                ;;
> > +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> We should output something when the answer is 'no' like "User
> aborted",
> or whatever.
> 
> > 
> > +                exit
> > +                ;;
> > +            *)
> > +                echo Please select Y or N
> > +                ;;
> > +        esac
> > +    done
> > +}
> > +
> >  function run()
> >  {
> >      local testname="$1"
> > @@ -48,10 +68,16 @@ function run()
> >          return
> >      fi
> >  
> > -    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups;
> > then
> > +    if [ -n "$only_group" ] && ! grep -qw "$only_group"
> > <<<$groups; then
> >          return
> >      fi
> >  
> > +    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> > +            skip_nodefault; then
> > +        echo -e "`SKIP` $testname (test marked as manual run
> > only)"
> > +        return;
> > +    fi
> > +
> >      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
> >          echo "`SKIP` $1 ($arch only)"
> >          return 2
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 60747cf..4a1f74e 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:00 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Invoking run_tests.sh without the -g parameter will by default run
> > all of
> > the tests for a given architecture. This patch series will add a
> > test which
> > has the ability to bring down the host and thus it might be nice if
> > we
> > double check that the user actually wants to run that test instead
> > of
> > them unknowingly bringing down a machine they might not want to.
> > 
> > In order to do this add the option for a tests' group parameter in
> > unittests.cfg to include "nodefault" on order to indicate that it
> > shouldn't
> > be run be default.
> > 
> > When tests are invoked via run_tests.sh those with the nodefault
> > group
> > parameter will be skipped unless explicitly specified by the "-g"
> > command
> > line option. When tests with the nodefault group parameter are
> > built and
> > run standalone the user will be prompted on invocation to confirm
> > that
> > they actually want to run the test.
> > 
> > This allows a developer to mark a test as having potentially
> > adverse
> > effects and thus requires an extra level of confirmation from the
> > user
> > before they are invoked. Existing functionality will be preserved
> > and new
> > tests can choose any group other than "nodefault" if they want to
> > be run
> > by default.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Move checking on standalone invokation into a function
> > 	  "skip_nodefault" in scripts/runtime.bash
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  arm/unittests.cfg       |  3 +++
> >  powerpc/unittests.cfg   |  3 +++
> >  scripts/mkstandalone.sh |  4 ++++
> >  scripts/runtime.bash    | 28 +++++++++++++++++++++++++++-
> >  x86/unittests.cfg       |  3 +++
> >  5 files changed, 40 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arm/unittests.cfg b/arm/unittests.cfg
> > index ffd12e5..3f6fa45 100644
> > --- a/arm/unittests.cfg
> > +++ b/arm/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index ed4fdbe..0098cb6 100644
> > --- a/powerpc/unittests.cfg
> > +++ b/powerpc/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > index d2bae19..b921416 100755
> > --- a/scripts/mkstandalone.sh
> > +++ b/scripts/mkstandalone.sh
> > @@ -74,6 +74,10 @@ generate_test ()
> >  
> >  	cat scripts/runtime.bash
> >  
> > +	if grep -qw "nodefault" <<<${args[1]}; then
> > +		echo "export STANDALONE=yes"
> > +	fi
> > +
> The answer is 42. Or, rather, we already unconditionally do this
> on line 42 of this file, so this hunk isn't needed.
Woops, missed that.
> 
> > 
> >  	echo "run ${args[@]}"
> >  }
> >  
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > index 0503cf0..383ebd4 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -32,6 +32,26 @@ get_cmdline()
> >      echo "TESTNAME=$testname TIMEOUT=$timeout ACCEL=$accel
> > $RUNTIME_arch_run $kernel -smp $smp $opts"
> >  }
> >  
> > +skip_nodefault()
> > +{
> > +    [ "$STANDALONE" != yes ] && return 0
> For consistency throughout our scripts, please quote all strings,
> like "yes"
> 
> > 
> > +
> > +    while true; do
> > +        read -p "Test marked not to be run by default, are you
> > sure (Y/N)? " yn
> > +        case $yn in
> > +            "Y" | "y" | "Yes" | "yes")
> What about "YES" :-)
> 
> Actually, I'd just accept 'Y' for yes, and nothing else, like the
> prompt says.
> And, instead of looping for valid input, all other input can just
> mean no.
> 
> > 
> > +                return 1
> > +                ;;
> > +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> We should output something when the answer is 'no' like "User
> aborted",
> or whatever.
> 
> > 
> > +                exit
> > +                ;;
> > +            *)
> > +                echo Please select Y or N
> > +                ;;
> > +        esac
> > +    done
> > +}
> > +
> >  function run()
> >  {
> >      local testname="$1"
> > @@ -48,10 +68,16 @@ function run()
> >          return
> >      fi
> >  
> > -    if [ -n "$only_group" ] && ! grep -q "$only_group" <<<$groups;
> > then
> > +    if [ -n "$only_group" ] && ! grep -qw "$only_group"
> > <<<$groups; then
> >          return
> >      fi
> >  
> > +    if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> > +            skip_nodefault; then
> > +        echo -e "`SKIP` $testname (test marked as manual run
> > only)"
> > +        return;
> > +    fi
> > +
> >      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
> >          echo "`SKIP` $1 ($arch only)"
> >          return 2
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 60747cf..4a1f74e 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 18:03 +0200, Radim Krčmář wrote:
> 2016-08-16 14:00+0200, Andrew Jones:
> > 
> > On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh
> > wrote:
> > > 
> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > @@ -32,6 +32,26 @@ get_cmdline()
> > > +skip_nodefault()
> > > +{
> > > +    while true; do
> > > +        read -p "Test marked not to be run by default, are you
> > > sure (Y/N)? " yn
> I'd write "run this test" instead of "are you sure", or something
> similar for the question.  The user can be sure that the has is
> marked
> with nodefault. ;)
I'll go with "run this test?"
> 
> > 
> > > 
> > > +        case $yn in
> > > +            "Y" | "y" | "Yes" | "yes")
> > What about "YES" :-)
> And exclamation marks!
> 
> "YES!!!"
> 
> > 
> > Actually, I'd just accept 'Y' for yes, and nothing else, like the
> > prompt says.
> NO.  If it is only one value, then make it "y".
> motto: saving the Earth, one shift at a time.
> 
> This kind of user interface usually accepts at least "[yY]|[yY]es"
> ...
> users will already be pissed that they have to input something and
> denying a perfectly logical "yes" (which is what "y" stands for) is
> going too overboard, IMO.
> 
> > 
> > And, instead of looping for valid input, all other input can just
> > mean no.
> "y/N" is the convention for writing a bool question that defaults to
> no.
> I'd accept "" (just enter) as the default and then, looping isn't
> unexpected and user already typed some crap in that case, so they
> probably want to answer the question without having to run the
> command
> again.
I'd rather keep the "[yY]|[yY]es" options it doesn't really make sense
not to.
I can see the argument for having all other input default to no. You
really see a mix of the two in the wild where sometimes junk will
default to no and sometimes it will loop and ask again.
I tend to think that most of the time when it doesn't match one of the
options it's going to be because of a mis-type by the user and they'd
probably prefer that it looped than have to run the command again.
> 
> > 
> > > 
> > > +                return 1
> > > +                ;;
> > > +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> > We should output something when the answer is 'no' like "User
> > aborted",
> > or whatever.
I don't really see this as being necessary, I guess there could be some
ambiguity as to what has happened but I think it's pretty obvious that
the test has aborted.
I'll change it so that the skip message is printed.
> > 
> > > 
> > > +                exit
> Wouldn't "return 1" and the SKIP message be enough?
Yeah if I have return 0 here (because bash things) then it'll print the
skip message and abort the test.
> 
> > 
> > > 
> > > +                ;;
> > > +            *)
> > > +                echo Please select Y or N
> Just asking the question again is more common -- it's not hard to
> figure
> out that the answer was not accepted.
Ok, I've seen places where the options are specified like this but will
change it to just have the same question again.

Re: [kvm-unit-tests PATCH V3 1/5] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 18:03 +0200, Radim Krčmář wrote:
> 2016-08-16 14:00+0200, Andrew Jones:
> > 
> > On Tue, Aug 16, 2016 at 05:18:11PM +1000, Suraj Jitindar Singh
> > wrote:
> > > 
> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > @@ -32,6 +32,26 @@ get_cmdline()
> > > +skip_nodefault()
> > > +{
> > > +    while true; do
> > > +        read -p "Test marked not to be run by default, are you
> > > sure (Y/N)? " yn
> I'd write "run this test" instead of "are you sure", or something
> similar for the question.  The user can be sure that the has is
> marked
> with nodefault. ;)
I'll go with "run this test?"
> 
> > 
> > > 
> > > +        case $yn in
> > > +            "Y" | "y" | "Yes" | "yes")
> > What about "YES" :-)
> And exclamation marks!
> 
> "YES!!!"
> 
> > 
> > Actually, I'd just accept 'Y' for yes, and nothing else, like the
> > prompt says.
> NO.  If it is only one value, then make it "y".
> motto: saving the Earth, one shift at a time.
> 
> This kind of user interface usually accepts at least "[yY]|[yY]es"
> ...
> users will already be pissed that they have to input something and
> denying a perfectly logical "yes" (which is what "y" stands for) is
> going too overboard, IMO.
> 
> > 
> > And, instead of looping for valid input, all other input can just
> > mean no.
> "y/N" is the convention for writing a bool question that defaults to
> no.
> I'd accept "" (just enter) as the default and then, looping isn't
> unexpected and user already typed some crap in that case, so they
> probably want to answer the question without having to run the
> command
> again.
I'd rather keep the "[yY]|[yY]es" options it doesn't really make sense
not to.
I can see the argument for having all other input default to no. You
really see a mix of the two in the wild where sometimes junk will
default to no and sometimes it will loop and ask again.
I tend to think that most of the time when it doesn't match one of the
options it's going to be because of a mis-type by the user and they'd
probably prefer that it looped than have to run the command again.
> 
> > 
> > > 
> > > +                return 1
> > > +                ;;
> > > +            "N" | "n" | "No" | "no" | "q" | "quit" | "exit")
> > We should output something when the answer is 'no' like "User
> > aborted",
> > or whatever.
I don't really see this as being necessary, I guess there could be some
ambiguity as to what has happened but I think it's pretty obvious that
the test has aborted.
I'll change it so that the skip message is printed.
> > 
> > > 
> > > +                exit
> Wouldn't "return 1" and the SKIP message be enough?
Yeah if I have return 0 here (because bash things) then it'll print the
skip message and abort the test.
> 
> > 
> > > 
> > > +                ;;
> > > +            *)
> > > +                echo Please select Y or N
> Just asking the question again is more common -- it's not hard to
> figure
> out that the answer was not accepted.
Ok, I've seen places where the options are specified like this but will
change it to just have the same question again.

Re: [kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:27 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:13PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Add the lib/powerpc/smp.c file and associated header files as a
> > place
> > to implement generic smp functionality for inclusion in tests.
> > 
> > Add functions start_all_cpus(), start_cpu() and start_thread() to
> > start
> > all stopped threads of all cpus, all stopped threads of a single
> > cpu or a
> > single stopped thread of a guest at a given execution location,
> > respectively.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- start_thread now returns int to reflect error, success or
> > failure to
> > 	  start thread
> > 	- start_cpu returns number of threads on cpu and number
> > successfully
> > 	  started
> > 	- start_all_cpus checks if number of threads started == total
> > number of
> > 	  threads - 1
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/smp.h   |  22 ++++++++++
> >  lib/powerpc/smp.c       | 106
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ppc64/asm/smp.h     |   1 +
> >  powerpc/Makefile.common |   1 +
> >  4 files changed, 130 insertions(+)
> >  create mode 100644 lib/powerpc/asm/smp.h
> >  create mode 100644 lib/powerpc/smp.c
> >  create mode 100644 lib/ppc64/asm/smp.h
> > 
> > diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
> > new file mode 100644
> > index 0000000..21940b4
> > --- /dev/null
> > +++ b/lib/powerpc/asm/smp.h
> > @@ -0,0 +1,22 @@
> > +#ifndef _ASMPOWERPC_SMP_H_
> > +#define _ASMPOWERPC_SMP_H_
> > +
> > +#include <libcflat.h>
> > +
> > +extern int nr_threads;
> > +
> > +struct start_threads {
> > +	int nr_threads;
> > +	int nr_started;
> > +};
> > +
> > +typedef void (*secondary_entry_fn)(void);
> > +
> > +extern void halt(void);
> > +
> > +extern int start_thread(int cpu_id, secondary_entry_fn entry,
> > uint32_t r3);
> > +extern struct start_threads start_cpu(int cpu_node,
> > secondary_entry_fn entry,
> > +				      uint32_t r3);
> > +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> > +
> > +#endif /* _ASMPOWERPC_SMP_H_ */
> > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > new file mode 100644
> > index 0000000..48c636e
> > --- /dev/null
> > +++ b/lib/powerpc/smp.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Secondary cpu support
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +
> > +#include <devicetree.h>
> > +#include <asm/setup.h>
> > +#include <asm/rtas.h>
> > +#include <asm/smp.h>
> > +
> > +int nr_threads;
> > +
> > +struct secondary_entry_data {
> > +	secondary_entry_fn entry;
> > +	uint64_t r3;
> > +	int nr_started;
> > +};
> > +
> > +/*
> > + * Start stopped thread cpu_id at entry
> > + * Returns:	<0 on failure to start stopped cpu
> > + *		0  on success
> > + *		>0 on cpu not in stopped state
> > + */
> > +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t
> > r3)
> > +{
> > +	int query_token, start_token, outputs[1], ret;
> > +
> > +	query_token = rtas_token("query-cpu-stopped-state");
> > +	start_token = rtas_token("start-cpu");
> > +	assert(query_token != RTAS_UNKNOWN_SERVICE &&
> > +			start_token != RTAS_UNKNOWN_SERVICE);
> > +
> > +	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> > +	if (ret) /* rtas query call failed */
> > +		printf("query-cpu-stopped-state failed for cpu
> > %d\n", cpu_id);
> > +	else if (!outputs[0]) { /* cpu in stopped state */
> > +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> > entry, r3);
> > +		if (ret) /* rtas start-cpu call failed */
> > +			printf("failed to start cpu %d\n",
> > cpu_id);
> > +	} else /* cpu not in stopped state */
> > +		ret = outputs[0];
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Start all stopped threads (vcpus) on cpu_node
> > + * Returns: Number of stopped cpus which were successfully started
> > + */
> > +struct start_threads start_cpu(int cpu_node, secondary_entry_fn
> > entry,
> > +			       uint32_t r3)
> > +{
> > +	int len, cpu, nr_threads, nr_started = 0;
> > +	const struct fdt_property *prop;
> > +	u32 *cpus;
> The mixing of the terms cpus and threads in the function is
> confusing.
> Let's change the variables like this
> 
> /cpus/threads/
> /cpu/i/
Sounds good
> 
> > 
> > +
> > +	/* Get the id array of threads on this cpu_node */
> > +	prop = fdt_get_property(dt_fdt(), cpu_node,
> > +			"ibm,ppc-interrupt-server#s", &len);
> > +	assert(prop);
> > +
> > +	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per
> > cpu */
> /cpu/thread/ in comment
ok
> 
> > 
> > +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> /cpu/thread/ in comment
> 
> > 
> > +
> > +	for (cpu = 0; cpu < nr_threads; cpu++) {
> > +		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > r3))
> > +			nr_started++;
> > +	}
> > +
> > +	return (struct start_threads) {nr_threads, nr_started};
> > +}
> > +
> > +static void start_each_secondary(int fdtnode, u32 regval __unused,
> > void *info)
> > +{
> > +	struct secondary_entry_data *datap = info;
> > +	struct start_threads ret = start_cpu(fdtnode, datap-
> > >entry, datap->r3);
> > +
> > +	nr_threads += ret.nr_threads;
> > +	datap->nr_started += ret.nr_started;
> > +}
> > +
> > +/*
> > + * Start all stopped cpus on the guest at entry with register 3
> > set to r3
> > + * We expect that we come in with only one thread currently
> > started
> > + * Returns:	TRUE on success
> > + *		FALSE on failure
> > + */
> > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > +{
> > +	struct secondary_entry_data data = {
> > +		entry,
> > +		r3,
> > +		0
> > +	};
> nit: no need for all the white space, can just do = { entry, r3, 0 };
Ok
> 
> > 
> > +	int ret;
> > +
> > +	ret = dt_for_each_cpu_node(&start_each_secondary, &data);
> nit: no need for '&' on function pointer.
I like having it
> 
> > 
> > +	assert(ret == 0);
> > +
> > +	/* We expect that we come in with one thread already
> > started */
> > +	return data.nr_started == (nr_threads - 1);
> nit: no need for the ()
> 
> You could also init data.nr_started to 1 above, and then avoid
> the -1 here, but whatever.
But then nr_started wouldn't actually represent the number of threads
that were started, it'd be ++ that. I'll keep the -1.
> 
> > 
> > +}
> > diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
> > new file mode 100644
> > index 0000000..67ced75
> > --- /dev/null
> > +++ b/lib/ppc64/asm/smp.h
> > @@ -0,0 +1 @@
> > +#include "../../powerpc/asm/smp.h"
> > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> > index 404194b..677030a 100644
> > --- a/powerpc/Makefile.common
> > +++ b/powerpc/Makefile.common
> > @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
> >  cflatobjs += lib/powerpc/rtas.o
> >  cflatobjs += lib/powerpc/processor.o
> >  cflatobjs += lib/powerpc/handlers.o
> > +cflatobjs += lib/powerpc/smp.o
> >  
> >  FLATLIBS = $(libcflat) $(LIBFDT_archive)
> >  %.elf: CFLAGS += $(arch_CFLAGS)

Re: [kvm-unit-tests PATCH V3 3/5] lib/powerpc: Add function to start secondary threads

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:27 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:13PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Add the lib/powerpc/smp.c file and associated header files as a
> > place
> > to implement generic smp functionality for inclusion in tests.
> > 
> > Add functions start_all_cpus(), start_cpu() and start_thread() to
> > start
> > all stopped threads of all cpus, all stopped threads of a single
> > cpu or a
> > single stopped thread of a guest at a given execution location,
> > respectively.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- start_thread now returns int to reflect error, success or
> > failure to
> > 	  start thread
> > 	- start_cpu returns number of threads on cpu and number
> > successfully
> > 	  started
> > 	- start_all_cpus checks if number of threads started = total
> > number of
> > 	  threads - 1
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/smp.h   |  22 ++++++++++
> >  lib/powerpc/smp.c       | 106
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ppc64/asm/smp.h     |   1 +
> >  powerpc/Makefile.common |   1 +
> >  4 files changed, 130 insertions(+)
> >  create mode 100644 lib/powerpc/asm/smp.h
> >  create mode 100644 lib/powerpc/smp.c
> >  create mode 100644 lib/ppc64/asm/smp.h
> > 
> > diff --git a/lib/powerpc/asm/smp.h b/lib/powerpc/asm/smp.h
> > new file mode 100644
> > index 0000000..21940b4
> > --- /dev/null
> > +++ b/lib/powerpc/asm/smp.h
> > @@ -0,0 +1,22 @@
> > +#ifndef _ASMPOWERPC_SMP_H_
> > +#define _ASMPOWERPC_SMP_H_
> > +
> > +#include <libcflat.h>
> > +
> > +extern int nr_threads;
> > +
> > +struct start_threads {
> > +	int nr_threads;
> > +	int nr_started;
> > +};
> > +
> > +typedef void (*secondary_entry_fn)(void);
> > +
> > +extern void halt(void);
> > +
> > +extern int start_thread(int cpu_id, secondary_entry_fn entry,
> > uint32_t r3);
> > +extern struct start_threads start_cpu(int cpu_node,
> > secondary_entry_fn entry,
> > +				      uint32_t r3);
> > +extern bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> > +
> > +#endif /* _ASMPOWERPC_SMP_H_ */
> > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > new file mode 100644
> > index 0000000..48c636e
> > --- /dev/null
> > +++ b/lib/powerpc/smp.c
> > @@ -0,0 +1,106 @@
> > +/*
> > + * Secondary cpu support
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +
> > +#include <devicetree.h>
> > +#include <asm/setup.h>
> > +#include <asm/rtas.h>
> > +#include <asm/smp.h>
> > +
> > +int nr_threads;
> > +
> > +struct secondary_entry_data {
> > +	secondary_entry_fn entry;
> > +	uint64_t r3;
> > +	int nr_started;
> > +};
> > +
> > +/*
> > + * Start stopped thread cpu_id at entry
> > + * Returns:	<0 on failure to start stopped cpu
> > + *		0  on success
> > + *		>0 on cpu not in stopped state
> > + */
> > +int start_thread(int cpu_id, secondary_entry_fn entry, uint32_t
> > r3)
> > +{
> > +	int query_token, start_token, outputs[1], ret;
> > +
> > +	query_token = rtas_token("query-cpu-stopped-state");
> > +	start_token = rtas_token("start-cpu");
> > +	assert(query_token != RTAS_UNKNOWN_SERVICE &&
> > +			start_token != RTAS_UNKNOWN_SERVICE);
> > +
> > +	ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> > +	if (ret) /* rtas query call failed */
> > +		printf("query-cpu-stopped-state failed for cpu
> > %d\n", cpu_id);
> > +	else if (!outputs[0]) { /* cpu in stopped state */
> > +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> > entry, r3);
> > +		if (ret) /* rtas start-cpu call failed */
> > +			printf("failed to start cpu %d\n",
> > cpu_id);
> > +	} else /* cpu not in stopped state */
> > +		ret = outputs[0];
> > +
> > +	return ret;
> > +}
> > +
> > +/*
> > + * Start all stopped threads (vcpus) on cpu_node
> > + * Returns: Number of stopped cpus which were successfully started
> > + */
> > +struct start_threads start_cpu(int cpu_node, secondary_entry_fn
> > entry,
> > +			       uint32_t r3)
> > +{
> > +	int len, cpu, nr_threads, nr_started = 0;
> > +	const struct fdt_property *prop;
> > +	u32 *cpus;
> The mixing of the terms cpus and threads in the function is
> confusing.
> Let's change the variables like this
> 
> /cpus/threads/
> /cpu/i/
Sounds good
> 
> > 
> > +
> > +	/* Get the id array of threads on this cpu_node */
> > +	prop = fdt_get_property(dt_fdt(), cpu_node,
> > +			"ibm,ppc-interrupt-server#s", &len);
> > +	assert(prop);
> > +
> > +	nr_threads = len >> 2; /* Divide by 4 since 4 bytes per
> > cpu */
> /cpu/thread/ in comment
ok
> 
> > 
> > +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> /cpu/thread/ in comment
> 
> > 
> > +
> > +	for (cpu = 0; cpu < nr_threads; cpu++) {
> > +		if (!start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > r3))
> > +			nr_started++;
> > +	}
> > +
> > +	return (struct start_threads) {nr_threads, nr_started};
> > +}
> > +
> > +static void start_each_secondary(int fdtnode, u32 regval __unused,
> > void *info)
> > +{
> > +	struct secondary_entry_data *datap = info;
> > +	struct start_threads ret = start_cpu(fdtnode, datap-
> > >entry, datap->r3);
> > +
> > +	nr_threads += ret.nr_threads;
> > +	datap->nr_started += ret.nr_started;
> > +}
> > +
> > +/*
> > + * Start all stopped cpus on the guest at entry with register 3
> > set to r3
> > + * We expect that we come in with only one thread currently
> > started
> > + * Returns:	TRUE on success
> > + *		FALSE on failure
> > + */
> > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > +{
> > +	struct secondary_entry_data data = {
> > +		entry,
> > +		r3,
> > +		0
> > +	};
> nit: no need for all the white space, can just do = { entry, r3, 0 };
Ok
> 
> > 
> > +	int ret;
> > +
> > +	ret = dt_for_each_cpu_node(&start_each_secondary, &data);
> nit: no need for '&' on function pointer.
I like having it
> 
> > 
> > +	assert(ret = 0);
> > +
> > +	/* We expect that we come in with one thread already
> > started */
> > +	return data.nr_started = (nr_threads - 1);
> nit: no need for the ()
> 
> You could also init data.nr_started to 1 above, and then avoid
> the -1 here, but whatever.
But then nr_started wouldn't actually represent the number of threads
that were started, it'd be ++ that. I'll keep the -1.
> 
> > 
> > +}
> > diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
> > new file mode 100644
> > index 0000000..67ced75
> > --- /dev/null
> > +++ b/lib/ppc64/asm/smp.h
> > @@ -0,0 +1 @@
> > +#include "../../powerpc/asm/smp.h"
> > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> > index 404194b..677030a 100644
> > --- a/powerpc/Makefile.common
> > +++ b/powerpc/Makefile.common
> > @@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
> >  cflatobjs += lib/powerpc/rtas.o
> >  cflatobjs += lib/powerpc/processor.o
> >  cflatobjs += lib/powerpc/handlers.o
> > +cflatobjs += lib/powerpc/smp.o
> >  
> >  FLATLIBS = $(libcflat) $(LIBFDT_archive)
> >  %.elf: CFLAGS += $(arch_CFLAGS)

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:41 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:14PM +1000, Suraj Jitindar Singh wrote:
> > 
> > It would be nice if we had a generic sleep function which could be
> > used
> > in unit tests, add one.
> > 
> > Add the variable tb_hz used to store the time base frequency which
> > is read
> > from the device tree on setup.
> > 
> > Add functions msleep, usleep and sleep in processor.c to sleep for
> > a given
> > number of milliseconds, microseconds and time base ticks
> > respectively.
> Above you talk about adding *sleep functions, but below you add
> *delay
> functions.
Woops
> 
> > 
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Add patch to series
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
> >  lib/powerpc/asm/setup.h     |  2 ++
> >  lib/powerpc/processor.c     | 13 +++++++++++++
> >  lib/powerpc/setup.c         | 12 ++++++++++++
> >  4 files changed, 46 insertions(+)
> > 
> > diff --git a/lib/powerpc/asm/processor.h
> > b/lib/powerpc/asm/processor.h
> > index 09692bd..9554e08 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -1,11 +1,30 @@
> >  #ifndef _ASMPOWERPC_PROCESSOR_H_
> >  #define _ASMPOWERPC_PROCESSOR_H_
> >  
> > +#include <libcflat.h>
> >  #include <asm/ptrace.h>
> >  
> > +#define cpu_relax() asm volatile ("" : : : "memory")
> Please add this to lib/asm-generic/barrier.h, then
> lib/ppc64/asm/barrier.h
> will pick it up from there and you'll just need to make sure you
> include
> asm/barrier.h in processor.c
Will do
> 
> > 
> > +
> >  #ifndef __ASSEMBLY__
> >  void handle_exception(int trap, void (*func)(struct pt_regs *,
> > void *), void *);
> >  void do_handle_exception(struct pt_regs *regs);
> >  #endif /* __ASSEMBLY__ */
> >  
> > +static inline uint64_t get_tb(void)
> > +{
> > +	uint64_t tb;
> > +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> > +	return tb;
> > +}
> > +
> > +extern void delay(uint64_t cycles);
> > +extern void udelay(uint64_t us);
> > +
> > +static inline void mdelay(uint64_t ms)
> > +{
> > +	while (ms--)
> > +		udelay(1000);
> > +}
> > +
> >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> > index b1e1e5a..23b4156 100644
> > --- a/lib/powerpc/asm/setup.h
> > +++ b/lib/powerpc/asm/setup.h
> > @@ -11,6 +11,8 @@
> >  extern u32 cpus[NR_CPUS];
> >  extern int nr_cpus;
> >  
> > +extern uint64_t tb_hz;
> > +
> >  #define NR_MEM_REGIONS		8
> >  #define MR_F_PRIMARY		(1U << 0)
> >  struct mem_region {
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index a78bc3c..c84bae5 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -5,6 +5,7 @@
> >  #include <libcflat.h>
> >  #include <asm/processor.h>
> >  #include <asm/ptrace.h>
> > +#include <asm/setup.h>
> >  
> >  static struct {
> >  	void (*func)(struct pt_regs *, void *data);
> > @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
> >  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
> >  	abort();
> >  }
> > +
> > +void delay(uint64_t cycles)
> > +{
> > +	uint64_t start = get_tb();
> > +	while((get_tb() - start) < cycles)
> > +		cpu_relax();
> > +}
> > +
> > +void udelay(uint64_t us)
> > +{
> > +	delay((us * tb_hz) / 1000000);
> > +}
> > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> > index e3d2afa..e7fa525 100644
> > --- a/lib/powerpc/setup.c
> > +++ b/lib/powerpc/setup.c
> > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char
> > *args);
> >  
> >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
> >  int nr_cpus;
> > +uint64_t tb_hz;
> >  
> >  struct mem_region mem_regions[NR_MEM_REGIONS];
> >  phys_addr_t __physical_start, __physical_end;
> > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval,
> > void *info)
> >  	}
> >  	cpus[cpu] = regval;
> >  
> > +	/* set timebase frequency */
> > +	if (!tb_hz) {
> Instead of this condition, can't we tuck this DT parsing into the
> if (!read_common_info) block we already have?
Didn't read that properly to realise that block is only called once,
I'll move it into there.
> 
> > 
> > +		const struct fdt_property *prop;
> > +		u32 *data;
> > +		prop = fdt_get_property(dt_fdt(), fdtnode,
> > +					"timebase-frequency",
> > NULL);
> > +		assert(prop != NULL);
> > +		data = (u32 *)prop->data;
> > +		tb_hz = fdt32_to_cpu(*data);
> > +	}
> > +
> >  	/* set exception stack address for this CPU (in SPGR0) */
> >  
> >  	asm volatile ("mtsprg0 %[addr]" ::

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit test

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:41 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:14PM +1000, Suraj Jitindar Singh wrote:
> > 
> > It would be nice if we had a generic sleep function which could be
> > used
> > in unit tests, add one.
> > 
> > Add the variable tb_hz used to store the time base frequency which
> > is read
> > from the device tree on setup.
> > 
> > Add functions msleep, usleep and sleep in processor.c to sleep for
> > a given
> > number of milliseconds, microseconds and time base ticks
> > respectively.
> Above you talk about adding *sleep functions, but below you add
> *delay
> functions.
Woops
> 
> > 
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Add patch to series
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
> >  lib/powerpc/asm/setup.h     |  2 ++
> >  lib/powerpc/processor.c     | 13 +++++++++++++
> >  lib/powerpc/setup.c         | 12 ++++++++++++
> >  4 files changed, 46 insertions(+)
> > 
> > diff --git a/lib/powerpc/asm/processor.h
> > b/lib/powerpc/asm/processor.h
> > index 09692bd..9554e08 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -1,11 +1,30 @@
> >  #ifndef _ASMPOWERPC_PROCESSOR_H_
> >  #define _ASMPOWERPC_PROCESSOR_H_
> >  
> > +#include <libcflat.h>
> >  #include <asm/ptrace.h>
> >  
> > +#define cpu_relax() asm volatile ("" : : : "memory")
> Please add this to lib/asm-generic/barrier.h, then
> lib/ppc64/asm/barrier.h
> will pick it up from there and you'll just need to make sure you
> include
> asm/barrier.h in processor.c
Will do
> 
> > 
> > +
> >  #ifndef __ASSEMBLY__
> >  void handle_exception(int trap, void (*func)(struct pt_regs *,
> > void *), void *);
> >  void do_handle_exception(struct pt_regs *regs);
> >  #endif /* __ASSEMBLY__ */
> >  
> > +static inline uint64_t get_tb(void)
> > +{
> > +	uint64_t tb;
> > +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> > +	return tb;
> > +}
> > +
> > +extern void delay(uint64_t cycles);
> > +extern void udelay(uint64_t us);
> > +
> > +static inline void mdelay(uint64_t ms)
> > +{
> > +	while (ms--)
> > +		udelay(1000);
> > +}
> > +
> >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> > index b1e1e5a..23b4156 100644
> > --- a/lib/powerpc/asm/setup.h
> > +++ b/lib/powerpc/asm/setup.h
> > @@ -11,6 +11,8 @@
> >  extern u32 cpus[NR_CPUS];
> >  extern int nr_cpus;
> >  
> > +extern uint64_t tb_hz;
> > +
> >  #define NR_MEM_REGIONS		8
> >  #define MR_F_PRIMARY		(1U << 0)
> >  struct mem_region {
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index a78bc3c..c84bae5 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -5,6 +5,7 @@
> >  #include <libcflat.h>
> >  #include <asm/processor.h>
> >  #include <asm/ptrace.h>
> > +#include <asm/setup.h>
> >  
> >  static struct {
> >  	void (*func)(struct pt_regs *, void *data);
> > @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
> >  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
> >  	abort();
> >  }
> > +
> > +void delay(uint64_t cycles)
> > +{
> > +	uint64_t start = get_tb();
> > +	while((get_tb() - start) < cycles)
> > +		cpu_relax();
> > +}
> > +
> > +void udelay(uint64_t us)
> > +{
> > +	delay((us * tb_hz) / 1000000);
> > +}
> > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> > index e3d2afa..e7fa525 100644
> > --- a/lib/powerpc/setup.c
> > +++ b/lib/powerpc/setup.c
> > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char
> > *args);
> >  
> >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
> >  int nr_cpus;
> > +uint64_t tb_hz;
> >  
> >  struct mem_region mem_regions[NR_MEM_REGIONS];
> >  phys_addr_t __physical_start, __physical_end;
> > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval,
> > void *info)
> >  	}
> >  	cpus[cpu] = regval;
> >  
> > +	/* set timebase frequency */
> > +	if (!tb_hz) {
> Instead of this condition, can't we tuck this DT parsing into the
> if (!read_common_info) block we already have?
Didn't read that properly to realise that block is only called once,
I'll move it into there.
> 
> > 
> > +		const struct fdt_property *prop;
> > +		u32 *data;
> > +		prop = fdt_get_property(dt_fdt(), fdtnode,
> > +					"timebase-frequency",
> > NULL);
> > +		assert(prop != NULL);
> > +		data = (u32 *)prop->data;
> > +		tb_hz = fdt32_to_cpu(*data);
> > +	}
> > +
> >  	/* set exception stack address for this CPU (in SPGR0) */
> >  
> >  	asm volatile ("mtsprg0 %[addr]" ::

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:54 +0200, Thomas Huth wrote:
> On 16.08.2016 09:18, Suraj Jitindar Singh wrote:
> > 
> > It would be nice if we had a generic sleep function which could be
> > used
> > in unit tests, add one.
> > 
> > Add the variable tb_hz used to store the time base frequency which
> > is read
> > from the device tree on setup.
> > 
> > Add functions msleep, usleep and sleep in processor.c to sleep for
> > a given
> > number of milliseconds, microseconds and time base ticks
> > respectively.
> You finally called them mdelay, udelay and delay instead, so in case
> you
> respin, please adjust the commit message.
Woops, I'll fix those
> 
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Add patch to series
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
> >  lib/powerpc/asm/setup.h     |  2 ++
> >  lib/powerpc/processor.c     | 13 +++++++++++++
> >  lib/powerpc/setup.c         | 12 ++++++++++++
> >  4 files changed, 46 insertions(+)
> > 
> > diff --git a/lib/powerpc/asm/processor.h
> > b/lib/powerpc/asm/processor.h
> > index 09692bd..9554e08 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -1,11 +1,30 @@
> >  #ifndef _ASMPOWERPC_PROCESSOR_H_
> >  #define _ASMPOWERPC_PROCESSOR_H_
> >  
> > +#include <libcflat.h>
> >  #include <asm/ptrace.h>
> >  
> > +#define cpu_relax() asm volatile ("" : : : "memory")
> I'd maybe add a "nop" instruction in there ... just in case?
We don't put the nop in anywhere else or in the kernel. I'm gonna leave
this.
> 
> > 
> >  #ifndef __ASSEMBLY__
> >  void handle_exception(int trap, void (*func)(struct pt_regs *,
> > void *), void *);
> >  void do_handle_exception(struct pt_regs *regs);
> >  #endif /* __ASSEMBLY__ */
> >  
> > +static inline uint64_t get_tb(void)
> > +{
> > +	uint64_t tb;
> > +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> > +	return tb;
> > +}
> > +
> > +extern void delay(uint64_t cycles);
> > +extern void udelay(uint64_t us);
> > +
> > +static inline void mdelay(uint64_t ms)
> > +{
> > +	while (ms--)
> > +		udelay(1000);
> > +}
> > +
> >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> > index b1e1e5a..23b4156 100644
> > --- a/lib/powerpc/asm/setup.h
> > +++ b/lib/powerpc/asm/setup.h
> > @@ -11,6 +11,8 @@
> >  extern u32 cpus[NR_CPUS];
> >  extern int nr_cpus;
> >  
> > +extern uint64_t tb_hz;
> > +
> >  #define NR_MEM_REGIONS		8
> >  #define MR_F_PRIMARY		(1U << 0)
> >  struct mem_region {
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index a78bc3c..c84bae5 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -5,6 +5,7 @@
> >  #include <libcflat.h>
> >  #include <asm/processor.h>
> >  #include <asm/ptrace.h>
> > +#include <asm/setup.h>
> >  
> >  static struct {
> >  	void (*func)(struct pt_regs *, void *data);
> > @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
> >  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
> >  	abort();
> >  }
> > +
> > +void delay(uint64_t cycles)
> > +{
> > +	uint64_t start = get_tb();
> > +	while((get_tb() - start) < cycles)
> > +		cpu_relax();
> Shouldn't there be some kind of wrap-around detection, like you had
> it
> in v2 for the decrementer based sleep function?
Thanks for catching that, highly unlikely this will overflow (64-bit
for timebase vs 31-bit for decrementer) but probably worth adding a
check to be sure.
> 
> > 
> > +}
> > +
> > +void udelay(uint64_t us)
> > +{
> > +	delay((us * tb_hz) / 1000000);
> > +}
> > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> > index e3d2afa..e7fa525 100644
> > --- a/lib/powerpc/setup.c
> > +++ b/lib/powerpc/setup.c
> > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char
> > *args);
> >  
> >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
> >  int nr_cpus;
> > +uint64_t tb_hz;
> >  
> >  struct mem_region mem_regions[NR_MEM_REGIONS];
> >  phys_addr_t __physical_start, __physical_end;
> > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval,
> > void *info)
> >  	}
> >  	cpus[cpu] = regval;
> >  
> > +	/* set timebase frequency */
> > +	if (!tb_hz) {
> > +		const struct fdt_property *prop;
> > +		u32 *data;
> > +		prop = fdt_get_property(dt_fdt(), fdtnode,
> > +					"timebase-frequency",
> > NULL);
> > +		assert(prop != NULL);
> > +		data = (u32 *)prop->data;
> > +		tb_hz = fdt32_to_cpu(*data);
> You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get
> rid
> of the "data" variable.
Moved this into the code block below were those are both already
defined.
> 
> > 
> > +	}
> > +
> >  	/* set exception stack address for this CPU (in SPGR0) */
> >  
> >  	asm volatile ("mtsprg0 %[addr]" ::
> > 
>  Thomas
> 

Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit test

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:54 +0200, Thomas Huth wrote:
> On 16.08.2016 09:18, Suraj Jitindar Singh wrote:
> > 
> > It would be nice if we had a generic sleep function which could be
> > used
> > in unit tests, add one.
> > 
> > Add the variable tb_hz used to store the time base frequency which
> > is read
> > from the device tree on setup.
> > 
> > Add functions msleep, usleep and sleep in processor.c to sleep for
> > a given
> > number of milliseconds, microseconds and time base ticks
> > respectively.
> You finally called them mdelay, udelay and delay instead, so in case
> you
> respin, please adjust the commit message.
Woops, I'll fix those
> 
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Add patch to series
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
> >  lib/powerpc/asm/setup.h     |  2 ++
> >  lib/powerpc/processor.c     | 13 +++++++++++++
> >  lib/powerpc/setup.c         | 12 ++++++++++++
> >  4 files changed, 46 insertions(+)
> > 
> > diff --git a/lib/powerpc/asm/processor.h
> > b/lib/powerpc/asm/processor.h
> > index 09692bd..9554e08 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -1,11 +1,30 @@
> >  #ifndef _ASMPOWERPC_PROCESSOR_H_
> >  #define _ASMPOWERPC_PROCESSOR_H_
> >  
> > +#include <libcflat.h>
> >  #include <asm/ptrace.h>
> >  
> > +#define cpu_relax() asm volatile ("" : : : "memory")
> I'd maybe add a "nop" instruction in there ... just in case?
We don't put the nop in anywhere else or in the kernel. I'm gonna leave
this.
> 
> > 
> >  #ifndef __ASSEMBLY__
> >  void handle_exception(int trap, void (*func)(struct pt_regs *,
> > void *), void *);
> >  void do_handle_exception(struct pt_regs *regs);
> >  #endif /* __ASSEMBLY__ */
> >  
> > +static inline uint64_t get_tb(void)
> > +{
> > +	uint64_t tb;
> > +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> > +	return tb;
> > +}
> > +
> > +extern void delay(uint64_t cycles);
> > +extern void udelay(uint64_t us);
> > +
> > +static inline void mdelay(uint64_t ms)
> > +{
> > +	while (ms--)
> > +		udelay(1000);
> > +}
> > +
> >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> > index b1e1e5a..23b4156 100644
> > --- a/lib/powerpc/asm/setup.h
> > +++ b/lib/powerpc/asm/setup.h
> > @@ -11,6 +11,8 @@
> >  extern u32 cpus[NR_CPUS];
> >  extern int nr_cpus;
> >  
> > +extern uint64_t tb_hz;
> > +
> >  #define NR_MEM_REGIONS		8
> >  #define MR_F_PRIMARY		(1U << 0)
> >  struct mem_region {
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index a78bc3c..c84bae5 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -5,6 +5,7 @@
> >  #include <libcflat.h>
> >  #include <asm/processor.h>
> >  #include <asm/ptrace.h>
> > +#include <asm/setup.h>
> >  
> >  static struct {
> >  	void (*func)(struct pt_regs *, void *data);
> > @@ -36,3 +37,15 @@ void do_handle_exception(struct pt_regs *regs)
> >  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
> >  	abort();
> >  }
> > +
> > +void delay(uint64_t cycles)
> > +{
> > +	uint64_t start = get_tb();
> > +	while((get_tb() - start) < cycles)
> > +		cpu_relax();
> Shouldn't there be some kind of wrap-around detection, like you had
> it
> in v2 for the decrementer based sleep function?
Thanks for catching that, highly unlikely this will overflow (64-bit
for timebase vs 31-bit for decrementer) but probably worth adding a
check to be sure.
> 
> > 
> > +}
> > +
> > +void udelay(uint64_t us)
> > +{
> > +	delay((us * tb_hz) / 1000000);
> > +}
> > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> > index e3d2afa..e7fa525 100644
> > --- a/lib/powerpc/setup.c
> > +++ b/lib/powerpc/setup.c
> > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char
> > *args);
> >  
> >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
> >  int nr_cpus;
> > +uint64_t tb_hz;
> >  
> >  struct mem_region mem_regions[NR_MEM_REGIONS];
> >  phys_addr_t __physical_start, __physical_end;
> > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval,
> > void *info)
> >  	}
> >  	cpus[cpu] = regval;
> >  
> > +	/* set timebase frequency */
> > +	if (!tb_hz) {
> > +		const struct fdt_property *prop;
> > +		u32 *data;
> > +		prop = fdt_get_property(dt_fdt(), fdtnode,
> > +					"timebase-frequency",
> > NULL);
> > +		assert(prop != NULL);
> > +		data = (u32 *)prop->data;
> > +		tb_hz = fdt32_to_cpu(*data);
> You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get
> rid
> of the "data" variable.
Moved this into the code block below were those are both already
defined.
> 
> > 
> > +	}
> > +
> >  	/* set exception stack address for this CPU (in SPGR0) */
> >  
> >  	asm volatile ("mtsprg0 %[addr]" ::
> > 
>  Thomas
> 

Re: [kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:57 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:15PM +1000, Suraj Jitindar Singh wrote:
> > 
> > On Power machines if a guest cedes while a tm transaction is in the
> > suspended state then the checkpointed state of the vcpu may be lost
> > and we
> > lose the cpu in the host.
> > 
> > Add a file for tm tests "powerpc/tm.c" and add a test to check if
> > the fix
> > has been applied to the host kernel. If this fix hasn't been
> > applied then
> > the test will never complete and the cpu will be lost. Otherwise
> > the test
> > should succeed. Since this has the ability to mess things up in the
> > host
> > mark this test as don't run by default.
> > 
> > This vulnerability has been assigned the ID CVE-2016-5412.
> > 
> > Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Remove get_dec() and sleep() functions as sleep functionality
> > is
> > 	  now provided by a generic implementation in processor.c.
> > 	- Replace TM instructions with raw machine code to support
> > older
> > 	  compilers
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/hcall.h |   1 +
> >  powerpc/Makefile.common |   3 +-
> >  powerpc/tm.c            | 121
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  powerpc/unittests.cfg   |   6 +++
> >  4 files changed, 130 insertions(+), 1 deletion(-)
> >  create mode 100644 powerpc/tm.c
> > 
> > diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> > index 99bce79..80aa3e3 100644
> > --- a/lib/powerpc/asm/hcall.h
> > +++ b/lib/powerpc/asm/hcall.h
> > @@ -18,6 +18,7 @@
> >  #define H_SET_SPRG0		0x24
> >  #define H_SET_DABR		0x28
> >  #define H_PAGE_INIT		0x2c
> > +#define H_CEDE			0xE0
> >  #define H_PUT_TERM_CHAR		0x58
> >  #define H_RANDOM		0x300
> >  #define H_SET_MODE		0x31C
> > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> > index 677030a..93e4f66 100644
> > --- a/powerpc/Makefile.common
> > +++ b/powerpc/Makefile.common
> > @@ -8,7 +8,8 @@ tests-common = \
> >  	$(TEST_DIR)/selftest.elf \
> >  	$(TEST_DIR)/spapr_hcall.elf \
> >  	$(TEST_DIR)/rtas.elf \
> > -	$(TEST_DIR)/emulator.elf
> > +	$(TEST_DIR)/emulator.elf \
> > +	$(TEST_DIR)/tm.elf
> >  
> >  all: $(TEST_DIR)/boot_rom.bin test_cases
> >  
> > diff --git a/powerpc/tm.c b/powerpc/tm.c
> > new file mode 100644
> > index 0000000..ba01f95
> > --- /dev/null
> > +++ b/powerpc/tm.c
> > @@ -0,0 +1,121 @@
> > +/*
> > + * Transactional Memory Unit Tests
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +#include <libcflat.h>
> > +#include <asm/hcall.h>
> > +#include <asm/processor.h>
> > +#include <asm/handlers.h>
> > +#include <asm/smp.h>
> > +
> > +static int h_cede(void)
> > +{
> > +	register uint64_t r3 asm("r3") = H_CEDE;
> > +
> > +	asm volatile ("sc 1" : "+r"(r3) :
> > +			     : "r0", "r4", "r5", "r6", "r7", "r8",
> > "r9",
> > +			       "r10", "r11", "r12", "xer", "ctr",
> > "cc");
> > +
> > +	return r3;
> > +}
> > +
> > +/*
> > + * Enable transactional memory
> > + * Returns:	FALSE - Failure
> > + *		TRUE - Success
> > + */
> > +static bool enable_tm(void)
> > +{
> > +	uint64_t msr = 0;
> > +
> > +	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
> > +
> > +	msr |= (((uint64_t) 1) << 32);
> > +
> > +	asm volatile ("mtmsrd %[msr]\n\t"
> > +		      "mfmsr %[msr]" : [msr] "+r" (msr));
> > +
> > +	return !!(msr & (((uint64_t) 1) << 32));
> > +}
> > +
> > +/*
> > + * Test H_CEDE call while transactional memory transaction is
> > suspended
> > + *
> > + * WARNING: This tests for a known vulnerability in which the host
> > may go down.
> > + * Probably best not to run this if your host going down is going
> > to cause
> > + * problems.
> > + *
> > + * If the test passes then your kernel probably has the necessary
> > patch.
> > + * If the test fails then the H_CEDE call was unsuccessful and the
> > + * vulnerability wasn't tested.
> > + * If the test hits the vulnerability then it will never complete
> > or report and
> > + * the qemu process will block indefinitely. RCU stalls will be
> > detected on the
> > + * cpu and any process scheduled on the lost cpu will also block
> > indefinitely.
> > + */
> > +static void test_h_cede_tm(int argc, char **argv)
> > +{
> > +	bool pass = true;
> > +	int i;
> > +
> > +	if (argc > 2)
> > +		report_abort("Unsupported argument: '%s'",
> > argv[2]);
> > +
> > +	handle_exception(0x900, &dec_except_handler, NULL);
> > +
> > +	if (!start_all_cpus(&halt, 0))
> nit: more unnecessary '&' on function pointers
Ok, guess I should change both
> 
> > 
> > +		report_abort("Failed to start secondary cpus");
> > +
> > +	if (!enable_tm())
> > +		report_abort("Failed to enable tm");
> > +
> > +	/*
> > +	 * Begin a transaction and guarantee we are in the suspend
> > state
> > +	 * before continuing
> > +	 */
> > +	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin.
> > */
> > +		      "beq 2f\n\t"
> > +		      ".long 0x7c0005dd\n\t"	/* tsuspend.
> > */
> > +		      "2: .long 0x7c00059c\n\t"	/* tcheck
> > cr0 */
> > +		      "bf 2,1b" : : : "cr0");
> > +
> > +	for (i = 0; i < 500 && pass; i++) {
> > +		uint64_t rval = h_cede();
> > +
> > +		if (rval != H_SUCCESS)
> > +			pass = false;
> nit: you could drop the '&& pass' and just do a break here.
Yes
> 
> > 
> > +		mdelay(5);
> > +	}
> > +
> > +	report("H_CEDE TM", pass);
> nit: with a break above you could just test for i == 500 here.
ok
> 
> > 
> > +}
> > +
> > +struct {
> > +	const char *name;
> > +	void (*func)(int argc, char **argv);
> > +} hctests[] = {
> > +	{ "h_cede_tm", test_h_cede_tm },
> > +	{ NULL, NULL }
> > +};
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	bool all;
> > +	int i;
> > +
> > +	report_prefix_push("tm");
> > +
> > +	all = argc == 1 || (argc == 2 && !strcmp(argv[1], "all"));
> I'd drop the argc == 2 test. You already know argc >= 2 at this
> point. And, if it's > 2, then you want to call the test function
> so the "Unsupported argument" abort will fire.
Sounds good
> 
> > 
> > +
> > +	for (i = 0; hctests[i].name != NULL; i++) {
> > +		if (all || strcmp(argv[1], hctests[i].name) == 0)
> > {
> > +			report_prefix_push(hctests[i].name);
> > +			hctests[i].func(argc, argv);
> > +			report_prefix_pop();
> > +		}
> > +	}
> > +
> > +	return report_summary();
> > +}
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index 0098cb6..20dbde6 100644
> > --- a/powerpc/unittests.cfg
> > +++ b/powerpc/unittests.cfg
> > @@ -53,3 +53,9 @@ groups = rtas
> >  
> >  [emulator]
> >  file = emulator.elf
> > +
> > +[h_cede_tm]
> > +file = tm.elf
> > +smp = 2,threads=2
> > +extra_params = -append "h_cede_tm"
> > +groups = nodefault,h_cede_tm

Re: [kvm-unit-tests PATCH V3 5/5] powerpc/tm: Add a test for H_CEDE while tm suspended

[Suraj Jitindar Singh]

On Tue, 2016-08-16 at 14:57 +0200, Andrew Jones wrote:
> On Tue, Aug 16, 2016 at 05:18:15PM +1000, Suraj Jitindar Singh wrote:
> > 
> > On Power machines if a guest cedes while a tm transaction is in the
> > suspended state then the checkpointed state of the vcpu may be lost
> > and we
> > lose the cpu in the host.
> > 
> > Add a file for tm tests "powerpc/tm.c" and add a test to check if
> > the fix
> > has been applied to the host kernel. If this fix hasn't been
> > applied then
> > the test will never complete and the cpu will be lost. Otherwise
> > the test
> > should succeed. Since this has the ability to mess things up in the
> > host
> > mark this test as don't run by default.
> > 
> > This vulnerability has been assigned the ID CVE-2016-5412.
> > 
> > Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Remove get_dec() and sleep() functions as sleep functionality
> > is
> > 	  now provided by a generic implementation in processor.c.
> > 	- Replace TM instructions with raw machine code to support
> > older
> > 	  compilers
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/hcall.h |   1 +
> >  powerpc/Makefile.common |   3 +-
> >  powerpc/tm.c            | 121
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  powerpc/unittests.cfg   |   6 +++
> >  4 files changed, 130 insertions(+), 1 deletion(-)
> >  create mode 100644 powerpc/tm.c
> > 
> > diff --git a/lib/powerpc/asm/hcall.h b/lib/powerpc/asm/hcall.h
> > index 99bce79..80aa3e3 100644
> > --- a/lib/powerpc/asm/hcall.h
> > +++ b/lib/powerpc/asm/hcall.h
> > @@ -18,6 +18,7 @@
> >  #define H_SET_SPRG0		0x24
> >  #define H_SET_DABR		0x28
> >  #define H_PAGE_INIT		0x2c
> > +#define H_CEDE			0xE0
> >  #define H_PUT_TERM_CHAR		0x58
> >  #define H_RANDOM		0x300
> >  #define H_SET_MODE		0x31C
> > diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> > index 677030a..93e4f66 100644
> > --- a/powerpc/Makefile.common
> > +++ b/powerpc/Makefile.common
> > @@ -8,7 +8,8 @@ tests-common = \
> >  	$(TEST_DIR)/selftest.elf \
> >  	$(TEST_DIR)/spapr_hcall.elf \
> >  	$(TEST_DIR)/rtas.elf \
> > -	$(TEST_DIR)/emulator.elf
> > +	$(TEST_DIR)/emulator.elf \
> > +	$(TEST_DIR)/tm.elf
> >  
> >  all: $(TEST_DIR)/boot_rom.bin test_cases
> >  
> > diff --git a/powerpc/tm.c b/powerpc/tm.c
> > new file mode 100644
> > index 0000000..ba01f95
> > --- /dev/null
> > +++ b/powerpc/tm.c
> > @@ -0,0 +1,121 @@
> > +/*
> > + * Transactional Memory Unit Tests
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +#include <libcflat.h>
> > +#include <asm/hcall.h>
> > +#include <asm/processor.h>
> > +#include <asm/handlers.h>
> > +#include <asm/smp.h>
> > +
> > +static int h_cede(void)
> > +{
> > +	register uint64_t r3 asm("r3") = H_CEDE;
> > +
> > +	asm volatile ("sc 1" : "+r"(r3) :
> > +			     : "r0", "r4", "r5", "r6", "r7", "r8",
> > "r9",
> > +			       "r10", "r11", "r12", "xer", "ctr",
> > "cc");
> > +
> > +	return r3;
> > +}
> > +
> > +/*
> > + * Enable transactional memory
> > + * Returns:	FALSE - Failure
> > + *		TRUE - Success
> > + */
> > +static bool enable_tm(void)
> > +{
> > +	uint64_t msr = 0;
> > +
> > +	asm volatile ("mfmsr %[msr]" : [msr] "=r" (msr));
> > +
> > +	msr |= (((uint64_t) 1) << 32);
> > +
> > +	asm volatile ("mtmsrd %[msr]\n\t"
> > +		      "mfmsr %[msr]" : [msr] "+r" (msr));
> > +
> > +	return !!(msr & (((uint64_t) 1) << 32));
> > +}
> > +
> > +/*
> > + * Test H_CEDE call while transactional memory transaction is
> > suspended
> > + *
> > + * WARNING: This tests for a known vulnerability in which the host
> > may go down.
> > + * Probably best not to run this if your host going down is going
> > to cause
> > + * problems.
> > + *
> > + * If the test passes then your kernel probably has the necessary
> > patch.
> > + * If the test fails then the H_CEDE call was unsuccessful and the
> > + * vulnerability wasn't tested.
> > + * If the test hits the vulnerability then it will never complete
> > or report and
> > + * the qemu process will block indefinitely. RCU stalls will be
> > detected on the
> > + * cpu and any process scheduled on the lost cpu will also block
> > indefinitely.
> > + */
> > +static void test_h_cede_tm(int argc, char **argv)
> > +{
> > +	bool pass = true;
> > +	int i;
> > +
> > +	if (argc > 2)
> > +		report_abort("Unsupported argument: '%s'",
> > argv[2]);
> > +
> > +	handle_exception(0x900, &dec_except_handler, NULL);
> > +
> > +	if (!start_all_cpus(&halt, 0))
> nit: more unnecessary '&' on function pointers
Ok, guess I should change both
> 
> > 
> > +		report_abort("Failed to start secondary cpus");
> > +
> > +	if (!enable_tm())
> > +		report_abort("Failed to enable tm");
> > +
> > +	/*
> > +	 * Begin a transaction and guarantee we are in the suspend
> > state
> > +	 * before continuing
> > +	 */
> > +	asm volatile ("1: .long 0x7c00051d\n\t"	/* tbegin.
> > */
> > +		      "beq 2f\n\t"
> > +		      ".long 0x7c0005dd\n\t"	/* tsuspend.
> > */
> > +		      "2: .long 0x7c00059c\n\t"	/* tcheck
> > cr0 */
> > +		      "bf 2,1b" : : : "cr0");
> > +
> > +	for (i = 0; i < 500 && pass; i++) {
> > +		uint64_t rval = h_cede();
> > +
> > +		if (rval != H_SUCCESS)
> > +			pass = false;
> nit: you could drop the '&& pass' and just do a break here.
Yes
> 
> > 
> > +		mdelay(5);
> > +	}
> > +
> > +	report("H_CEDE TM", pass);
> nit: with a break above you could just test for i = 500 here.
ok
> 
> > 
> > +}
> > +
> > +struct {
> > +	const char *name;
> > +	void (*func)(int argc, char **argv);
> > +} hctests[] = {
> > +	{ "h_cede_tm", test_h_cede_tm },
> > +	{ NULL, NULL }
> > +};
> > +
> > +int main(int argc, char **argv)
> > +{
> > +	bool all;
> > +	int i;
> > +
> > +	report_prefix_push("tm");
> > +
> > +	all = argc = 1 || (argc = 2 && !strcmp(argv[1], "all"));
> I'd drop the argc = 2 test. You already know argc >= 2 at this
> point. And, if it's > 2, then you want to call the test function
> so the "Unsupported argument" abort will fire.
Sounds good
> 
> > 
> > +
> > +	for (i = 0; hctests[i].name != NULL; i++) {
> > +		if (all || strcmp(argv[1], hctests[i].name) = 0)
> > {
> > +			report_prefix_push(hctests[i].name);
> > +			hctests[i].func(argc, argv);
> > +			report_prefix_pop();
> > +		}
> > +	}
> > +
> > +	return report_summary();
> > +}
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index 0098cb6..20dbde6 100644
> > --- a/powerpc/unittests.cfg
> > +++ b/powerpc/unittests.cfg
> > @@ -53,3 +53,9 @@ groups = rtas
> >  
> >  [emulator]
> >  file = emulator.elf
> > +
> > +[h_cede_tm]
> > +file = tm.elf
> > +smp = 2,threads=2
> > +extra_params = -append "h_cede_tm"
> > +groups = nodefault,h_cede_tm