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

[kvm-unit-tests PATCH V2 1/4] 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>
---
 arm/unittests.cfg       |  3 +++
 powerpc/unittests.cfg   |  3 +++
 scripts/mkstandalone.sh | 21 +++++++++++++++++++++
 scripts/runtime.bash    |  8 +++++++-
 x86/unittests.cfg       |  3 +++
 5 files changed, 37 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..64e85c9 100755
--- a/scripts/mkstandalone.sh
+++ b/scripts/mkstandalone.sh
@@ -74,6 +74,27 @@ generate_test ()
 
 	cat scripts/runtime.bash
 
+	if grep -qw "nodefault" <<<${args[1]}; then
+		echo -e "while true; do\n"\
+			"\tread -p \"Test marked as not to be run by default,"\
+			"are you sure (Y/N)? \" response\n"\
+			"\tcase \$response in\n"\
+			"\t\t\"Y\" | \"y\" | \"Yes\" | \"yes\")\n"\
+			"\t\t\tbreak\n"\
+			"\t\t\t;;\n"\
+			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
+			"\t\t\t;&\n"\
+			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
+			"\t\t\texit\n"\
+			"\t\t\t;;\n"\
+			"\t\t*)\n"\
+			"\t\t\techo Please select Y or N\n"\
+			"\t\t\t;;\n"\
+			"\tesac\n"\
+			"done"
+		echo "standalone=\"true\""
+	fi
+
 	echo "run ${args[@]}"
 }
 
diff --git a/scripts/runtime.bash b/scripts/runtime.bash
index 0503cf0..a7a2250 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -48,10 +48,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 &&
+            ([ -z $standalone ] || [ $standalone != "true" ]); 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 V2 2/4] 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>
---
 lib/powerpc/asm/handlers.h |  8 ++++++++
 lib/powerpc/handlers.c     | 25 +++++++++++++++++++++++++
 lib/ppc64/asm/handlers.h   |  1 +
 powerpc/Makefile.common    |  1 +
 4 files changed, 35 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..6a621f1
--- /dev/null
+++ b/lib/powerpc/handlers.c
@@ -0,0 +1,25 @@
+/*
+ * 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

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

[Thomas Huth]

On 10.08.2016 03:59, 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>
> ---
>  lib/powerpc/asm/handlers.h |  8 ++++++++
>  lib/powerpc/handlers.c     | 25 +++++++++++++++++++++++++
>  lib/ppc64/asm/handlers.h   |  1 +
>  powerpc/Makefile.common    |  1 +
>  4 files changed, 35 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..6a621f1
> --- /dev/null
> +++ b/lib/powerpc/handlers.c
> @@ -0,0 +1,25 @@
> +/*
> + * 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)
> +					:
> +		     );

I think most "asm volatile" statements are written in a more condensed
way in kvm-unit-tests, so it might be more consistent to do that here,
too instead:

	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)

Apart from the above cosmetic nit, the patch looks fine to me.

Reviewed-by: Thomas Huth <thuth@redhat.com>

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

[Suraj Jitindar Singh]

On Wed, 2016-08-10 at 12:38 +0200, Thomas Huth wrote:
> On 10.08.2016 03:59, 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>
> > ---
> >  lib/powerpc/asm/handlers.h |  8 ++++++++
> >  lib/powerpc/handlers.c     | 25 +++++++++++++++++++++++++
> >  lib/ppc64/asm/handlers.h   |  1 +
> >  powerpc/Makefile.common    |  1 +
> >  4 files changed, 35 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..6a621f1
> > --- /dev/null
> > +++ b/lib/powerpc/handlers.c
> > @@ -0,0 +1,25 @@
> > +/*
> > + * 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)
> > +					:
> > +		     );
> I think most "asm volatile" statements are written in a more
> condensed
> way in kvm-unit-tests, so it might be more consistent to do that
> here,
> too instead:
> 
> 	asm volatile("mtdec %0"	: : "r" (dec));
> 
Will make whitespace changes to make this look more like existing code.
> > 
> > +}
> > 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)
> Apart from the above cosmetic nit, the patch looks fine to me.
> 
> Reviewed-by: Thomas Huth <thuth@redhat.com>
> 
Thanks for looking at this

[kvm-unit-tests PATCH V2 3/4] 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>
---
 lib/powerpc/asm/smp.h   |  15 +++++++
 lib/powerpc/smp.c       | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ppc64/asm/smp.h     |   1 +
 powerpc/Makefile.common |   1 +
 4 files changed, 132 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..a4f3e7f
--- /dev/null
+++ b/lib/powerpc/asm/smp.h
@@ -0,0 +1,15 @@
+#ifndef _ASMPOWERPC_SMP_H_
+#define _ASMPOWERPC_SMP_H_
+
+#include <libcflat.h>
+#include <stdint.h>
+
+typedef void (*secondary_entry_fn)(void);
+
+extern void halt(void);
+
+bool start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
+bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t r3);
+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..8968907
--- /dev/null
+++ b/lib/powerpc/smp.c
@@ -0,0 +1,115 @@
+/*
+ * Secondary cpu support
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#include <stdint.h>
+#include <libfdt/libfdt.h>
+#include <libfdt/fdt.h>
+#include <devicetree.h>
+#include <libcflat.h>
+#include <string.h>
+#include <asm/rtas.h>
+#include <asm/smp.h>
+
+struct secondary_entry_data {
+	secondary_entry_fn entry;
+	uint64_t r3;
+	bool init_failed;
+};
+
+/*
+ * Start stopped thread cpu_id at entry
+ * Returns:	1 on success or cpu not in stopped state
+ *		0 on failure to start stopped cpu
+ *
+ * Note: This function returns 1 on success in starting a stopped cpu or if the
+ *	 given cpu was not in the stopped state. Thus this can be called on a
+ *	 list of cpus and all the stopped ones will be started while false
+ *	 won't be returned if some cpus in that list were already running. Thus
+ *	 the user should check that cpus passed to this function are already in
+ *	 the stopped state if they want to guarantee that a return value of
+ *	 true corresponds to the given cpu now executing at entry. This
+ *	 function checks again however as calling cpu-start on a not stopped
+ *	 cpu results in undefined behaviour.
+ */
+bool 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) {
+		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
+		return false;
+	}
+
+	if (!outputs[0]) {	/* cpu in stopped state */
+		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
+		if (ret) {
+			printf("failed to start cpu %d\n", cpu_id);
+			return false;
+		}
+	}
+
+	return true;
+}
+
+/*
+ * Start all stopped threads (vcpus) on cpu_node
+ * Returns:	1 on success
+ *		0 on failure
+ */
+bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t r3)
+{
+	const struct fdt_property *prop;
+	int len, nr_cpu, cpu;
+	u32 *cpus;
+	bool ret = true;
+
+	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
+	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
+
+	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
+		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);
+
+	return ret;
+}
+
+static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
+{
+	struct secondary_entry_data *datap = info;
+
+	datap->init_failed |= !start_cpu(fdtnode, datap->entry, datap->r3);
+}
+
+/*
+ * Start all stopped cpus on the guest at entry with register 3 set to r3
+ * Returns:	1 on success
+ *		0 on failure
+ */
+bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
+{
+	struct secondary_entry_data data = {
+		entry,
+		r3,
+		false
+	};
+	int ret;
+
+	ret = dt_for_each_cpu_node(&start_each_secondary, (void *) &data);
+
+	return !(ret || data.init_failed);
+}
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

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

[Thomas Huth]

On 10.08.2016 03:59, 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>
> ---
[...]
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns:	1 on success or cpu not in stopped state
> + *		0 on failure to start stopped cpu
> + *
> + * Note: This function returns 1 on success in starting a stopped cpu or if the
> + *	 given cpu was not in the stopped state. Thus this can be called on a
> + *	 list of cpus and all the stopped ones will be started while false
> + *	 won't be returned if some cpus in that list were already running. Thus
> + *	 the user should check that cpus passed to this function are already in
> + *	 the stopped state if they want to guarantee that a return value of
> + *	 true corresponds to the given cpu now executing at entry. This
> + *	 function checks again however as calling cpu-start on a not stopped
> + *	 cpu results in undefined behaviour.
> + */
> +bool 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) {
> +		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
> +		return false;
> +	}
> +
> +	if (!outputs[0]) {	/* cpu in stopped state */

Maybe add an "assert(outputs[0] != 1)" before the if-statement?

> +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
> +		if (ret) {
> +			printf("failed to start cpu %d\n", cpu_id);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * Start all stopped threads (vcpus) on cpu_node
> + * Returns:	1 on success
> + *		0 on failure
> + */
> +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t r3)
> +{
> +	const struct fdt_property *prop;
> +	int len, nr_cpu, cpu;
> +	u32 *cpus;
> +	bool ret = true;
> +
> +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> +
> +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);

This way you only return the success or failure of the last thread that
has been started. All other information will be lost. Wouldn't it be
better to return false as soon as one of the threads could not be started?

> +	return ret;
> +}
> +
> +static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
> +{
> +	struct secondary_entry_data *datap = info;
> +
> +	datap->init_failed |= !start_cpu(fdtnode, datap->entry, datap->r3);
> +}
> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * Returns:	1 on success
> + *		0 on failure
> + */
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> +{
> +	struct secondary_entry_data data = {
> +		entry,
> +		r3,
> +		false
> +	};
> +	int ret;
> +
> +	ret = dt_for_each_cpu_node(&start_each_secondary, (void *) &data);

I think you don't need the (void*) cast here.

> +	return !(ret || data.init_failed);
> +}

 Thomas

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

[Suraj Jitindar Singh]

On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
> On 10.08.2016 03:59, 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>
> > ---
> [...]
> > 
> > +/*
> > + * Start stopped thread cpu_id at entry
> > + * Returns:	1 on success or cpu not in stopped state
> > + *		0 on failure to start stopped cpu
> > + *
> > + * Note: This function returns 1 on success in starting a stopped
> > cpu or if the
> > + *	 given cpu was not in the stopped state. Thus this can
> > be called on a
> > + *	 list of cpus and all the stopped ones will be started
> > while false
> > + *	 won't be returned if some cpus in that list were
> > already running. Thus
> > + *	 the user should check that cpus passed to this function
> > are already in
> > + *	 the stopped state if they want to guarantee that a
> > return value of
> > + *	 true corresponds to the given cpu now executing at
> > entry. This
> > + *	 function checks again however as calling cpu-start on a
> > not stopped
> > + *	 cpu results in undefined behaviour.
> > + */
> > +bool 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) {
> > +		printf("query-cpu-stopped-state failed for cpu
> > %d\n", cpu_id);
> > +		return false;
> > +	}
> > +
> > +	if (!outputs[0]) {	/* cpu in stopped state */
> Maybe add an "assert(outputs[0] != 1)" before the if-statement?
> 
I'm torn because if I add the assert then the caller has to check the
cpu-stopped-state as well as it being checked here to avoid calling
this on a !stopped cpu (and hitting the assert) which means that this
will always be checked twice.
I could just define that this must be called on a stopped cpu, not
check in this function and leave it up to the caller. In the event this
is called on an already running cpu I'm not really sure what the
behaviour is though...
The way I'm using this function in the start_cpu function below I
assume that this function doesn't exit the test when an already running
cpu is hit because I basically just want to start all the stopped cpus
somewhere so I call it on every cpu in the system and expect to just
skip already running ones.
I think the best option is to have this return an int based on whether
the cpu was started successfully, the start-cpu call failed or the cpu
passed to the function was not in the stopped state. Then the caller
can just filter on the return value based on whether it cares if an
already running cpu was hit or not. 
> > 
> > +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> > entry, r3);
> > +		if (ret) {
> > +			printf("failed to start cpu %d\n",
> > cpu_id);
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Start all stopped threads (vcpus) on cpu_node
> > + * Returns:	1 on success
> > + *		0 on failure
> > + */
> > +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
> > r3)
> > +{
> > +	const struct fdt_property *prop;
> > +	int len, nr_cpu, cpu;
> > +	u32 *cpus;
> > +	bool ret = true;
> > +
> > +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> > +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> > +
> > +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > r3);
> This way you only return the success or failure of the last thread
> that
> has been started. All other information will be lost. Wouldn't it be
> better to return false as soon as one of the threads could not be
> started?
> 
AFAIK that is the current functionality given the "cpu < nr_cpu && ret"
								   ^^^
in the for conditional.
> > 
> > +	return ret;
> > +}
> > +
> > +static void start_each_secondary(int fdtnode, u32 regval __unused,
> > void *info)
> > +{
> > +	struct secondary_entry_data *datap = info;
> > +
> > +	datap->init_failed |= !start_cpu(fdtnode, datap->entry,
> > datap->r3);
> > +}
> > +
> > +/*
> > + * Start all stopped cpus on the guest at entry with register 3
> > set to r3
> > + * Returns:	1 on success
> > + *		0 on failure
> > + */
> > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > +{
> > +	struct secondary_entry_data data = {
> > +		entry,
> > +		r3,
> > +		false
> > +	};
> > +	int ret;
> > +
> > +	ret = dt_for_each_cpu_node(&start_each_secondary, (void *)
> > &data);
> I think you don't need the (void*) cast here.
> 
That's probably the case
> > 
> > +	return !(ret || data.init_failed);
> > +}
>  Thomas
> 
> 
Thanks for the code review

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

[Thomas Huth]

On 12.08.2016 08:30, Suraj Jitindar Singh wrote:
> On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
>> On 10.08.2016 03:59, 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>
>>> ---
>> [...]
>>>
>>> +/*
>>> + * Start stopped thread cpu_id at entry
>>> + * Returns:	1 on success or cpu not in stopped state
>>> + *		0 on failure to start stopped cpu
>>> + *
>>> + * Note: This function returns 1 on success in starting a stopped
>>> cpu or if the
>>> + *	 given cpu was not in the stopped state. Thus this can
>>> be called on a
>>> + *	 list of cpus and all the stopped ones will be started
>>> while false
>>> + *	 won't be returned if some cpus in that list were
>>> already running. Thus
>>> + *	 the user should check that cpus passed to this function
>>> are already in
>>> + *	 the stopped state if they want to guarantee that a
>>> return value of
>>> + *	 true corresponds to the given cpu now executing at
>>> entry. This
>>> + *	 function checks again however as calling cpu-start on a
>>> not stopped
>>> + *	 cpu results in undefined behaviour.
>>> + */
>>> +bool 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) {
>>> +		printf("query-cpu-stopped-state failed for cpu
>>> %d\n", cpu_id);
>>> +		return false;
>>> +	}
>>> +
>>> +	if (!outputs[0]) {	/* cpu in stopped state */
>> Maybe add an "assert(outputs[0] != 1)" before the if-statement?
>>
> I'm torn because if I add the assert then the caller has to check the
> cpu-stopped-state as well as it being checked here to avoid calling
> this on a !stopped cpu (and hitting the assert) which means that this
> will always be checked twice.
> [...]

Not sure if you've got me right, or whether I've got your concerns here
right, but what I meant to say was:
query-cpu-stopped-state can return three different values:

 0: The processor thread is in the RTAS stopped state
 1: stop-self is in progress
 2: The processor thread is not in the RTAS stopped state

For 0 and 2, your code is certainly fine. But what happens if the return
value was "stop-self is in progress"? Can "start-cpu" start a CPU again
that is currently in progress of entering the stopped state? If yes, I
think your code is fine as it is, but if not, you end up with a CPU that
is finally stopped again, though you assume that it is running at the
end of your function. In that case it might be helpful to report that
strange state with an assert() to ease debugging later (currently, I
think qemu won't return 1 here, so this should never happen).

Anyway, looking at the code again, I think start-cpu should return an
error if it is not able to start a CPU that is currently in that
"stop-self is in progress" state ... and that should catch the
hypothetical error condition, too. So never mind, there's likely no
additional handling needed here.

>>>
>>> +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
>>> entry, r3);
>>> +		if (ret) {
>>> +			printf("failed to start cpu %d\n",
>>> cpu_id);
>>> +			return false;
>>> +		}
>>> +	}
>>> +
>>> +	return true;
>>> +}
>>> +
>>> +/*
>>> + * Start all stopped threads (vcpus) on cpu_node
>>> + * Returns:	1 on success
>>> + *		0 on failure
>>> + */
>>> +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
>>> r3)
>>> +{
>>> +	const struct fdt_property *prop;
>>> +	int len, nr_cpu, cpu;
>>> +	u32 *cpus;
>>> +	bool ret = true;
>>> +
>>> +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
>>> +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
>>> +
>>> +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
>>> +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry,
>>> r3);
>> This way you only return the success or failure of the last thread
>> that
>> has been started. All other information will be lost. Wouldn't it be
>> better to return false as soon as one of the threads could not be
>> started?
>>
> AFAIK that is the current functionality given the "cpu < nr_cpu && ret"
> 								   ^^^
> in the for conditional.

Oh, right, my bad, I overlooked that check. So never mind.

 Thomas

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

[Suraj Jitindar Singh]

On Fri, 2016-08-12 at 13:19 +0200, Thomas Huth wrote:
> On 12.08.2016 08:30, Suraj Jitindar Singh wrote:
> > 
> > On Wed, 2016-08-10 at 13:25 +0200, Thomas Huth wrote:
> > > 
> > > On 10.08.2016 03:59, 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>
> > > > ---
> > > [...]
> > > > 
> > > > 
> > > > +/*
> > > > + * Start stopped thread cpu_id at entry
> > > > + * Returns:	1 on success or cpu not in stopped state
> > > > + *		0 on failure to start stopped cpu
> > > > + *
> > > > + * Note: This function returns 1 on success in starting a
> > > > stopped
> > > > cpu or if the
> > > > + *	 given cpu was not in the stopped state. Thus this
> > > > can
> > > > be called on a
> > > > + *	 list of cpus and all the stopped ones will be
> > > > started
> > > > while false
> > > > + *	 won't be returned if some cpus in that list were
> > > > already running. Thus
> > > > + *	 the user should check that cpus passed to this
> > > > function
> > > > are already in
> > > > + *	 the stopped state if they want to guarantee that a
> > > > return value of
> > > > + *	 true corresponds to the given cpu now executing at
> > > > entry. This
> > > > + *	 function checks again however as calling cpu-start
> > > > on a
> > > > not stopped
> > > > + *	 cpu results in undefined behaviour.
> > > > + */
> > > > +bool 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) {
> > > > +		printf("query-cpu-stopped-state failed for cpu
> > > > %d\n", cpu_id);
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	if (!outputs[0]) {	/* cpu in stopped state */
> > > Maybe add an "assert(outputs[0] != 1)" before the if-statement?
> > > 
> > I'm torn because if I add the assert then the caller has to check
> > the
> > cpu-stopped-state as well as it being checked here to avoid calling
> > this on a !stopped cpu (and hitting the assert) which means that
> > this
> > will always be checked twice.
> > [...]
> Not sure if you've got me right, or whether I've got your concerns
> here
> right, but what I meant to say was:
> query-cpu-stopped-state can return three different values:
> 
>  0: The processor thread is in the RTAS stopped state
>  1: stop-self is in progress
>  2: The processor thread is not in the RTAS stopped state
> 
> For 0 and 2, your code is certainly fine. But what happens if the
> return
> value was "stop-self is in progress"? Can "start-cpu" start a CPU
> again
> that is currently in progress of entering the stopped state? If yes, 
>From my understanding start-cpu is only guaranteed to exhibit defined
behaviour when called on a cpu in the stopped state, otherwise its
behaviour is undefined and probably implementation dependant.
> I
> think your code is fine as it is, but if not, you end up with a CPU
> that
> is finally stopped again, though you assume that it is running at the
> end of your function. In that case it might be helpful to report that
> strange state with an assert() to ease debugging later (currently, I
> think qemu won't return 1 here, so this should never happen).
Yeah it looks like QEMU doesn't have the concept of a transitional
state and for QEMU at least it is safe to call start-cpu on an already
running cpu.
> 
> Anyway, looking at the code again, I think start-cpu should return an
> error if it is not able to start a CPU that is currently in that
> "stop-self is in progress" state ... and that should catch the
> hypothetical error condition, too. So never mind, there's likely no
> additional handling needed here.
I was thinking of changing this function to return 1|2 if the cpu
specified isn't in the stoped state without trying to start it. And
then return the negative error return value if either of the rtas calls
failed. Then it is up to the caller to filter the return value based on
what they think is acceptable (e.g. just ignore if a cpu was already
running).
> 
> > 
> > > 
> > > > 
> > > > 
> > > > +		ret = rtas_call(start_token, 3, 1, NULL,
> > > > cpu_id,
> > > > entry, r3);
> > > > +		if (ret) {
> > > > +			printf("failed to start cpu %d\n",
> > > > cpu_id);
> > > > +			return false;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Start all stopped threads (vcpus) on cpu_node
> > > > + * Returns:	1 on success
> > > > + *		0 on failure
> > > > + */
> > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3)
> > > > +{
> > > > +	const struct fdt_property *prop;
> > > > +	int len, nr_cpu, cpu;
> > > > +	u32 *cpus;
> > > > +	bool ret = true;
> > > > +
> > > > +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per
> > > > cpu */
> > > > +	cpus = (u32 *)prop->data; /* Array of valid cpu
> > > > numbers */
> > > > +
> > > > +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > > > +		ret = start_thread(fdt32_to_cpu(cpus[cpu]),
> > > > entry,
> > > > r3);
> > > This way you only return the success or failure of the last
> > > thread
> > > that
> > > has been started. All other information will be lost. Wouldn't it
> > > be
> > > better to return false as soon as one of the threads could not be
> > > started?
> > > 
> > AFAIK that is the current functionality given the "cpu < nr_cpu &&
> > ret"
> > 								   ^^^
> > in the for conditional.
> Oh, right, my bad, I overlooked that check. So never mind.
> 
>  Thomas
> 

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

[Andrew Jones]

On Wed, Aug 10, 2016 at 11:59:36AM +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>
> ---
>  lib/powerpc/asm/smp.h   |  15 +++++++
>  lib/powerpc/smp.c       | 115 ++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ppc64/asm/smp.h     |   1 +
>  powerpc/Makefile.common |   1 +
>  4 files changed, 132 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..a4f3e7f
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,15 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +#include <libcflat.h>
> +#include <stdint.h>

nit: no need to include stdint.h, libcflat.h already does, so most
files neglect it.

> +
> +typedef void (*secondary_entry_fn)(void);
> +
> +extern void halt(void);
> +
> +bool start_thread(int cpu_id, secondary_entry_fn entry, uint32_t r3);
> +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t r3);
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);

nit: kvm-unit-tests likes to use 'extern' on function declarations

> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..8968907
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,115 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#include <stdint.h>

same nit as above

> +#include <libfdt/libfdt.h>
> +#include <libfdt/fdt.h>

nit: only need libfdt/libfdt.h, as that includes fdt.h

> +#include <devicetree.h>
> +#include <libcflat.h>
> +#include <string.h>
> +#include <asm/rtas.h>
> +#include <asm/smp.h>
> +
> +struct secondary_entry_data {
> +	secondary_entry_fn entry;
> +	uint64_t r3;
> +	bool init_failed;
> +};
> +
> +/*
> + * Start stopped thread cpu_id at entry
> + * Returns:	1 on success or cpu not in stopped state
> + *		0 on failure to start stopped cpu

Returns: TRUE  ...
         FALSE ...

> + *
> + * Note: This function returns 1 on success in starting a stopped cpu or if the

returns true

> + *	 given cpu was not in the stopped state. Thus this can be called on a
> + *	 list of cpus and all the stopped ones will be started while false
> + *	 won't be returned if some cpus in that list were already running. Thus
> + *	 the user should check that cpus passed to this function are already in
> + *	 the stopped state if they want to guarantee that a return value of
> + *	 true corresponds to the given cpu now executing at entry. This
> + *	 function checks again however as calling cpu-start on a not stopped
> + *	 cpu results in undefined behaviour.
> + */
> +bool 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) {
> +		printf("query-cpu-stopped-state failed for cpu %d\n", cpu_id);
> +		return false;
> +	}
> +
> +	if (!outputs[0]) {	/* cpu in stopped state */
> +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id, entry, r3);
> +		if (ret) {
> +			printf("failed to start cpu %d\n", cpu_id);
> +			return false;
> +		}
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * Start all stopped threads (vcpus) on cpu_node
> + * Returns:	1 on success
> + *		0 on failure

TRUE/FALSE, but I'd actually change the return type to struct start_threads;

 struct start_threads {
    int nr_threads;
    int nr_started;
 };

Then...

> + */
> +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t r3)
> +{
> +	const struct fdt_property *prop;
> +	int len, nr_cpu, cpu;
> +	u32 *cpus;
> +	bool ret = true;
> +
> +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> +
> +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);
> +	return ret;

...change this to

 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++)
     nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);

 return (struct start_threads){ nr_threads, nr_started };

and...
    
> +}
> +
> +static void start_each_secondary(int fdtnode, u32 regval __unused, void *info)
> +{
> +	struct secondary_entry_data *datap = info;
> +
> +	datap->init_failed |= !start_cpu(fdtnode, datap->entry, datap->r3);
> +}

...change init_failed to nr_started

 start_threads = start_cpu(fdtnode, datap->entry, datap->r3);
 nr_cpus += start_threads.nr_threads; // nr_cpus is global like ARM has
 datap->nr_started += start_threads.nr_started;

and below just check that datap->nr_started == nr_cpus.

> +
> +/*
> + * Start all stopped cpus on the guest at entry with register 3 set to r3
> + * Returns:	1 on success
> + *		0 on failure

TRUE/FALSE

> + */
> +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> +{
> +	struct secondary_entry_data data = {
> +		entry,
> +		r3,
> +		false
> +	};
> +	int ret;
> +
> +	ret = dt_for_each_cpu_node(&start_each_secondary, (void *) &data);

assert(ret == 0)

> +
> +	return !(ret || data.init_failed);

See comment above about setting nr_cpus, and then just confirming they all
started here instead.

> +}
> 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

Thanks,
drew

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

[Suraj Jitindar Singh]

On Fri, 2016-08-12 at 19:07 +0200, Andrew Jones wrote:
> On Wed, Aug 10, 2016 at 11:59:36AM +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>
> > ---
> >  lib/powerpc/asm/smp.h   |  15 +++++++
> >  lib/powerpc/smp.c       | 115
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ppc64/asm/smp.h     |   1 +
> >  powerpc/Makefile.common |   1 +
> >  4 files changed, 132 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..a4f3e7f
> > --- /dev/null
> > +++ b/lib/powerpc/asm/smp.h
> > @@ -0,0 +1,15 @@
> > +#ifndef _ASMPOWERPC_SMP_H_
> > +#define _ASMPOWERPC_SMP_H_
> > +
> > +#include <libcflat.h>
> > +#include <stdint.h>
> nit: no need to include stdint.h, libcflat.h already does, so most
> files neglect it.
Will remove
> 
> > 
> > +
> > +typedef void (*secondary_entry_fn)(void);
> > +
> > +extern void halt(void);
> > +
> > +bool start_thread(int cpu_id, secondary_entry_fn entry, uint32_t
> > r3);
> > +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
> > r3);
> > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> nit: kvm-unit-tests likes to use 'extern' on function declarations
Ok, I'll add this
> 
> > 
> > +
> > +#endif /* _ASMPOWERPC_SMP_H_ */
> > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > new file mode 100644
> > index 0000000..8968907
> > --- /dev/null
> > +++ b/lib/powerpc/smp.c
> > @@ -0,0 +1,115 @@
> > +/*
> > + * Secondary cpu support
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +
> > +#include <stdint.h>
> same nit as above
> 
> > 
> > +#include <libfdt/libfdt.h>
> > +#include <libfdt/fdt.h>
> nit: only need libfdt/libfdt.h, as that includes fdt.h
> 
> > 
> > +#include <devicetree.h>
> > +#include <libcflat.h>
> > +#include <string.h>
> > +#include <asm/rtas.h>
> > +#include <asm/smp.h>
> > +
> > +struct secondary_entry_data {
> > +	secondary_entry_fn entry;
> > +	uint64_t r3;
> > +	bool init_failed;
> > +};
> > +
> > +/*
> > + * Start stopped thread cpu_id at entry
> > + * Returns:	1 on success or cpu not in stopped state
> > + *		0 on failure to start stopped cpu
> Returns: TRUE  ...
>          FALSE ...
Same thing, but ok
> 
> > 
> > + *
> > + * Note: This function returns 1 on success in starting a stopped
> > cpu or if the
> returns true
> 
> > 
> > + *	 given cpu was not in the stopped state. Thus this can
> > be called on a
> > + *	 list of cpus and all the stopped ones will be started
> > while false
> > + *	 won't be returned if some cpus in that list were
> > already running. Thus
> > + *	 the user should check that cpus passed to this function
> > are already in
> > + *	 the stopped state if they want to guarantee that a
> > return value of
> > + *	 true corresponds to the given cpu now executing at
> > entry. This
> > + *	 function checks again however as calling cpu-start on a
> > not stopped
> > + *	 cpu results in undefined behaviour.
> > + */
> > +bool 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) {
> > +		printf("query-cpu-stopped-state failed for cpu
> > %d\n", cpu_id);
> > +		return false;
> > +	}
> > +
> > +	if (!outputs[0]) {	/* cpu in stopped state */
> > +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> > entry, r3);
> > +		if (ret) {
> > +			printf("failed to start cpu %d\n",
> > cpu_id);
> > +			return false;
> > +		}
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * Start all stopped threads (vcpus) on cpu_node
> > + * Returns:	1 on success
> > + *		0 on failure
> TRUE/FALSE, but I'd actually change the return type to struct
> start_threads;
> 
>  struct start_threads {
>     int nr_threads;
>     int nr_started;
>  };
> 
> Then...
> 
> > 
> > + */
> > +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
> > r3)
> > +{
> > +	const struct fdt_property *prop;
> > +	int len, nr_cpu, cpu;
> > +	u32 *cpus;
> > +	bool ret = true;
> > +
> > +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> > +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> > +
> > +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > r3);
> > +	return ret;
> ...change this to
> 
>  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++)
>      nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);
I guess this fits the more generic use case. Although I can't really
see a scenario where the caller wants to start all cpus and continue
starting them when one fails, that is if one fails to start you
probably might as well return an error code immediately.
> 
>  return (struct start_threads){ nr_threads, nr_started };
> 
> and...
>     
> > 
> > +}
> > +
> > +static void start_each_secondary(int fdtnode, u32 regval __unused,
> > void *info)
> > +{
> > +	struct secondary_entry_data *datap = info;
> > +
> > +	datap->init_failed |= !start_cpu(fdtnode, datap->entry,
> > datap->r3);
> > +}
> ...change init_failed to nr_started
> 
>  start_threads = start_cpu(fdtnode, datap->entry, datap->r3);
>  nr_cpus += start_threads.nr_threads; // nr_cpus is global like ARM
> has
>  datap->nr_started += start_threads.nr_started;
> 
> and below just check that datap->nr_started == nr_cpus.
nr_cpus is set during setup so it would be possible to just have the
above return nr_started and then check this accumulated value against
nr_cpu below.
> 
> > 
> > +
> > +/*
> > + * Start all stopped cpus on the guest at entry with register 3
> > set to r3
> > + * Returns:	1 on success
> > + *		0 on failure
> TRUE/FALSE
Ok
> 
> > 
> > + */
> > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > +{
> > +	struct secondary_entry_data data = {
> > +		entry,
> > +		r3,
> > +		false
> > +	};
> > +	int ret;
> > +
> > +	ret = dt_for_each_cpu_node(&start_each_secondary, (void *)
> > &data);
> assert(ret == 0)
Sounds good
> 
> > 
> > +
> > +	return !(ret || data.init_failed);
> See comment above about setting nr_cpus, and then just confirming
> they all
> started here instead.
I think I'll change the above so that start_cpu returns the number of
cpus started (we already know the total number of cpus so the struct is
unnecessary), we come in with one cpu already started so I'll check
that nr_started == nr_cpu - 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)
Thanks for the review

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

[Andrew Jones]

On Mon, Aug 15, 2016 at 11:58:46AM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2016-08-12 at 19:07 +0200, Andrew Jones wrote:
> > On Wed, Aug 10, 2016 at 11:59:36AM +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>
> > > ---
> > >  lib/powerpc/asm/smp.h   |  15 +++++++
> > >  lib/powerpc/smp.c       | 115
> > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > >  lib/ppc64/asm/smp.h     |   1 +
> > >  powerpc/Makefile.common |   1 +
> > >  4 files changed, 132 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..a4f3e7f
> > > --- /dev/null
> > > +++ b/lib/powerpc/asm/smp.h
> > > @@ -0,0 +1,15 @@
> > > +#ifndef _ASMPOWERPC_SMP_H_
> > > +#define _ASMPOWERPC_SMP_H_
> > > +
> > > +#include <libcflat.h>
> > > +#include <stdint.h>
> > nit: no need to include stdint.h, libcflat.h already does, so most
> > files neglect it.
> Will remove
> > 
> > > 
> > > +
> > > +typedef void (*secondary_entry_fn)(void);
> > > +
> > > +extern void halt(void);
> > > +
> > > +bool start_thread(int cpu_id, secondary_entry_fn entry, uint32_t
> > > r3);
> > > +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
> > > r3);
> > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> > nit: kvm-unit-tests likes to use 'extern' on function declarations
> Ok, I'll add this
> > 
> > > 
> > > +
> > > +#endif /* _ASMPOWERPC_SMP_H_ */
> > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > > new file mode 100644
> > > index 0000000..8968907
> > > --- /dev/null
> > > +++ b/lib/powerpc/smp.c
> > > @@ -0,0 +1,115 @@
> > > +/*
> > > + * Secondary cpu support
> > > + *
> > > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > > + *
> > > + * This work is licensed under the terms of the GNU LGPL, version
> > > 2.
> > > + */
> > > +
> > > +#include <stdint.h>
> > same nit as above
> > 
> > > 
> > > +#include <libfdt/libfdt.h>
> > > +#include <libfdt/fdt.h>
> > nit: only need libfdt/libfdt.h, as that includes fdt.h
> > 
> > > 
> > > +#include <devicetree.h>
> > > +#include <libcflat.h>
> > > +#include <string.h>
> > > +#include <asm/rtas.h>
> > > +#include <asm/smp.h>
> > > +
> > > +struct secondary_entry_data {
> > > +	secondary_entry_fn entry;
> > > +	uint64_t r3;
> > > +	bool init_failed;
> > > +};
> > > +
> > > +/*
> > > + * Start stopped thread cpu_id at entry
> > > + * Returns:	1 on success or cpu not in stopped state
> > > + *		0 on failure to start stopped cpu
> > Returns: TRUE  ...
> >          FALSE ...
> Same thing, but ok
> > 
> > > 
> > > + *
> > > + * Note: This function returns 1 on success in starting a stopped
> > > cpu or if the
> > returns true
> > 
> > > 
> > > + *	 given cpu was not in the stopped state. Thus this can
> > > be called on a
> > > + *	 list of cpus and all the stopped ones will be started
> > > while false
> > > + *	 won't be returned if some cpus in that list were
> > > already running. Thus
> > > + *	 the user should check that cpus passed to this function
> > > are already in
> > > + *	 the stopped state if they want to guarantee that a
> > > return value of
> > > + *	 true corresponds to the given cpu now executing at
> > > entry. This
> > > + *	 function checks again however as calling cpu-start on a
> > > not stopped
> > > + *	 cpu results in undefined behaviour.
> > > + */
> > > +bool 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) {
> > > +		printf("query-cpu-stopped-state failed for cpu
> > > %d\n", cpu_id);
> > > +		return false;
> > > +	}
> > > +
> > > +	if (!outputs[0]) {	/* cpu in stopped state */
> > > +		ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> > > entry, r3);
> > > +		if (ret) {
> > > +			printf("failed to start cpu %d\n",
> > > cpu_id);
> > > +			return false;
> > > +		}
> > > +	}
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +/*
> > > + * Start all stopped threads (vcpus) on cpu_node
> > > + * Returns:	1 on success
> > > + *		0 on failure
> > TRUE/FALSE, but I'd actually change the return type to struct
> > start_threads;
> > 
> >  struct start_threads {
> >     int nr_threads;
> >     int nr_started;
> >  };
> > 
> > Then...
> > 
> > > 
> > > + */
> > > +bool start_cpu(int cpu_node, secondary_entry_fn entry, uint32_t
> > > r3)
> > > +{
> > > +	const struct fdt_property *prop;
> > > +	int len, nr_cpu, cpu;
> > > +	u32 *cpus;
> > > +	bool ret = true;
> > > +
> > > +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per cpu */
> > > +	cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> > > +
> > > +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > > +		ret = start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > > r3);
> > > +	return ret;
> > ...change this to
> > 
> >  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++)
> >      nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry, r3);
> I guess this fits the more generic use case. Although I can't really
> see a scenario where the caller wants to start all cpus and continue
> starting them when one fails, that is if one fails to start you
> probably might as well return an error code immediately.
> > 
> >  return (struct start_threads){ nr_threads, nr_started };
> > 
> > and...
> >     
> > > 
> > > +}
> > > +
> > > +static void start_each_secondary(int fdtnode, u32 regval __unused,
> > > void *info)
> > > +{
> > > +	struct secondary_entry_data *datap = info;
> > > +
> > > +	datap->init_failed |= !start_cpu(fdtnode, datap->entry,
> > > datap->r3);
> > > +}
> > ...change init_failed to nr_started
> > 
> >  start_threads = start_cpu(fdtnode, datap->entry, datap->r3);
> >  nr_cpus += start_threads.nr_threads; // nr_cpus is global like ARM
> > has
> >  datap->nr_started += start_threads.nr_started;
> > 
> > and below just check that datap->nr_started == nr_cpus.
> nr_cpus is set during setup so it would be possible to just have the
> above return nr_started and then check this accumulated value against
> nr_cpu below.
> > 
> > > 
> > > +
> > > +/*
> > > + * Start all stopped cpus on the guest at entry with register 3
> > > set to r3
> > > + * Returns:	1 on success
> > > + *		0 on failure
> > TRUE/FALSE
> Ok
> > 
> > > 
> > > + */
> > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > > +{
> > > +	struct secondary_entry_data data = {
> > > +		entry,
> > > +		r3,
> > > +		false
> > > +	};
> > > +	int ret;
> > > +
> > > +	ret = dt_for_each_cpu_node(&start_each_secondary, (void *)
> > > &data);
> > assert(ret == 0)
> Sounds good
> > 
> > > 
> > > +
> > > +	return !(ret || data.init_failed);
> > See comment above about setting nr_cpus, and then just confirming
> > they all
> > started here instead.
> I think I'll change the above so that start_cpu returns the number of
> cpus started (we already know the total number of cpus so the struct is
> unnecessary), we come in with one cpu already started so I'll check
> that nr_started == nr_cpu - 1.

I completely forgot that I wrote code setting up nr_cpus... After
reading this patch, I actually assumed I hadn't, because I didn't
recall addressing threads. So is the nr_cpus in setup correct?
Without accounting for threads, then it can't be, can it? Maybe
this patch series should start by fixing that, and also bumping
NR_CPUS in lib/powerpc/asm/setup.h up to whatever makes more sense.

> > 
> > > 
> > > +}
> > > 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)
> Thanks for the review

Thanks,
drew

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

[Suraj Jitindar Singh]

On Mon, 2016-08-15 at 08:27 +0200, Andrew Jones wrote:
> On Mon, Aug 15, 2016 at 11:58:46AM +1000, Suraj Jitindar Singh wrote:
> > 
> > On Fri, 2016-08-12 at 19:07 +0200, Andrew Jones wrote:
> > > 
> > > On Wed, Aug 10, 2016 at 11:59:36AM +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>
> > > > ---
> > > >  lib/powerpc/asm/smp.h   |  15 +++++++
> > > >  lib/powerpc/smp.c       | 115
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > >  lib/ppc64/asm/smp.h     |   1 +
> > > >  powerpc/Makefile.common |   1 +
> > > >  4 files changed, 132 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..a4f3e7f
> > > > --- /dev/null
> > > > +++ b/lib/powerpc/asm/smp.h
> > > > @@ -0,0 +1,15 @@
> > > > +#ifndef _ASMPOWERPC_SMP_H_
> > > > +#define _ASMPOWERPC_SMP_H_
> > > > +
> > > > +#include <libcflat.h>
> > > > +#include <stdint.h>
> > > nit: no need to include stdint.h, libcflat.h already does, so
> > > most
> > > files neglect it.
> > Will remove
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +typedef void (*secondary_entry_fn)(void);
> > > > +
> > > > +extern void halt(void);
> > > > +
> > > > +bool start_thread(int cpu_id, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3);
> > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3);
> > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3);
> > > nit: kvm-unit-tests likes to use 'extern' on function
> > > declarations
> > Ok, I'll add this
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +#endif /* _ASMPOWERPC_SMP_H_ */
> > > > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > > > new file mode 100644
> > > > index 0000000..8968907
> > > > --- /dev/null
> > > > +++ b/lib/powerpc/smp.c
> > > > @@ -0,0 +1,115 @@
> > > > +/*
> > > > + * Secondary cpu support
> > > > + *
> > > > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > > > + *
> > > > + * This work is licensed under the terms of the GNU LGPL,
> > > > version
> > > > 2.
> > > > + */
> > > > +
> > > > +#include <stdint.h>
> > > same nit as above
> > > 
> > > > 
> > > > 
> > > > +#include <libfdt/libfdt.h>
> > > > +#include <libfdt/fdt.h>
> > > nit: only need libfdt/libfdt.h, as that includes fdt.h
> > > 
> > > > 
> > > > 
> > > > +#include <devicetree.h>
> > > > +#include <libcflat.h>
> > > > +#include <string.h>
> > > > +#include <asm/rtas.h>
> > > > +#include <asm/smp.h>
> > > > +
> > > > +struct secondary_entry_data {
> > > > +	secondary_entry_fn entry;
> > > > +	uint64_t r3;
> > > > +	bool init_failed;
> > > > +};
> > > > +
> > > > +/*
> > > > + * Start stopped thread cpu_id at entry
> > > > + * Returns:	1 on success or cpu not in stopped state
> > > > + *		0 on failure to start stopped cpu
> > > Returns: TRUE  ...
> > >          FALSE ...
> > Same thing, but ok
> > > 
> > > 
> > > > 
> > > > 
> > > > + *
> > > > + * Note: This function returns 1 on success in starting a
> > > > stopped
> > > > cpu or if the
> > > returns true
> > > 
> > > > 
> > > > 
> > > > + *	 given cpu was not in the stopped state. Thus this
> > > > can
> > > > be called on a
> > > > + *	 list of cpus and all the stopped ones will be
> > > > started
> > > > while false
> > > > + *	 won't be returned if some cpus in that list were
> > > > already running. Thus
> > > > + *	 the user should check that cpus passed to this
> > > > function
> > > > are already in
> > > > + *	 the stopped state if they want to guarantee that a
> > > > return value of
> > > > + *	 true corresponds to the given cpu now executing at
> > > > entry. This
> > > > + *	 function checks again however as calling cpu-start
> > > > on a
> > > > not stopped
> > > > + *	 cpu results in undefined behaviour.
> > > > + */
> > > > +bool 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) {
> > > > +		printf("query-cpu-stopped-state failed for cpu
> > > > %d\n", cpu_id);
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	if (!outputs[0]) {	/* cpu in stopped state */
> > > > +		ret = rtas_call(start_token, 3, 1, NULL,
> > > > cpu_id,
> > > > entry, r3);
> > > > +		if (ret) {
> > > > +			printf("failed to start cpu %d\n",
> > > > cpu_id);
> > > > +			return false;
> > > > +		}
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * Start all stopped threads (vcpus) on cpu_node
> > > > + * Returns:	1 on success
> > > > + *		0 on failure
> > > TRUE/FALSE, but I'd actually change the return type to struct
> > > start_threads;
> > > 
> > >  struct start_threads {
> > >     int nr_threads;
> > >     int nr_started;
> > >  };
> > > 
> > > Then...
> > > 
> > > > 
> > > > 
> > > > + */
> > > > +bool start_cpu(int cpu_node, secondary_entry_fn entry,
> > > > uint32_t
> > > > r3)
> > > > +{
> > > > +	const struct fdt_property *prop;
> > > > +	int len, nr_cpu, cpu;
> > > > +	u32 *cpus;
> > > > +	bool ret = true;
> > > > +
> > > > +	/* 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_cpu = len >> 2; /* Divide by 4 since 4 bytes per
> > > > cpu */
> > > > +	cpus = (u32 *)prop->data; /* Array of valid cpu
> > > > numbers */
> > > > +
> > > > +	for (cpu = 0; cpu < nr_cpu && ret; cpu++)
> > > > +		ret = start_thread(fdt32_to_cpu(cpus[cpu]),
> > > > entry,
> > > > r3);
> > > > +	return ret;
> > > ...change this to
> > > 
> > >  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++)
> > >      nr_started += start_thread(fdt32_to_cpu(cpus[cpu]), entry,
> > > r3);
> > I guess this fits the more generic use case. Although I can't
> > really
> > see a scenario where the caller wants to start all cpus and
> > continue
> > starting them when one fails, that is if one fails to start you
> > probably might as well return an error code immediately.
> > > 
> > > 
> > >  return (struct start_threads){ nr_threads, nr_started };
> > > 
> > > and...
> > >     
> > > > 
> > > > 
> > > > +}
> > > > +
> > > > +static void start_each_secondary(int fdtnode, u32 regval
> > > > __unused,
> > > > void *info)
> > > > +{
> > > > +	struct secondary_entry_data *datap = info;
> > > > +
> > > > +	datap->init_failed |= !start_cpu(fdtnode, datap-
> > > > >entry,
> > > > datap->r3);
> > > > +}
> > > ...change init_failed to nr_started
> > > 
> > >  start_threads = start_cpu(fdtnode, datap->entry, datap->r3);
> > >  nr_cpus += start_threads.nr_threads; // nr_cpus is global like
> > > ARM
> > > has
> > >  datap->nr_started += start_threads.nr_started;
> > > 
> > > and below just check that datap->nr_started == nr_cpus.
> > nr_cpus is set during setup so it would be possible to just have
> > the
> > above return nr_started and then check this accumulated value
> > against
> > nr_cpu below.
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +/*
> > > > + * Start all stopped cpus on the guest at entry with register
> > > > 3
> > > > set to r3
> > > > + * Returns:	1 on success
> > > > + *		0 on failure
> > > TRUE/FALSE
> > Ok
> > > 
> > > 
> > > > 
> > > > 
> > > > + */
> > > > +bool start_all_cpus(secondary_entry_fn entry, uint32_t r3)
> > > > +{
> > > > +	struct secondary_entry_data data = {
> > > > +		entry,
> > > > +		r3,
> > > > +		false
> > > > +	};
> > > > +	int ret;
> > > > +
> > > > +	ret = dt_for_each_cpu_node(&start_each_secondary,
> > > > (void *)
> > > > &data);
> > > assert(ret == 0)
> > Sounds good
> > > 
> > > 
> > > > 
> > > > 
> > > > +
> > > > +	return !(ret || data.init_failed);
> > > See comment above about setting nr_cpus, and then just confirming
> > > they all
> > > started here instead.
> > I think I'll change the above so that start_cpu returns the number
> > of
> > cpus started (we already know the total number of cpus so the
> > struct is
> > unnecessary), we come in with one cpu already started so I'll check
> > that nr_started == nr_cpu - 1.
> I completely forgot that I wrote code setting up nr_cpus... After
> reading this patch, I actually assumed I hadn't, because I didn't
> recall addressing threads. So is the nr_cpus in setup correct?
nr_cpus in setup is done as just that, the number of cpus, not threads.
Which I didn't realise until I read the code closer.
> Without accounting for threads, then it can't be, can it? Maybe
> this patch series should start by fixing that, and also bumping
> NR_CPUS in lib/powerpc/asm/setup.h up to whatever makes more sense.
Depending on what we want nr_cpus to be. I suggest we keep it as the
number of cpus and I implement this as you initially suggested using
struct start_threads to keep track of the total number of threads and
the number successfully started, checking that the number started is
one less than the total number.
This is how I plan on implementing it currently for the next revision.
> 
> > 
> > > 
> > > 
> > > > 
> > > > 
> > > > +}
> > > > 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)
> > Thanks for the review
> Thanks,
> drew

[kvm-unit-tests PATCH V2 4/4] 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.

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

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/hcall.h |   1 +
 powerpc/Makefile.common |   3 +-
 powerpc/tm.c            | 176 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 ++
 4 files changed, 185 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..7f675ff
--- /dev/null
+++ b/powerpc/tm.c
@@ -0,0 +1,176 @@
+/*
+ * 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 <libfdt/libfdt.h>
+#include <devicetree.h>
+#include <util.h>
+#include <alloc.h>
+#include <asm/hcall.h>
+#include <asm/ppc_asm.h>
+#include <asm/processor.h>
+#include <asm/handlers.h>
+#include <asm/smp.h>
+
+#define US_TO_CYCLES(us)	(us << 9)
+
+/*
+ * Get decrementer value
+ */
+static uint64_t get_dec(void)
+{
+	uint64_t dec = 0;
+
+	asm volatile ( "mfdec %[dec]"	: [dec] "+r" (dec)
+					:
+					:
+		     );
+
+	return dec;
+}
+
+/*
+ * Sleep for <us> micro-seconds (must be less than 4 seconds)
+ */
+static void sleep(uint64_t us)
+{
+	uint64_t expire_time, dec, cycles = US_TO_CYCLES(us);
+
+	if (cycles > 0x7FFFFFFF)
+		cycles = 0x7FFFFFFF;
+
+	if (cycles > (dec = get_dec())) {
+		expire_time = 0x7FFFFFFF + dec - cycles;
+		while (get_dec() < dec)
+			;
+	} else {
+		expire_time = dec - cycles;
+	}
+
+	while (get_dec() > expire_time)
+		;
+}
+
+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:	0 - Failure
+ *		1 - 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 %1\n\t"
+			"mfmsr %0"		: "+r" (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 indefinately. 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: tbegin.\n\t"
+			"beq 2f\n\t"
+			"tsuspend.\n\t"
+			"2: tcheck cr0\n\t"
+			"bf 2,1b"		:
+						:
+						: "cr0"
+		     );
+
+	for (i = 0; i < 500 && pass; i++) {
+		uint64_t rval = h_cede();
+
+		if (rval != H_SUCCESS)
+			pass = false;
+		sleep(5000);
+	}
+
+	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 = false;
+	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 V2 4/4] powerpc/tm: Add a test for H_CEDE while tm suspended

[Thomas Huth]

 Hi,

thanks for contributing powerpc tests to kvm-unit-tests, that's very
welcome!
I've got some remarks / questions on the code, though, see below...

On 10.08.2016 03:59, 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.
> 
> Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>

Maybe add a reference to the CVE number? (I think there was one, wasn't it?)

> ---
>  lib/powerpc/asm/hcall.h |   1 +
>  powerpc/Makefile.common |   3 +-
>  powerpc/tm.c            | 176 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   6 ++
>  4 files changed, 185 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..7f675ff
> --- /dev/null
> +++ b/powerpc/tm.c
> @@ -0,0 +1,176 @@
> +/*
> + * 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 <libfdt/libfdt.h>
> +#include <devicetree.h>
> +#include <util.h>
> +#include <alloc.h>
> +#include <asm/hcall.h>
> +#include <asm/ppc_asm.h>
> +#include <asm/processor.h>
> +#include <asm/handlers.h>
> +#include <asm/smp.h>
> +
> +#define US_TO_CYCLES(us)	(us << 9)

That's maybe true for current systems (so it's OK for this specific
test, I think), but the cleaner way would be to get the
timebase-frequency from the device tree instead. So I'd like to suggest
that you either add some code to read this value from the device tree,
or add at least an appropriate comment here.

> +/*
> + * Get decrementer value
> + */
> +static uint64_t get_dec(void)
> +{
> +	uint64_t dec = 0;
> +
> +	asm volatile ( "mfdec %[dec]"	: [dec] "+r" (dec)

Why "+r"? I think "=r" should be enough here?

> +					:
> +					:

You can also omit the empty lines with ":" above.

> +		     );
> +
> +	return dec;
> +}
> +
> +/*
> + * Sleep for <us> micro-seconds (must be less than 4 seconds)
> + */
> +static void sleep(uint64_t us)

Could you please name that function "usleep" instead? The sleep()
function from the libc is traditionally waiting for seconds, not
microseconds, so that could help to avoid some confusing if you name it
usleep() instead.

> +{
> +	uint64_t expire_time, dec, cycles = US_TO_CYCLES(us);
> +
> +	if (cycles > 0x7FFFFFFF)
> +		cycles = 0x7FFFFFFF;

I'd maybe do an "assert(cycles <= 0x7FFFFFFF)" here instead since
otherwise, the code is not doing what the caller expected.

> +	if (cycles > (dec = get_dec())) {

It's always easier to read of you put that on two lines:

	dec = get_dec();
	if (dec < cycles) {
		 ...

> +		expire_time = 0x7FFFFFFF + dec - cycles;
> +		while (get_dec() < dec)
> +			;
> +	} else {
> +		expire_time = dec - cycles;
> +	}
> +
> +	while (get_dec() > expire_time)
> +		;
> +}
> +
> +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:	0 - Failure
> + *		1 - Success
> + */
> +static bool enable_tm(void)
> +{
> +	uint64_t msr = 0;
> +
> +	asm volatile ( "mfmsr %[msr]"	: [msr] "+r" (msr)

That should be "=r" instead of "+r".

> +					:
> +					:
> +		     );
> +
> +	msr |= (((uint64_t) 1) << 32);
> +
> +	asm volatile (	"mtmsrd %1\n\t"
> +			"mfmsr %0"		: "+r" (msr)
> +						: "r" (msr)

I think you should either use "=r" instead of "+r", or skip the "r"(msr)
input parameter, since the "+" modifier already declares it as
input+output (in the latter case, you've got to change the %1 to %0,
too, obviously).

> +						:
> +		     );
> +
> +	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 indefinately. RCU stalls will be detected on the

s/indefinately/indefinitely/

> + * 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: tbegin.\n\t"
> +			"beq 2f\n\t"
> +			"tsuspend.\n\t"
> +			"2: tcheck cr0\n\t"
> +			"bf 2,1b"		:
> +						:
> +						: "cr0"
> +		     );
> +
> +	for (i = 0; i < 500 && pass; i++) {
> +		uint64_t rval = h_cede();
> +
> +		if (rval != H_SUCCESS)
> +			pass = false;
> +		sleep(5000);
> +	}
> +
> +	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 = false;
> +	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();
> +}

 Thomas

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

[Suraj Jitindar Singh]

On Wed, 2016-08-10 at 11:43 +0200, Thomas Huth wrote:
>  Hi,
> 
> thanks for contributing powerpc tests to kvm-unit-tests, that's very
> welcome!
> I've got some remarks / questions on the code, though, see below...
> 
> On 10.08.2016 03:59, 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.
> > 
> > Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> Maybe add a reference to the CVE number? (I think there was one,
> wasn't it?)
> 
Yeah there is a CVE, I'll reference it
> > 
> > ---
> >  lib/powerpc/asm/hcall.h |   1 +
> >  powerpc/Makefile.common |   3 +-
> >  powerpc/tm.c            | 176
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  powerpc/unittests.cfg   |   6 ++
> >  4 files changed, 185 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..7f675ff
> > --- /dev/null
> > +++ b/powerpc/tm.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * 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 <libfdt/libfdt.h>
> > +#include <devicetree.h>
> > +#include <util.h>
> > +#include <alloc.h>
> > +#include <asm/hcall.h>
> > +#include <asm/ppc_asm.h>
> > +#include <asm/processor.h>
> > +#include <asm/handlers.h>
> > +#include <asm/smp.h>
> > +
> > +#define US_TO_CYCLES(us)	(us << 9)
> That's maybe true for current systems (so it's OK for this specific
> test, I think), but the cleaner way would be to get the
> timebase-frequency from the device tree instead. So I'd like to
> suggest
> that you either add some code to read this value from the device
> tree,
> or add at least an appropriate comment here.
> 
Yeah it makes sense to get this from the device tree, I'll do that.
> > 
> > +/*
> > + * Get decrementer value
> > + */
> > +static uint64_t get_dec(void)
> > +{
> > +	uint64_t dec = 0;
> > +
> > +	asm volatile ( "mfdec %[dec]"	: [dec] "+r" (dec)
> Why "+r"? I think "=r" should be enough here?
Yes, "=" is sufficient
> 
> > 
> > +					:
> > +					:
> You can also omit the empty lines with ":" above.
> 
Will do
> > 
> > +		     );
> > +
> > +	return dec;
> > +}
> > +
> > +/*
> > + * Sleep for <us> micro-seconds (must be less than 4 seconds)
> > + */
> > +static void sleep(uint64_t us)
> Could you please name that function "usleep" instead? The sleep()
> function from the libc is traditionally waiting for seconds, not
> microseconds, so that could help to avoid some confusing if you name
> it
> usleep() instead.
I'll change the name
> 
> > 
> > +{
> > +	uint64_t expire_time, dec, cycles = US_TO_CYCLES(us);
> > +
> > +	if (cycles > 0x7FFFFFFF)
> > +		cycles = 0x7FFFFFFF;
> I'd maybe do an "assert(cycles <= 0x7FFFFFFF)" here instead since
> otherwise, the code is not doing what the caller expected.
That makes sense, will do
> 
> > 
> > +	if (cycles > (dec = get_dec())) {
> It's always easier to read of you put that on two lines:
> 
> 	dec = get_dec();
> 	if (dec < cycles) {
> 		 ...
Ok
> 
> > 
> > +		expire_time = 0x7FFFFFFF + dec - cycles;
> > +		while (get_dec() < dec)
> > +			;
> > +	} else {
> > +		expire_time = dec - cycles;
> > +	}
> > +
> > +	while (get_dec() > expire_time)
> > +		;
> > +}
> > +
> > +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:	0 - Failure
> > + *		1 - Success
> > + */
> > +static bool enable_tm(void)
> > +{
> > +	uint64_t msr = 0;
> > +
> > +	asm volatile ( "mfmsr %[msr]"	: [msr] "+r" (msr)
> That should be "=r" instead of "+r".
Ok
> 
> > 
> > +					:
> > +					:
> > +		     );
> > +
> > +	msr |= (((uint64_t) 1) << 32);
> > +
> > +	asm volatile (	"mtmsrd %1\n\t"
> > +			"mfmsr %0"		: "+r" (msr)
> > +						: "r" (msr)
> I think you should either use "=r" instead of "+r", or skip the
> "r"(msr)
> input parameter, since the "+" modifier already declares it as
> input+output (in the latter case, you've got to change the %1 to %0,
> too, obviously).
Yeah, makes sense
> 
> > 
> > +						:
> > +		     );
> > +
> > +	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 indefinately. RCU stalls will be
> > detected on the
> s/indefinately/indefinitely/
> 
Ok
> > 
> > + * 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: tbegin.\n\t"
> > +			"beq 2f\n\t"
> > +			"tsuspend.\n\t"
> > +			"2: tcheck cr0\n\t"
> > +			"bf 2,1b"		:
> > +						:
> > +						: "cr0"
> > +		     );
> > +
> > +	for (i = 0; i < 500 && pass; i++) {
> > +		uint64_t rval = h_cede();
> > +
> > +		if (rval != H_SUCCESS)
> > +			pass = false;
> > +		sleep(5000);
> > +	}
> > +
> > +	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 = false;
> > +	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();
> > +}
>  Thomas
> 
Thanks for the review, I agree with pretty much all of your comments
and will incorporate them into the next review.

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

[Thomas Huth]

On 10.08.2016 03:59, 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.
> 
> Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
[...]
> +	/*
> +	 * Begin a transaction and guarantee we are in the suspend state
> +	 * before continuing
> +	 */
> +	asm volatile (	"1: tbegin.\n\t"
> +			"beq 2f\n\t"
> +			"tsuspend.\n\t"
> +			"2: tcheck cr0\n\t"
> +			"bf 2,1b"		:
> +						:
> +						: "cr0"
> +		     );

I now also tried to compile your patches, and looks like my standard GCC
cross-compiler does not know about these mnemonics yet:

/tmp/ccGEtgIa.s: Assembler messages:
/tmp/ccGEtgIa.s:96: Error: unrecognized opcode: `tbegin.'
/tmp/ccGEtgIa.s:98: Error: unrecognized opcode: `tsuspend.'
/tmp/ccGEtgIa.s:99: Error: unrecognized opcode: `tcheck'

That's happening with the standard cross-compiler which is shipping with
RHEL 7.2 (GCC 4.8.1 20130717). Could you please use ".long 0x..."
statements with the corresponding opcode values here instead, so that it
also compiles with such older versions of GCC?

 Thanks,
  Thomas

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

[Suraj Jitindar Singh]

On Wed, 2016-08-10 at 13:33 +0200, Thomas Huth wrote:
> On 10.08.2016 03:59, 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.
> > 
> > Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> [...]
> > 
> > +	/*
> > +	 * Begin a transaction and guarantee we are in the suspend
> > state
> > +	 * before continuing
> > +	 */
> > +	asm volatile (	"1: tbegin.\n\t"
> > +			"beq 2f\n\t"
> > +			"tsuspend.\n\t"
> > +			"2: tcheck cr0\n\t"
> > +			"bf 2,1b"		:
> > +						:
> > +						: "cr0"
> > +		     );
> I now also tried to compile your patches, and looks like my standard
> GCC
> cross-compiler does not know about these mnemonics yet:
> 
> /tmp/ccGEtgIa.s: Assembler messages:
> /tmp/ccGEtgIa.s:96: Error: unrecognized opcode: `tbegin.'
> /tmp/ccGEtgIa.s:98: Error: unrecognized opcode: `tsuspend.'
> /tmp/ccGEtgIa.s:99: Error: unrecognized opcode: `tcheck'
> 
> That's happening with the standard cross-compiler which is shipping
> with
> RHEL 7.2 (GCC 4.8.1 20130717). Could you please use ".long 0x..."
> statements with the corresponding opcode values here instead, so that
> it
> also compiles with such older versions of GCC?
I'll fix this up, thanks for the catch
> 
>  Thanks,
>   Thomas
> 

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

[Andrew Jones]

On Wed, Aug 10, 2016 at 11:59:37AM +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.
> 
> Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/hcall.h |   1 +
>  powerpc/Makefile.common |   3 +-
>  powerpc/tm.c            | 176 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   6 ++
>  4 files changed, 185 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..7f675ff
> --- /dev/null
> +++ b/powerpc/tm.c
> @@ -0,0 +1,176 @@
> +/*
> + * 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 <libfdt/libfdt.h>
> +#include <devicetree.h>
> +#include <util.h>
> +#include <alloc.h>

above four includes aren't used

> +#include <asm/hcall.h>
> +#include <asm/ppc_asm.h>

ppc_asm.h unused

> +#include <asm/processor.h>
> +#include <asm/handlers.h>
> +#include <asm/smp.h>
> +
> +#define US_TO_CYCLES(us)	(us << 9)
> +
> +/*
> + * Get decrementer value
> + */
> +static uint64_t get_dec(void)
> +{
> +	uint64_t dec = 0;
> +
> +	asm volatile ( "mfdec %[dec]"	: [dec] "+r" (dec)
> +					:
> +					:
> +		     );
> +
> +	return dec;
> +}
> +
> +/*
> + * Sleep for <us> micro-seconds (must be less than 4 seconds)
> + */
> +static void sleep(uint64_t us)
> +{
> +	uint64_t expire_time, dec, cycles = US_TO_CYCLES(us);
> +
> +	if (cycles > 0x7FFFFFFF)
> +		cycles = 0x7FFFFFFF;
> +
> +	if (cycles > (dec = get_dec())) {
> +		expire_time = 0x7FFFFFFF + dec - cycles;
> +		while (get_dec() < dec)
> +			;
> +	} else {
> +		expire_time = dec - cycles;
> +	}
> +
> +	while (get_dec() > expire_time)
> +		;
> +}

You may want to rename this to udelay (like the kernel's naming and
make them available to all powerpc unit tests by adding them to
processor.c. See (not yet merged) patch [*] where I do that for ARM.

[*] https://github.com/rhdrjones/kvm-unit-tests/commit/40f1d91930b3deebdd81d59438083ebcf2ac49e6

> +
> +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:	0 - Failure
> + *		1 - Success

TRUE/FALSE

> + */
> +static bool enable_tm(void)
> +{
> +	uint64_t msr = 0;
> +
> +	asm volatile ( "mfmsr %[msr]"	: [msr] "+r" (msr)
> +					:
> +					:
> +		     );
> +
> +	msr |= (((uint64_t) 1) << 32);
> +
> +	asm volatile (	"mtmsrd %1\n\t"
> +			"mfmsr %0"		: "+r" (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 indefinately. 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: tbegin.\n\t"
> +			"beq 2f\n\t"
> +			"tsuspend.\n\t"
> +			"2: tcheck cr0\n\t"
> +			"bf 2,1b"		:
> +						:
> +						: "cr0"
> +		     );
> +
> +	for (i = 0; i < 500 && pass; i++) {
> +		uint64_t rval = h_cede();
> +
> +		if (rval != H_SUCCESS)
> +			pass = false;
> +		sleep(5000);
> +	}
> +
> +	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 = false;

nit: initialization unnecessary

> +	int i;
> +
> +	report_prefix_push("tm");
> +
> +	all = (argc == 1 || (argc == 2 && !strcmp(argv[1], "all")));

nit: outer braces unnecessary (and not preferred)

> +
> +	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
>

Thanks,
drew 

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

[Suraj Jitindar Singh]

On Fri, 2016-08-12 at 19:19 +0200, Andrew Jones wrote:
> On Wed, Aug 10, 2016 at 11:59:37AM +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.
> > 
> > Based on initial work done by: Cyril Bur <cyril.bur@au1.ibm.com>
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/hcall.h |   1 +
> >  powerpc/Makefile.common |   3 +-
> >  powerpc/tm.c            | 176
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  powerpc/unittests.cfg   |   6 ++
> >  4 files changed, 185 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..7f675ff
> > --- /dev/null
> > +++ b/powerpc/tm.c
> > @@ -0,0 +1,176 @@
> > +/*
> > + * 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 <libfdt/libfdt.h>
> > +#include <devicetree.h>
> > +#include <util.h>
> > +#include <alloc.h>
> above four includes aren't used
Looks like these are left over from when I had all the code in this
file, I'll remove these.
> 
> > 
> > +#include <asm/hcall.h>
> > +#include <asm/ppc_asm.h>
> ppc_asm.h unused
Ok
> 
> > 
> > +#include <asm/processor.h>
> > +#include <asm/handlers.h>
> > +#include <asm/smp.h>
> > +
> > +#define US_TO_CYCLES(us)	(us << 9)
> > +
> > +/*
> > + * Get decrementer value
> > + */
> > +static uint64_t get_dec(void)
> > +{
> > +	uint64_t dec = 0;
> > +
> > +	asm volatile ( "mfdec %[dec]"	: [dec] "+r" (dec)
> > +					:
> > +					:
> > +		     );
> > +
> > +	return dec;
> > +}
> > +
> > +/*
> > + * Sleep for <us> micro-seconds (must be less than 4 seconds)
> > + */
> > +static void sleep(uint64_t us)
> > +{
> > +	uint64_t expire_time, dec, cycles = US_TO_CYCLES(us);
> > +
> > +	if (cycles > 0x7FFFFFFF)
> > +		cycles = 0x7FFFFFFF;
> > +
> > +	if (cycles > (dec = get_dec())) {
> > +		expire_time = 0x7FFFFFFF + dec - cycles;
> > +		while (get_dec() < dec)
> > +			;
> > +	} else {
> > +		expire_time = dec - cycles;
> > +	}
> > +
> > +	while (get_dec() > expire_time)
> > +		;
> > +}
> You may want to rename this to udelay (like the kernel's naming and
> make them available to all powerpc unit tests by adding them to
> processor.c. See (not yet merged) patch [*] where I do that for ARM.
> 
> [*] https://github.com/rhdrjones/kvm-unit-
> tests/commit/40f1d91930b3deebdd81d59438083ebcf2ac49e6
I'll take a look. That name is much better.
> 
> > 
> > +
> > +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:	0 - Failure
> > + *		1 - Success
> TRUE/FALSE
Ok
> 
> > 
> > + */
> > +static bool enable_tm(void)
> > +{
> > +	uint64_t msr = 0;
> > +
> > +	asm volatile ( "mfmsr %[msr]"	: [msr] "+r" (msr)
> > +					:
> > +					:
> > +		     );
> > +
> > +	msr |= (((uint64_t) 1) << 32);
> > +
> > +	asm volatile (	"mtmsrd %1\n\t"
> > +			"mfmsr %0"		: "+r" (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 indefinately. 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: tbegin.\n\t"
> > +			"beq 2f\n\t"
> > +			"tsuspend.\n\t"
> > +			"2: tcheck cr0\n\t"
> > +			"bf 2,1b"		:
> > +						:
> > +						: "cr0"
> > +		     );
> > +
> > +	for (i = 0; i < 500 && pass; i++) {
> > +		uint64_t rval = h_cede();
> > +
> > +		if (rval != H_SUCCESS)
> > +			pass = false;
> > +		sleep(5000);
> > +	}
> > +
> > +	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 = false;
> nit: initialization unnecessary
Ok
> 
> > 
> > +	int i;
> > +
> > +	report_prefix_push("tm");
> > +
> > +	all = (argc == 1 || (argc == 2 && !strcmp(argv[1],
> > "all")));
> nit: outer braces unnecessary (and not preferred)
Ok
> 
> > 
> > +
> > +	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
Thanks for the review

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

[Radim Krčmář]

2016-08-10 11:59+1000, Suraj Jitindar Singh:
> diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> @@ -74,6 +74,27 @@ generate_test ()
>  
>  	cat scripts/runtime.bash
>  
> +	if grep -qw "nodefault" <<<${args[1]}; then
> +		echo -e "while true; do\n"\
> +			"\tread -p \"Test marked as not to be run by default,"\
> +			"are you sure (Y/N)? \" response\n"\
> +			"\tcase \$response in\n"\
> +			"\t\t\"Y\" | \"y\" | \"Yes\" | \"yes\")\n"\
> +			"\t\t\tbreak\n"\
> +			"\t\t\t;;\n"\
> +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
> +			"\t\t\t;&\n"\
> +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
> +			"\t\t\texit\n"\
> +			"\t\t\t;;\n"\
> +			"\t\t*)\n"\
> +			"\t\t\techo Please select Y or N\n"\
> +			"\t\t\t;;\n"\
> +			"\tesac\n"\
> +			"done"

Uff, this is hard to read.

We do not care much about readability of the standalone script itself,
but the source code should be.  It doesn't have to have be that fancy
with user input either:

  echo 'read -p "$question? (y/N)' response
  echo 'case $response in'
  echo '	Y|y|Yes|yes) break;;'
  echo '	*) exit;;
  echo 'esac'

It's still ugly, what about adding a function to scripts/runtime.bash?
More on that below.

> +		echo "standalone=\"true\""

We already have $STANDALONE,

  echo "export STANDALONE=yes"

> diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> @@ -48,10 +48,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 &&
> +            ([ -z $standalone ] || [ $standalone != "true" ]); then

Continuing the idea about a function:  This can be replaced with

  if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups && skip_nodefault;

with skip_nodefault defined earlier; It is not a horrible loss to load
more code in the normal run,

  skip_nodefault () {
  	[ "$STANDALONE" != yes ] && return true

  	# code ask the question and handle responses -- can be a fancier
  	# now, that it actually is readable
  }

That said, I am not a huge fan of user interaction in tests ...
What is the targeted use-case?

The user has already specifically called this test, ./host_killer, so
asking for confirmation is implying that the user is a monkey.

If the test was scripted, then we forced something like
`yes | ./host_killer`.

> +        echo -e "`SKIP` $testname - (test marked as manual run only)"

Please remove the whitespaced dash " - " from output.

Thanks.

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

[Suraj Jitindar Singh]

On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
> 2016-08-10 11:59+1000, Suraj Jitindar Singh:
> > 
> > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > @@ -74,6 +74,27 @@ generate_test ()
> >  
> >  	cat scripts/runtime.bash
> >  
> > +	if grep -qw "nodefault" <<<${args[1]}; then
> > +		echo -e "while true; do\n"\
> > +			"\tread -p \"Test marked as not to be run
> > by default,"\
> > +			"are you sure (Y/N)? \" response\n"\
> > +			"\tcase \$response in\n"\
> > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
> > \"yes\")\n"\
> > +			"\t\t\tbreak\n"\
> > +			"\t\t\t;;\n"\
> > +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
> > +			"\t\t\t;&\n"\
> > +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
> > +			"\t\t\texit\n"\
> > +			"\t\t\t;;\n"\
> > +			"\t\t*)\n"\
> > +			"\t\t\techo Please select Y or N\n"\
> > +			"\t\t\t;;\n"\
> > +			"\tesac\n"\
> > +			"done"
> Uff, this is hard to read.
> 
> We do not care much about readability of the standalone script
> itself,
> but the source code should be.  It doesn't have to have be that fancy
> with user input either:
> 
>   echo 'read -p "$question? (y/N)' response
>   echo 'case $response in'
>   echo '	Y|y|Yes|yes) break;;'
>   echo '	*) exit;;
>   echo 'esac'
> 
> It's still ugly, what about adding a function to
> scripts/runtime.bash?
> More on that below.
> 
> > 
> > +		echo "standalone=\"true\""
> We already have $STANDALONE,
> 
>   echo "export STANDALONE=yes"
> 
> > 
> > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > @@ -48,10 +48,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 &&
> > +            ([ -z $standalone ] || [ $standalone != "true" ]);
> > then
> Continuing the idea about a function:  This can be replaced with
> 
>   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> skip_nodefault;
> 
> with skip_nodefault defined earlier; It is not a horrible loss to
> load
> more code in the normal run,
> 
>   skip_nodefault () {
>   	[ "$STANDALONE" != yes ] && return true
> 
>   	# code ask the question and handle responses -- can be a
> fancier
>   	# now, that it actually is readable
>   }
> 
> That said, I am not a huge fan of user interaction in tests ...
> What is the targeted use-case?
The idea was basically to add the option to mark a test as not to
be run by default when invoking run_tests.sh. It was then suggested
on a previous version of this series that when invoked as a standalone
test the user be prompted to confirm that they actually want to
run the test.

Since there may be tests which can have a detrimental effect on the
host system or some other unintended side effect I thought it better to
require the user specifically invoke them.
> 
> The user has already specifically called this test, ./host_killer, so
> asking for confirmation is implying that the user is a monkey.
> 
> If the test was scripted, then we forced something like
> `yes | ./host_killer`.
I agree in hindsight that it doesn't make much sense to have the user
confirm that they want to run a test that they have specifically
invoked. That being said it's possible that someone running it may not
know that it has potentially negative effects on the host.

I think it might be better to have tests in the nodefault group require
explicit selection by the "-g" parameter when running through
run_tests.sh (current effect of series), while when a test is run
standalone just run it without any additional user input (different to
current operation) and assume the user knows what they are doing. Do
you agree with this?
> 
> > 
> > +        echo -e "`SKIP` $testname - (test marked as manual run
> > only)"
> Please remove the whitespaced dash " - " from output.
> 
Will Fix
> Thanks.
Thanks for the comments

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

[Andrew Jones]

On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote:
> On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
> > 2016-08-10 11:59+1000, Suraj Jitindar Singh:
> > > 
> > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> > > @@ -74,6 +74,27 @@ generate_test ()
> > >  
> > >  	cat scripts/runtime.bash
> > >  
> > > +	if grep -qw "nodefault" <<<${args[1]}; then
> > > +		echo -e "while true; do\n"\
> > > +			"\tread -p \"Test marked as not to be run
> > > by default,"\
> > > +			"are you sure (Y/N)? \" response\n"\
> > > +			"\tcase \$response in\n"\
> > > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
> > > \"yes\")\n"\
> > > +			"\t\t\tbreak\n"\
> > > +			"\t\t\t;;\n"\
> > > +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
> > > +			"\t\t\t;&\n"\
> > > +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
> > > +			"\t\t\texit\n"\
> > > +			"\t\t\t;;\n"\
> > > +			"\t\t*)\n"\
> > > +			"\t\t\techo Please select Y or N\n"\
> > > +			"\t\t\t;;\n"\
> > > +			"\tesac\n"\
> > > +			"done"
> > Uff, this is hard to read.
> > 
> > We do not care much about readability of the standalone script
> > itself,
> > but the source code should be.  It doesn't have to have be that fancy
> > with user input either:
> > 
> >   echo 'read -p "$question? (y/N)' response
> >   echo 'case $response in'
> >   echo '	Y|y|Yes|yes) break;;'
> >   echo '	*) exit;;
> >   echo 'esac'
> > 
> > It's still ugly, what about adding a function to
> > scripts/runtime.bash?
> > More on that below.
> > 
> > > 
> > > +		echo "standalone=\"true\""
> > We already have $STANDALONE,
> > 
> >   echo "export STANDALONE=yes"
> > 
> > > 
> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > @@ -48,10 +48,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 &&
> > > +            ([ -z $standalone ] || [ $standalone != "true" ]);
> > > then
> > Continuing the idea about a function:  This can be replaced with
> > 
> >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> > skip_nodefault;
> > 
> > with skip_nodefault defined earlier; It is not a horrible loss to
> > load
> > more code in the normal run,
> > 
> >   skip_nodefault () {
> >   	[ "$STANDALONE" != yes ] && return true
> > 
> >   	# code ask the question and handle responses -- can be a
> > fancier
> >   	# now, that it actually is readable
> >   }
> > 
> > That said, I am not a huge fan of user interaction in tests ...
> > What is the targeted use-case?
> The idea was basically to add the option to mark a test as not to
> be run by default when invoking run_tests.sh. It was then suggested
> on a previous version of this series that when invoked as a standalone
> test the user be prompted to confirm that they actually want to
> run the test.
> 
> Since there may be tests which can have a detrimental effect on the
> host system or some other unintended side effect I thought it better to
> require the user specifically invoke them.
> > 
> > The user has already specifically called this test, ./host_killer, so
> > asking for confirmation is implying that the user is a monkey.
> > 
> > If the test was scripted, then we forced something like
> > `yes | ./host_killer`.
> I agree in hindsight that it doesn't make much sense to have the user
> confirm that they want to run a test that they have specifically
> invoked. That being said it's possible that someone running it may not
> know that it has potentially negative effects on the host.
> 
> I think it might be better to have tests in the nodefault group require
> explicit selection by the "-g" parameter when running through
> run_tests.sh (current effect of series), while when a test is run
> standalone just run it without any additional user input (different to
> current operation) and assume the user knows what they are doing. Do
> you agree with this?

I disagree. I like the extra protection. The name of the test won't
be "host-killer", it'll be something like "test-obscure-named-feature".
The point of standalone tests is to be able to pass them around easily
and store them for later use. So it's quite likely that the person who
stores it won't be the person who runs it (or the person who stores it
will forget what it does by the time they run it) Anybody who wants to
avoid the prompt can simply wrap the standalone script in another one

cat <<EOF > set-trap-for-unsuspecting-users
#/bin/bash
yes | ./test-obscure-named-feature
EOF


We could also add a couple standard options to standalone tests,
-h (help - output what the test does, warn about crashing hosts, etc.)
-y (yes  - say yes at any prompts)

-h would take its text from the unittests.cfg file (we'd add a new
unit test property called 'help' there)

Thanks,
drew

> > 
> > > 
> > > +        echo -e "`SKIP` $testname - (test marked as manual run
> > > only)"
> > Please remove the whitespaced dash " - " from output.
> > 
> Will Fix
> > Thanks.
> Thanks for the comments
> --
> 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 V2 1/4] scripts/runtime: Add ability to mark test as don't run by default

[Radim Krčmář]

2016-08-12 12:00+0200, Andrew Jones:
> On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote:
>> On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
>> > 2016-08-10 11:59+1000, Suraj Jitindar Singh:
>> > > 
>> > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
>> > > @@ -74,6 +74,27 @@ generate_test ()
>> > >  
>> > >  	cat scripts/runtime.bash
>> > >  
>> > > +	if grep -qw "nodefault" <<<${args[1]}; then
>> > > +		echo -e "while true; do\n"\
>> > > +			"\tread -p \"Test marked as not to be run
>> > > by default,"\
>> > > +			"are you sure (Y/N)? \" response\n"\
>> > > +			"\tcase \$response in\n"\
>> > > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
>> > > \"yes\")\n"\
>> > > +			"\t\t\tbreak\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
>> > > +			"\t\t\t;&\n"\
>> > > +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
>> > > +			"\t\t\texit\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\t\t*)\n"\
>> > > +			"\t\t\techo Please select Y or N\n"\
>> > > +			"\t\t\t;;\n"\
>> > > +			"\tesac\n"\
>> > > +			"done"
>> > Uff, this is hard to read.
>> > 
>> > We do not care much about readability of the standalone script
>> > itself,
>> > but the source code should be.  It doesn't have to have be that fancy
>> > with user input either:
>> > 
>> >   echo 'read -p "$question? (y/N)' response
>> >   echo 'case $response in'
>> >   echo '	Y|y|Yes|yes) break;;'
>> >   echo '	*) exit;;
>> >   echo 'esac'
>> > 
>> > It's still ugly, what about adding a function to
>> > scripts/runtime.bash?
>> > More on that below.
>> > 
>> > > 
>> > > +		echo "standalone=\"true\""
>> > We already have $STANDALONE,
>> > 
>> >   echo "export STANDALONE=yes"
>> > 
>> > > 
>> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
>> > > @@ -48,10 +48,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 &&
>> > > +            ([ -z $standalone ] || [ $standalone != "true" ]);
>> > > then
>> > Continuing the idea about a function:  This can be replaced with
>> > 
>> >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
>> > skip_nodefault;
>> > 
>> > with skip_nodefault defined earlier; It is not a horrible loss to
>> > load
>> > more code in the normal run,
>> > 
>> >   skip_nodefault () {
>> >   	[ "$STANDALONE" != yes ] && return true
>> > 
>> >   	# code ask the question and handle responses -- can be a
>> > fancier
>> >   	# now, that it actually is readable
>> >   }
>> > 
>> > That said, I am not a huge fan of user interaction in tests ...
>> > What is the targeted use-case?
>> The idea was basically to add the option to mark a test as not to
>> be run by default when invoking run_tests.sh. It was then suggested
>> on a previous version of this series that when invoked as a standalone
>> test the user be prompted to confirm that they actually want to
>> run the test.
>> 
>> Since there may be tests which can have a detrimental effect on the
>> host system or some other unintended side effect I thought it better to
>> require the user specifically invoke them.
>> > 
>> > The user has already specifically called this test, ./host_killer, so
>> > asking for confirmation is implying that the user is a monkey.
>> > 
>> > If the test was scripted, then we forced something like
>> > `yes | ./host_killer`.
>> I agree in hindsight that it doesn't make much sense to have the user
>> confirm that they want to run a test that they have specifically
>> invoked. That being said it's possible that someone running it may not
>> know that it has potentially negative effects on the host.
>> 
>> I think it might be better to have tests in the nodefault group require
>> explicit selection by the "-g" parameter when running through
>> run_tests.sh (current effect of series), while when a test is run
>> standalone just run it without any additional user input (different to
>> current operation) and assume the user knows what they are doing. Do
>> you agree with this?
> 
> I disagree. I like the extra protection. The name of the test won't
> be "host-killer", it'll be something like "test-obscure-named-feature".
> The point of standalone tests is to be able to pass them around easily
> and store them for later use. So it's quite likely that the person who
> stores it won't be the person who runs it (or the person who stores it
> will forget what it does by the time they run it) Anybody who wants to
> avoid the prompt can simply wrap the standalone script in another one
> 
> cat <<EOF > set-trap-for-unsuspecting-users
> #/bin/bash
> yes | ./test-obscure-named-feature
> EOF

Ok, experience with `yum` made me tolerant. :)
I would go with the check inside scripts/runtime.bash then.

> We could also add a couple standard options to standalone tests,
> -h (help - output what the test does, warn about crashing hosts, etc.)

Sounds nice.
Could also work with `./run_tests.sh -h` to print them all.

> -y (yes  - say yes at any prompts)

What about adding a "-g $group" option to standalone tests instead.?

We could then use

  for test in tests/*; do $test -g $group; done

to run the same tests as

  ./run_test.sh -g $group

> -h would take its text from the unittests.cfg file (we'd add a new
> unit test property called 'help' there)

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

[Andrew Jones]

On Fri, Aug 12, 2016 at 02:06:36PM +0200, Radim Krčmář wrote:
> 2016-08-12 12:00+0200, Andrew Jones:
> > On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh wrote:
> >> On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
> >> > 2016-08-10 11:59+1000, Suraj Jitindar Singh:
> >> > > 
> >> > > diff --git a/scripts/mkstandalone.sh b/scripts/mkstandalone.sh
> >> > > @@ -74,6 +74,27 @@ generate_test ()
> >> > >  
> >> > >  	cat scripts/runtime.bash
> >> > >  
> >> > > +	if grep -qw "nodefault" <<<${args[1]}; then
> >> > > +		echo -e "while true; do\n"\
> >> > > +			"\tread -p \"Test marked as not to be run
> >> > > by default,"\
> >> > > +			"are you sure (Y/N)? \" response\n"\
> >> > > +			"\tcase \$response in\n"\
> >> > > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
> >> > > \"yes\")\n"\
> >> > > +			"\t\t\tbreak\n"\
> >> > > +			"\t\t\t;;\n"\
> >> > > +			"\t\t\"N\" | \"n\" | \"No\" | \"no\")\n"\
> >> > > +			"\t\t\t;&\n"\
> >> > > +			"\t\t\"q\" | \"quit\" | \"exit\")\n"\
> >> > > +			"\t\t\texit\n"\
> >> > > +			"\t\t\t;;\n"\
> >> > > +			"\t\t*)\n"\
> >> > > +			"\t\t\techo Please select Y or N\n"\
> >> > > +			"\t\t\t;;\n"\
> >> > > +			"\tesac\n"\
> >> > > +			"done"
> >> > Uff, this is hard to read.
> >> > 
> >> > We do not care much about readability of the standalone script
> >> > itself,
> >> > but the source code should be.  It doesn't have to have be that fancy
> >> > with user input either:
> >> > 
> >> >   echo 'read -p "$question? (y/N)' response
> >> >   echo 'case $response in'
> >> >   echo '	Y|y|Yes|yes) break;;'
> >> >   echo '	*) exit;;
> >> >   echo 'esac'
> >> > 
> >> > It's still ugly, what about adding a function to
> >> > scripts/runtime.bash?
> >> > More on that below.
> >> > 
> >> > > 
> >> > > +		echo "standalone=\"true\""
> >> > We already have $STANDALONE,
> >> > 
> >> >   echo "export STANDALONE=yes"
> >> > 
> >> > > 
> >> > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> >> > > @@ -48,10 +48,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 &&
> >> > > +            ([ -z $standalone ] || [ $standalone != "true" ]);
> >> > > then
> >> > Continuing the idea about a function:  This can be replaced with
> >> > 
> >> >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups &&
> >> > skip_nodefault;
> >> > 
> >> > with skip_nodefault defined earlier; It is not a horrible loss to
> >> > load
> >> > more code in the normal run,
> >> > 
> >> >   skip_nodefault () {
> >> >   	[ "$STANDALONE" != yes ] && return true
> >> > 
> >> >   	# code ask the question and handle responses -- can be a
> >> > fancier
> >> >   	# now, that it actually is readable
> >> >   }
> >> > 
> >> > That said, I am not a huge fan of user interaction in tests ...
> >> > What is the targeted use-case?
> >> The idea was basically to add the option to mark a test as not to
> >> be run by default when invoking run_tests.sh. It was then suggested
> >> on a previous version of this series that when invoked as a standalone
> >> test the user be prompted to confirm that they actually want to
> >> run the test.
> >> 
> >> Since there may be tests which can have a detrimental effect on the
> >> host system or some other unintended side effect I thought it better to
> >> require the user specifically invoke them.
> >> > 
> >> > The user has already specifically called this test, ./host_killer, so
> >> > asking for confirmation is implying that the user is a monkey.
> >> > 
> >> > If the test was scripted, then we forced something like
> >> > `yes | ./host_killer`.
> >> I agree in hindsight that it doesn't make much sense to have the user
> >> confirm that they want to run a test that they have specifically
> >> invoked. That being said it's possible that someone running it may not
> >> know that it has potentially negative effects on the host.
> >> 
> >> I think it might be better to have tests in the nodefault group require
> >> explicit selection by the "-g" parameter when running through
> >> run_tests.sh (current effect of series), while when a test is run
> >> standalone just run it without any additional user input (different to
> >> current operation) and assume the user knows what they are doing. Do
> >> you agree with this?
> > 
> > I disagree. I like the extra protection. The name of the test won't
> > be "host-killer", it'll be something like "test-obscure-named-feature".
> > The point of standalone tests is to be able to pass them around easily
> > and store them for later use. So it's quite likely that the person who
> > stores it won't be the person who runs it (or the person who stores it
> > will forget what it does by the time they run it) Anybody who wants to
> > avoid the prompt can simply wrap the standalone script in another one
> > 
> > cat <<EOF > set-trap-for-unsuspecting-users
> > #/bin/bash
> > yes | ./test-obscure-named-feature
> > EOF
> 
> Ok, experience with `yum` made me tolerant. :)
> I would go with the check inside scripts/runtime.bash then.
> 
> > We could also add a couple standard options to standalone tests,
> > -h (help - output what the test does, warn about crashing hosts, etc.)
> 
> Sounds nice.
> Could also work with `./run_tests.sh -h` to print them all.

Sounds good.

> 
> > -y (yes  - say yes at any prompts)
> 
> What about adding a "-g $group" option to standalone tests instead.?

I'd rather the concept of group disappear for standalone tests. IMO,
a standalone test isn't a member of a group or of a test framework.
It's just a script with an embedded binary.

> 
> We could then use
> 
>   for test in tests/*; do $test -g $group; done
> 
> to run the same tests as
> 
>   ./run_test.sh -g $group

Being able to run all standalone tests in a group isn't a bad idea,
but to keep the standalone test feel we could provide generated scripts
named group-name.sh that does the above. IOW, I'm OK with adding -g
support to standalone scripts if it stays hidden within another
"just a script"

Suraj,

IMO, you don't need to worry about these ideas (-h, -y, group-name.sh)
for this series. We can do those later. However I'm happy to review
anything you pull together along these lines :-)

Thanks,
drew

> 
> > -h would take its text from the unittests.cfg file (we'd add a new
> > unit test property called 'help' there)
> --
> 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 V2 1/4] scripts/runtime: Add ability to mark test as don't run by default

[Suraj Jitindar Singh]

On Fri, 2016-08-12 at 14:58 +0200, Andrew Jones wrote:
> On Fri, Aug 12, 2016 at 02:06:36PM +0200, Radim Krčmář wrote:
> > 
> > 2016-08-12 12:00+0200, Andrew Jones:
> > > 
> > > On Fri, Aug 12, 2016 at 04:13:13PM +1000, Suraj Jitindar Singh
> > > wrote:
> > > > 
> > > > On Wed, 2016-08-10 at 15:22 +0200, Radim Krčmář wrote:
> > > > > 
> > > > > 2016-08-10 11:59+1000, Suraj Jitindar Singh:
> > > > > > 
> > > > > > 
> > > > > > diff --git a/scripts/mkstandalone.sh
> > > > > > b/scripts/mkstandalone.sh
> > > > > > @@ -74,6 +74,27 @@ generate_test ()
> > > > > >  
> > > > > >  	cat scripts/runtime.bash
> > > > > >  
> > > > > > +	if grep -qw "nodefault" <<<${args[1]}; then
> > > > > > +		echo -e "while true; do\n"\
> > > > > > +			"\tread -p \"Test marked as not to
> > > > > > be run
> > > > > > by default,"\
> > > > > > +			"are you sure (Y/N)? \"
> > > > > > response\n"\
> > > > > > +			"\tcase \$response in\n"\
> > > > > > +			"\t\t\"Y\" | \"y\" | \"Yes\" |
> > > > > > \"yes\")\n"\
> > > > > > +			"\t\t\tbreak\n"\
> > > > > > +			"\t\t\t;;\n"\
> > > > > > +			"\t\t\"N\" | \"n\" | \"No\" |
> > > > > > \"no\")\n"\
> > > > > > +			"\t\t\t;&\n"\
> > > > > > +			"\t\t\"q\" | \"quit\" |
> > > > > > \"exit\")\n"\
> > > > > > +			"\t\t\texit\n"\
> > > > > > +			"\t\t\t;;\n"\
> > > > > > +			"\t\t*)\n"\
> > > > > > +			"\t\t\techo Please select Y or
> > > > > > N\n"\
> > > > > > +			"\t\t\t;;\n"\
> > > > > > +			"\tesac\n"\
> > > > > > +			"done"
> > > > > Uff, this is hard to read.
> > > > > 
> > > > > We do not care much about readability of the standalone
> > > > > script
> > > > > itself,
> > > > > but the source code should be.  It doesn't have to have be
> > > > > that fancy
> > > > > with user input either:
> > > > > 
> > > > >   echo 'read -p "$question? (y/N)' response
> > > > >   echo 'case $response in'
> > > > >   echo '	Y|y|Yes|yes) break;;'
> > > > >   echo '	*) exit;;
> > > > >   echo 'esac'
> > > > > 
> > > > > It's still ugly, what about adding a function to
> > > > > scripts/runtime.bash?
> > > > > More on that below.
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > +		echo "standalone=\"true\""
> > > > > We already have $STANDALONE,
> > > > > 
> > > > >   echo "export STANDALONE=yes"
> > > > > 
> > > > > > 
> > > > > > 
> > > > > > diff --git a/scripts/runtime.bash b/scripts/runtime.bash
> > > > > > @@ -48,10 +48,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 &&
> > > > > > +            ([ -z $standalone ] || [ $standalone != "true"
> > > > > > ]);
> > > > > > then
> > > > > Continuing the idea about a function:  This can be replaced
> > > > > with
> > > > > 
> > > > >   if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups
> > > > > &&
> > > > > skip_nodefault;
> > > > > 
> > > > > with skip_nodefault defined earlier; It is not a horrible
> > > > > loss to
> > > > > load
> > > > > more code in the normal run,
> > > > > 
> > > > >   skip_nodefault () {
> > > > >   	[ "$STANDALONE" != yes ] && return true
> > > > > 
> > > > >   	# code ask the question and handle responses -- can
> > > > > be a
> > > > > fancier
> > > > >   	# now, that it actually is readable
> > > > >   }
> > > > > 
> > > > > That said, I am not a huge fan of user interaction in tests
> > > > > ...
> > > > > What is the targeted use-case?
> > > > The idea was basically to add the option to mark a test as not
> > > > to
> > > > be run by default when invoking run_tests.sh. It was then
> > > > suggested
> > > > on a previous version of this series that when invoked as a
> > > > standalone
> > > > test the user be prompted to confirm that they actually want to
> > > > run the test.
> > > > 
> > > > Since there may be tests which can have a detrimental effect on
> > > > the
> > > > host system or some other unintended side effect I thought it
> > > > better to
> > > > require the user specifically invoke them.
> > > > > 
> > > > > 
> > > > > The user has already specifically called this test,
> > > > > ./host_killer, so
> > > > > asking for confirmation is implying that the user is a
> > > > > monkey.
> > > > > 
> > > > > If the test was scripted, then we forced something like
> > > > > `yes | ./host_killer`.
> > > > I agree in hindsight that it doesn't make much sense to have
> > > > the user
> > > > confirm that they want to run a test that they have
> > > > specifically
> > > > invoked. That being said it's possible that someone running it
> > > > may not
> > > > know that it has potentially negative effects on the host.
> > > > 
> > > > I think it might be better to have tests in the nodefault group
> > > > require
> > > > explicit selection by the "-g" parameter when running through
> > > > run_tests.sh (current effect of series), while when a test is
> > > > run
> > > > standalone just run it without any additional user input
> > > > (different to
> > > > current operation) and assume the user knows what they are
> > > > doing. Do
> > > > you agree with this?
> > > I disagree. I like the extra protection. The name of the test
> > > won't
> > > be "host-killer", it'll be something like "test-obscure-named-
> > > feature".
> > > The point of standalone tests is to be able to pass them around
> > > easily
> > > and store them for later use. So it's quite likely that the
> > > person who
> > > stores it won't be the person who runs it (or the person who
> > > stores it
> > > will forget what it does by the time they run it) Anybody who
> > > wants to
> > > avoid the prompt can simply wrap the standalone script in another
> > > one
> > > 
> > > cat <<EOF > set-trap-for-unsuspecting-users
> > > #/bin/bash
> > > yes | ./test-obscure-named-feature
> > > EOF
> > Ok, experience with `yum` made me tolerant. :)
> > I would go with the check inside scripts/runtime.bash then.
> > 
> > > 
> > > We could also add a couple standard options to standalone tests,
> > > -h (help - output what the test does, warn about crashing hosts,
> > > etc.)
> > Sounds nice.
> > Could also work with `./run_tests.sh -h` to print them all.
> Sounds good.
> 
> > 
> > 
> > > 
> > > -y (yes  - say yes at any prompts)
> > What about adding a "-g $group" option to standalone tests
> > instead.?
> I'd rather the concept of group disappear for standalone tests. IMO,
> a standalone test isn't a member of a group or of a test framework.
> It's just a script with an embedded binary.
I agree that it'd be better to keep the idea of groups and standalone
tests separate. You shouldn't have to worry about groups when running a
standalone test.
> 
> > 
> > 
> > We could then use
> > 
> >   for test in tests/*; do $test -g $group; done
> > 
> > to run the same tests as
> > 
> >   ./run_test.sh -g $group
> Being able to run all standalone tests in a group isn't a bad idea,
> but to keep the standalone test feel we could provide generated
> scripts
> named group-name.sh that does the above. IOW, I'm OK with adding -g
> support to standalone scripts if it stays hidden within another
> "just a script"
The idea of being able to run all tests in a given group makes sense.
Although we could see the case with some overlap if a test subscribes
to many different groups, although currently this isn't the case with
any existing tests and may never be the case.
> 
> Suraj,
> 
> IMO, you don't need to worry about these ideas (-h, -y, group-
> name.sh)
> for this series. We can do those later. However I'm happy to review
> anything you pull together along these lines :-)
Alright, for this series I think I'll move the checking into a function
in scripts/runtime.bash and call it a day.

Then I'll look at putting some of the -h and -y functionality into
another series.
> 
> Thanks,
> drew
> 
> > 
> > 
> > > 
> > > -h would take its text from the unittests.cfg file (we'd add a
> > > new
> > > unit test property called 'help' there)
> > --
> > 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