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

[kvm-unit-tests PATCH 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 be set as "nodefault" on order to indicate that
it shouldn't be run be default. Modify runtime.bash such that if one of
these tests is encountered a message will be printed to the user to
indicate that the task was skipped and with instructions on how to run
the test on it's own.

This allows a user to confirm that they want to run a test which has been
marked as not to be run by default for whatever reason by the creator.
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/runtime.bash  | 10 ++++++++++
 x86/unittests.cfg     |  3 +++
 4 files changed, 19 insertions(+)

I was going to have the long error message gated behind
[ $verbose == "yes" ] but this means that when that test is run standalone
it will skip without any obvious indication as to why.
Thus IMO it's better to have the message printed regardless.

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/runtime.bash b/scripts/runtime.bash
index 0503cf0..6bf28fb 100644
--- a/scripts/runtime.bash
+++ b/scripts/runtime.bash
@@ -52,6 +52,16 @@ function run()
         return
     fi
 
+    if grep -q "nodefault" <<<$groups && ! grep -qw "$only_group" <<<$groups; then
+        echo -e "`SKIP` $testname\n" \
+            "Test $testname marked as not to be run by default,\n" \
+            "please ensure that you actually want to run this test\n" \
+            "To run this using run_tests.sh append \"-g $groups\"\n" \
+            "To run this standalone set the only_group parameter\n" \
+            "\"only_group=$groups tests/$testname\""
+        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 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     | 26 ++++++++++++++++++++++++++
 lib/ppc64/asm/handlers.h   |  1 +
 powerpc/Makefile.common    |  1 +
 4 files changed, 36 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..1475052
--- /dev/null
+++ b/lib/powerpc/asm/handlers.h
@@ -0,0 +1,8 @@
+#ifndef _ASMPOWERPC_HANDLERS_H_
+#define _ASMPOWERPC_HANDLERS_H_
+
+#include <asm/ptrace.h>
+
+extern 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..1fb35d7
--- /dev/null
+++ b/lib/powerpc/handlers.c
@@ -0,0 +1,26 @@
+/*
+ * 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(__attribute__ ((unused)) struct pt_regs *regs,
+			__attribute__ ((unused)) void *data)
+{
+	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 2/4] lib/powerpc: Add generic decrementer exception handler

[Andrew Jones]

Please CC the powerpc maintainers on powerpc patches, see
the MAINTAINERS file.

On Fri, Aug 05, 2016 at 05:33:11PM +1000, Suraj Jitindar Singh wrote:
> Add the lib/powerpc/handlers.c file and associated header files as a place
> to implement generic exception handler functions for inclusion in tests.
> 
> Add a generic exception handler for a decrementer (0x900) interrupt which
> will reset the decrementer to its maximum value.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/handlers.h |  8 ++++++++
>  lib/powerpc/handlers.c     | 26 ++++++++++++++++++++++++++
>  lib/ppc64/asm/handlers.h   |  1 +
>  powerpc/Makefile.common    |  1 +
>  4 files changed, 36 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..1475052
> --- /dev/null
> +++ b/lib/powerpc/asm/handlers.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASMPOWERPC_HANDLERS_H_
> +#define _ASMPOWERPC_HANDLERS_H_
> +
> +#include <asm/ptrace.h>
> +
> +extern 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..1fb35d7
> --- /dev/null
> +++ b/lib/powerpc/handlers.c
> @@ -0,0 +1,26 @@
> +/*
> + * 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(__attribute__ ((unused)) struct pt_regs *regs,
> +			__attribute__ ((unused)) void *data)

We have __unused defined in libcflat.h, and my preference is to follow
the variable with it, e.g. 'int foo __unused'

> +{
> +	uint32_t dec = 0x7FFFFFFF;
> +
> +	asm volatile ( "mtdec %0"	:
> +					: "r" (dec)
> +					:
> +		     );
> +}
> diff --git a/lib/ppc64/asm/handlers.h b/lib/ppc64/asm/handlers.h
> new file mode 100644
> index 0000000..92e6fb2
> --- /dev/null
> +++ b/lib/ppc64/asm/handlers.h
> @@ -0,0 +1 @@
> +#include "../../powerpc/asm/handlers.h"
> diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
> index 3f8887d..404194b 100644
> --- a/powerpc/Makefile.common
> +++ b/powerpc/Makefile.common
> @@ -36,6 +36,7 @@ cflatobjs += lib/powerpc/hcall.o
>  cflatobjs += lib/powerpc/setup.o
>  cflatobjs += lib/powerpc/rtas.o
>  cflatobjs += lib/powerpc/processor.o
> +cflatobjs += lib/powerpc/handlers.o
>  
>  FLATLIBS = $(libcflat) $(LIBFDT_archive)
>  %.elf: CFLAGS += $(arch_CFLAGS)
> -- 
> 2.5.5
> 
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Suraj Jitindar Singh]

On Fri, 2016-08-05 at 10:23 +0200, Andrew Jones wrote:
> Please CC the powerpc maintainers on powerpc patches, see
> the MAINTAINERS file.
Woops, didn't see the maintainers file, will be sure to do that on
subsequent patches
> 
> On Fri, Aug 05, 2016 at 05:33:11PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Add the lib/powerpc/handlers.c file and associated header files as
> > a place
> > to implement generic exception handler functions for inclusion in
> > tests.
> > 
> > Add a generic exception handler for a decrementer (0x900) interrupt
> > which
> > will reset the decrementer to its maximum value.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/handlers.h |  8 ++++++++
> >  lib/powerpc/handlers.c     | 26 ++++++++++++++++++++++++++
> >  lib/ppc64/asm/handlers.h   |  1 +
> >  powerpc/Makefile.common    |  1 +
> >  4 files changed, 36 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..1475052
> > --- /dev/null
> > +++ b/lib/powerpc/asm/handlers.h
> > @@ -0,0 +1,8 @@
> > +#ifndef _ASMPOWERPC_HANDLERS_H_
> > +#define _ASMPOWERPC_HANDLERS_H_
> > +
> > +#include <asm/ptrace.h>
> > +
> > +extern 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..1fb35d7
> > --- /dev/null
> > +++ b/lib/powerpc/handlers.c
> > @@ -0,0 +1,26 @@
> > +/*
> > + * 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(__attribute__ ((unused)) struct pt_regs
> > *regs,
> > +			__attribute__ ((unused)) void *data)
> We have __unused defined in libcflat.h, and my preference is to
> follow
> the variable with it, e.g. 'int foo __unused'
Ok, I'll change this
> 
> > 
> > +{
> > +	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)

[kvm-unit-tests PATCH 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 a function "get_secondaries" to start stopped threads of a guest at a
given function location.

Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
---
 lib/powerpc/asm/smp.h   |  8 +++++
 lib/powerpc/smp.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
 lib/ppc64/asm/smp.h     |  1 +
 powerpc/Makefile.common |  1 +
 4 files changed, 96 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..c4e3ad8
--- /dev/null
+++ b/lib/powerpc/asm/smp.h
@@ -0,0 +1,8 @@
+#ifndef _ASMPOWERPC_SMP_H_
+#define _ASMPOWERPC_SMP_H_
+
+extern void halt(void);
+
+extern int get_secondaries(void (* secondary_func)(void));
+
+#endif /* _ASMPOWERPC_SMP_H_ */
diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
new file mode 100644
index 0000000..1f8922e
--- /dev/null
+++ b/lib/powerpc/smp.c
@@ -0,0 +1,86 @@
+/*
+ * Secondary cpu support
+ *
+ * Copyright 2016 Suraj Jitindar Singh, IBM.
+ *
+ * This work is licensed under the terms of the GNU LGPL, version 2.
+ */
+
+#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>
+
+/*
+ * Start stopped secondary threads at secondary_func
+ * Returns:  0 on success
+ * 	    -1 on failure
+ */
+int get_secondaries(void (* secondary_func)(void))
+{
+	int cpu_root_node, cpu_node, query_token, start_token;
+	int ret, outputs[1], nr_cpu, cpu, lenp;
+	const struct fdt_property *prop;
+	u32 *cpus;
+
+	cpu_root_node = fdt_path_offset(dt_fdt(), "/cpus");
+	if (cpu_root_node < 0) {
+		report("cpu root node not found", 0);
+		return -1;
+	}
+
+	query_token = rtas_token("query-cpu-stopped-state");
+	start_token = rtas_token("start-cpu");
+	if (query_token == RTAS_UNKNOWN_SERVICE ||
+			start_token == RTAS_UNKNOWN_SERVICE) {
+		report("rtas token not found", 0);
+		return -1;
+	}
+
+	dt_for_each_subnode(cpu_root_node, cpu_node) {
+		prop = fdt_get_property(dt_fdt(), cpu_node, "device_type", &lenp);
+		/* Not a cpu node */
+		if (prop == NULL || lenp != 4 ||
+				strncmp((char *)prop->data, "cpu", 4))
+			continue;
+
+		/* Get the id array of threads on this cpu */
+		prop = fdt_get_property(dt_fdt(), cpu_node,
+				"ibm,ppc-interrupt-server#s", &lenp);
+		if (!prop) {
+			report("ibm,ppc-interrupt-server#s prop not found"
+					, 0);
+			return -1;
+		}
+
+		nr_cpu = lenp >> 2;	/* Divide by 4 since 4 bytes per cpu */
+		cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
+
+		/* Iterate over valid cpus to see if they are stopped */
+		for (cpu = 0; cpu < nr_cpu; cpu++) {
+			int cpu_id = fdt32_to_cpu(cpus[cpu]);
+			/* Query cpu status */
+			ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
+			/* RTAS call failed */
+			if (ret)
+				goto RTAS_FAILED;
+			/* cpu is stopped, start it */
+			if (!*outputs) {
+				ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
+						secondary_func,	cpu_id);
+				/* RTAS call failed */
+				if (ret)
+					goto RTAS_FAILED;
+			}
+		}
+	}
+
+	return 0;
+
+RTAS_FAILED:
+	report("RTAS call failed", 0);
+	return -1;
+}
diff --git a/lib/ppc64/asm/smp.h b/lib/ppc64/asm/smp.h
new file mode 100644
index 0000000..67ced75
--- /dev/null
+++ b/lib/ppc64/asm/smp.h
@@ -0,0 +1 @@
+#include "../../powerpc/asm/smp.h"
diff --git a/powerpc/Makefile.common b/powerpc/Makefile.common
index 404194b..677030a 100644
--- a/powerpc/Makefile.common
+++ b/powerpc/Makefile.common
@@ -37,6 +37,7 @@ cflatobjs += lib/powerpc/setup.o
 cflatobjs += lib/powerpc/rtas.o
 cflatobjs += lib/powerpc/processor.o
 cflatobjs += lib/powerpc/handlers.o
+cflatobjs += lib/powerpc/smp.o
 
 FLATLIBS = $(libcflat) $(LIBFDT_archive)
 %.elf: CFLAGS += $(arch_CFLAGS)
-- 
2.5.5

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

[Andrew Jones]

On Fri, Aug 05, 2016 at 05:33:12PM +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 a function "get_secondaries" to start stopped threads of a guest at a
> given function location.
> 
> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> ---
>  lib/powerpc/asm/smp.h   |  8 +++++
>  lib/powerpc/smp.c       | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
>  lib/ppc64/asm/smp.h     |  1 +
>  powerpc/Makefile.common |  1 +
>  4 files changed, 96 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..c4e3ad8
> --- /dev/null
> +++ b/lib/powerpc/asm/smp.h
> @@ -0,0 +1,8 @@
> +#ifndef _ASMPOWERPC_SMP_H_
> +#define _ASMPOWERPC_SMP_H_
> +
> +extern void halt(void);
> +
> +extern int get_secondaries(void (* secondary_func)(void));
> +
> +#endif /* _ASMPOWERPC_SMP_H_ */
> diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> new file mode 100644
> index 0000000..1f8922e
> --- /dev/null
> +++ b/lib/powerpc/smp.c
> @@ -0,0 +1,86 @@
> +/*
> + * Secondary cpu support
> + *
> + * Copyright 2016 Suraj Jitindar Singh, IBM.
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +
> +#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>
> +
> +/*
> + * Start stopped secondary threads at secondary_func
> + * Returns:  0 on success
> + * 	    -1 on failure
> + */
> +int get_secondaries(void (* secondary_func)(void))
> +{
> +	int cpu_root_node, cpu_node, query_token, start_token;
> +	int ret, outputs[1], nr_cpu, cpu, lenp;
> +	const struct fdt_property *prop;
> +	u32 *cpus;
> +
> +	cpu_root_node = fdt_path_offset(dt_fdt(), "/cpus");
> +	if (cpu_root_node < 0) {
> +		report("cpu root node not found", 0);
> +		return -1;

We only call the report API from unit tests. lib code uses printf.
Also, in general, anything that should never happen (like missing
DT nodes and properties) is probably best using asserts and aborts.

> +	}
> +
> +	query_token = rtas_token("query-cpu-stopped-state");
> +	start_token = rtas_token("start-cpu");
> +	if (query_token == RTAS_UNKNOWN_SERVICE ||
> +			start_token == RTAS_UNKNOWN_SERVICE) {
> +		report("rtas token not found", 0);
> +		return -1;
> +	}
> +
> +	dt_for_each_subnode(cpu_root_node, cpu_node) {
> +		prop = fdt_get_property(dt_fdt(), cpu_node, "device_type", &lenp);
> +		/* Not a cpu node */
> +		if (prop == NULL || lenp != 4 ||
> +				strncmp((char *)prop->data, "cpu", 4))
> +			continue;
> +

Isn't it possible to use dt_for_each_cpu_node? Where the code below is
in the callback?

> +		/* Get the id array of threads on this cpu */
> +		prop = fdt_get_property(dt_fdt(), cpu_node,
> +				"ibm,ppc-interrupt-server#s", &lenp);
> +		if (!prop) {
> +			report("ibm,ppc-interrupt-server#s prop not found"
> +					, 0);
> +			return -1;
> +		}
> +
> +		nr_cpu = lenp >> 2;	/* Divide by 4 since 4 bytes per cpu */
> +		cpus = (u32 *)prop->data; /* Array of valid cpu numbers */
> +
> +		/* Iterate over valid cpus to see if they are stopped */
> +		for (cpu = 0; cpu < nr_cpu; cpu++) {
> +			int cpu_id = fdt32_to_cpu(cpus[cpu]);
> +			/* Query cpu status */
> +			ret = rtas_call(query_token, 1, 2, outputs, cpu_id);
> +			/* RTAS call failed */
> +			if (ret)
> +				goto RTAS_FAILED;
> +			/* cpu is stopped, start it */
> +			if (!*outputs) {
> +				ret = rtas_call(start_token, 3, 1, NULL, cpu_id,
> +						secondary_func,	cpu_id);

Hmm, rtas_calls are generally in unit test territory as they can fail
due to KVM failures. It probably makes sense to return failures for
them, like you do. How about restructuring it though to make sure you
try as much as possible, printing errors as you go.

 int get_secondaries()
 {

  for-each-cpu(cpu) {
     ret = rtas_call(query_token, ...)
     if (ret) {
        printf("query-cpu-stopped-state failed for cpu %d\n);
        failed = true;
        continue;
     }
     ret = rtas_call(start_token, ...)
     if (ret) {
        printf("failed to start cpu %d\n);
        failed = true;
     }
  }

  return failed ? -1 : 0; // get_secondaries could also be bool and then
                          // just return !failed
 }

unit test callers do

 report("starting secondaries", get_secondaries() == 0)

Actually, why call it "get_" secondaries instead of "start_" ?

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

Thanks,
drew 

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

[Suraj Jitindar Singh]

On Fri, 2016-08-05 at 10:54 +0200, Andrew Jones wrote:
> On Fri, Aug 05, 2016 at 05:33:12PM +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 a function "get_secondaries" to start stopped threads of a
> > guest at a
> > given function location.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> >  lib/powerpc/asm/smp.h   |  8 +++++
> >  lib/powerpc/smp.c       | 86
> > +++++++++++++++++++++++++++++++++++++++++++++++++
> >  lib/ppc64/asm/smp.h     |  1 +
> >  powerpc/Makefile.common |  1 +
> >  4 files changed, 96 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..c4e3ad8
> > --- /dev/null
> > +++ b/lib/powerpc/asm/smp.h
> > @@ -0,0 +1,8 @@
> > +#ifndef _ASMPOWERPC_SMP_H_
> > +#define _ASMPOWERPC_SMP_H_
> > +
> > +extern void halt(void);
> > +
> > +extern int get_secondaries(void (* secondary_func)(void));
> > +
> > +#endif /* _ASMPOWERPC_SMP_H_ */
> > diff --git a/lib/powerpc/smp.c b/lib/powerpc/smp.c
> > new file mode 100644
> > index 0000000..1f8922e
> > --- /dev/null
> > +++ b/lib/powerpc/smp.c
> > @@ -0,0 +1,86 @@
> > +/*
> > + * Secondary cpu support
> > + *
> > + * Copyright 2016 Suraj Jitindar Singh, IBM.
> > + *
> > + * This work is licensed under the terms of the GNU LGPL, version
> > 2.
> > + */
> > +
> > +#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>
> > +
> > +/*
> > + * Start stopped secondary threads at secondary_func
> > + * Returns:  0 on success
> > + * 	    -1 on failure
> > + */
> > +int get_secondaries(void (* secondary_func)(void))
> > +{
> > +	int cpu_root_node, cpu_node, query_token, start_token;
> > +	int ret, outputs[1], nr_cpu, cpu, lenp;
> > +	const struct fdt_property *prop;
> > +	u32 *cpus;
> > +
> > +	cpu_root_node = fdt_path_offset(dt_fdt(), "/cpus");
> > +	if (cpu_root_node < 0) {
> > +		report("cpu root node not found", 0);
> > +		return -1;
> We only call the report API from unit tests. lib code uses printf.
> Also, in general, anything that should never happen (like missing
> DT nodes and properties) is probably best using asserts and aborts.
Woops, this got copied when I moved this code from a unit test into
a library file. I'll change these to printfs.
> 
> > 
> > +	}
> > +
> > +	query_token = rtas_token("query-cpu-stopped-state");
> > +	start_token = rtas_token("start-cpu");
> > +	if (query_token == RTAS_UNKNOWN_SERVICE ||
> > +			start_token == RTAS_UNKNOWN_SERVICE) {
> > +		report("rtas token not found", 0);
> > +		return -1;
> > +	}
> > +
> > +	dt_for_each_subnode(cpu_root_node, cpu_node) {
> > +		prop = fdt_get_property(dt_fdt(), cpu_node,
> > "device_type", &lenp);
> > +		/* Not a cpu node */
> > +		if (prop == NULL || lenp != 4 ||
> > +				strncmp((char *)prop->data, "cpu",
> > 4))
> > +			continue;
> > +
> Isn't it possible to use dt_for_each_cpu_node? Where the code below
> is
> in the callback?
I didn't initially use dt_for_each_cpu_node as setting up the callback
function seemed like more effort than it was worth and there is no
mechanism to get a return value back from the function.
But I've thought of a better way to do this using dt_for_each_cpu_node
which I'm going to implement.
> 
> > 
> > +		/* Get the id array of threads on this cpu */
> > +		prop = fdt_get_property(dt_fdt(), cpu_node,
> > +				"ibm,ppc-interrupt-server#s",
> > &lenp);
> > +		if (!prop) {
> > +			report("ibm,ppc-interrupt-server#s prop
> > not found"
> > +					, 0);
> > +			return -1;
> > +		}
> > +
> > +		nr_cpu = lenp >> 2;	/* Divide by 4 since 4
> > bytes per cpu */
> > +		cpus = (u32 *)prop->data; /* Array of valid cpu
> > numbers */
> > +
> > +		/* Iterate over valid cpus to see if they are
> > stopped */
> > +		for (cpu = 0; cpu < nr_cpu; cpu++) {
> > +			int cpu_id = fdt32_to_cpu(cpus[cpu]);
> > +			/* Query cpu status */
> > +			ret = rtas_call(query_token, 1, 2,
> > outputs, cpu_id);
> > +			/* RTAS call failed */
> > +			if (ret)
> > +				goto RTAS_FAILED;
> > +			/* cpu is stopped, start it */
> > +			if (!*outputs) {
> > +				ret = rtas_call(start_token, 3, 1,
> > NULL, cpu_id,
> > +						secondary_func,	
> > cpu_id);
> Hmm, rtas_calls are generally in unit test territory as they can fail
> due to KVM failures. It probably makes sense to return failures for
> them, like you do. How about restructuring it though to make sure you
> try as much as possible, printing errors as you go.
This would make it more readable in the event of a failure, should be
trivial to change, will do.
> 
>  int get_secondaries()
>  {
> 
>   for-each-cpu(cpu) {
>      ret = rtas_call(query_token, ...)
>      if (ret) {
>         printf("query-cpu-stopped-state failed for cpu %d\n);
>         failed = true;
>         continue;
>      }
>      ret = rtas_call(start_token, ...)
>      if (ret) {
>         printf("failed to start cpu %d\n);
>         failed = true;
>      }
>   }
> 
>   return failed ? -1 : 0; // get_secondaries could also be bool and
> then
>                           // just return !failed
>  }
> 
> unit test callers do
> 
>  report("starting secondaries", get_secondaries() == 0)
Will rework it to something like this
> 
> Actually, why call it "get_" secondaries instead of "start_" ?
I agree start sounds better
> 
> > 
> > +				/* RTAS call failed */
> > +				if (ret)
> > +					goto RTAS_FAILED;
> > +			}
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +RTAS_FAILED:
> > +	report("RTAS call failed", 0);
> > +	return -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)

[kvm-unit-tests PATCH 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            | 159 ++++++++++++++++++++++++++++++++++++++++++++++++
 powerpc/unittests.cfg   |   6 ++
 4 files changed, 168 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..64d2ddf
--- /dev/null
+++ b/powerpc/tm.c
@@ -0,0 +1,159 @@
+/*
+ * 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 int 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 this test succeeds then most likely your kernel has the necessary patch.
+ * If it fails, you'll know about it.
+ */
+static void test_h_cede_tm(int argc, char **argv)
+{
+	int i, pass = 1;
+
+	if (argc > 2)
+		report_abort("Unsupported argument: '%s'", argv[2]);
+
+	handle_exception(0x900, &dec_except_handler, NULL);
+
+	if (get_secondaries(&halt))
+		report_abort("Failed to start secondary cpus", 0);
+
+	if (!enable_tm())
+		report_abort("Failed to enable tm", 0);
+
+	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; i++) {
+		uint64_t rval = h_cede();
+
+		if (rval != H_SUCCESS)
+			pass = 0;
+		sleep(5000);
+	}
+
+	report("%s", pass, pass ? "success" : "fail");
+}
+
+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)
+{
+	int all = 0;
+	int i;
+
+	report_prefix_push("tm");
+
+	if (argc == 1 || (argc == 2 && !strcmp(argv[1], "all")))
+		all = 1;
+
+	for (i = 0; hctests[i].name != NULL; i++) {
+		report_prefix_push(hctests[i].name);
+		if (all || strcmp(argv[1], hctests[i].name) == 0) {
+			hctests[i].func(argc, argv);
+		}
+		report_prefix_pop();
+	}
+
+	return report_summary();
+}
diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
index 0098cb6..2819a89 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
-- 
2.5.5

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

[Andrew Jones]

I'll leave most of this to be reviewed by powerpc people. I just have
a couple minor comments.

On Fri, Aug 05, 2016 at 05:33:13PM +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            | 159 ++++++++++++++++++++++++++++++++++++++++++++++++
>  powerpc/unittests.cfg   |   6 ++
>  4 files changed, 168 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..64d2ddf
> --- /dev/null
> +++ b/powerpc/tm.c
> @@ -0,0 +1,159 @@
> +/*
> + * 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
> + */

I don't really mind, but this is a different comment block style
than we use (we're trying to use the kernel coding style). You
can run the kernel's checkpatch on your patches to see if it
complains about anything.

> +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 int enable_tm(void)

Can use bool.

> +{
> +	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 this test succeeds then most likely your kernel has the necessary patch.
> + * If it fails, you'll know about it.
> + */
> +static void test_h_cede_tm(int argc, char **argv)
> +{
> +	int i, pass = 1;

pass can be bool

> +
> +	if (argc > 2)
> +		report_abort("Unsupported argument: '%s'", argv[2]);
> +
> +	handle_exception(0x900, &dec_except_handler, NULL);
> +
> +	if (get_secondaries(&halt))
> +		report_abort("Failed to start secondary cpus", 0);

No need for the ', 0' argument

> +
> +	if (!enable_tm())
> +		report_abort("Failed to enable tm", 0);

No need for the ', 0' argument

> +
> +	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; i++) {
> +		uint64_t rval = h_cede();
> +
> +		if (rval != H_SUCCESS)
> +			pass = 0;

break here?

> +		sleep(5000);
> +	}
> +
> +	report("%s", pass, pass ? "success" : "fail");

Should be

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)
> +{
> +	int all = 0;

all can be bool

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

Or the shorter,

 all = argc == 1 || strcmp(argv[1], "all") == 0;

> +
> +	for (i = 0; hctests[i].name != NULL; i++) {
> +		report_prefix_push(hctests[i].name);
> +		if (all || strcmp(argv[1], hctests[i].name) == 0) {
> +			hctests[i].func(argc, argv);
> +		}
> +		report_prefix_pop();

doesn't really matter but the push/pop should probably directly
wrap the function call inside the if statement

> +	}
> +
> +	return report_summary();
> +}
> diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> index 0098cb6..2819a89 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

We should add another group name here, like 'h_cede_tm', because if we add
other nodefault tests, but only want to run one of them, then the user
should do './run_tests.sh -g h_cede_tm' to run this, and only this, one.

Thanks,
drew

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

[Suraj Jitindar Singh]

On Fri, 2016-08-05 at 11:15 +0200, Andrew Jones wrote:
> I'll leave most of this to be reviewed by powerpc people. I just have
> a couple minor comments.
> 
> On Fri, Aug 05, 2016 at 05:33:13PM +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            | 159
> > ++++++++++++++++++++++++++++++++++++++++++++++++
> >  powerpc/unittests.cfg   |   6 ++
> >  4 files changed, 168 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..64d2ddf
> > --- /dev/null
> > +++ b/powerpc/tm.c
> > @@ -0,0 +1,159 @@
> > +/*
> > + * 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
> > + */
> I don't really mind, but this is a different comment block style
> than we use (we're trying to use the kernel coding style). You
> can run the kernel's checkpatch on your patches to see if it
> complains about anything.
I'll change this to align to kernel style.
> 
> > 
> > +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 int enable_tm(void)
> Can use bool.
Will change this and subsequent comments to the same effect.
> 
> > 
> > +{
> > +	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 this test succeeds then most likely your kernel has the
> > necessary patch.
> > + * If it fails, you'll know about it.
> > + */
> > +static void test_h_cede_tm(int argc, char **argv)
> > +{
> > +	int i, pass = 1;
> pass can be bool
> 
> > 
> > +
> > +	if (argc > 2)
> > +		report_abort("Unsupported argument: '%s'",
> > argv[2]);
> > +
> > +	handle_exception(0x900, &dec_except_handler, NULL);
> > +
> > +	if (get_secondaries(&halt))
> > +		report_abort("Failed to start secondary cpus", 0);
> No need for the ', 0' argument
> 
> > 
> > +
> > +	if (!enable_tm())
> > +		report_abort("Failed to enable tm", 0);
> No need for the ', 0' argument
> 
> > 
> > +
> > +	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; i++) {
> > +		uint64_t rval = h_cede();
> > +
> > +		if (rval != H_SUCCESS)
> > +			pass = 0;
> break here?
Yes
> 
> > 
> > +		sleep(5000);
> > +	}
> > +
> > +	report("%s", pass, pass ? "success" : "fail");
> Should be
> 
> 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)
> > +{
> > +	int all = 0;
> all can be bool
> 
> > 
> > +	int i;
> > +
> > +	report_prefix_push("tm");
> > +
> > +	if (argc == 1 || (argc == 2 && !strcmp(argv[1], "all")))
> > +		all = 1;
> Or the shorter,
> 
>  all = argc == 1 || strcmp(argv[1], "all") == 0;
> 
> > 
> > +
> > +	for (i = 0; hctests[i].name != NULL; i++) {
> > +		report_prefix_push(hctests[i].name);
> > +		if (all || strcmp(argv[1], hctests[i].name) == 0)
> > {
> > +			hctests[i].func(argc, argv);
> > +		}
> > +		report_prefix_pop();
> doesn't really matter but the push/pop should probably directly
> wrap the function call inside the if statement
> 
> > 
> > +	}
> > +
> > +	return report_summary();
> > +}
> > diff --git a/powerpc/unittests.cfg b/powerpc/unittests.cfg
> > index 0098cb6..2819a89 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
> We should add another group name here, like 'h_cede_tm', because if
> we add
> other nodefault tests, but only want to run one of them, then the
> user
> should do './run_tests.sh -g h_cede_tm' to run this, and only this,
> one.
Yes, that's a good idea.
> 
> Thanks,
> drew

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

[Andrew Jones]

On Fri, Aug 05, 2016 at 05:33:10PM +1000, Suraj Jitindar Singh wrote:
> Invoking run_tests.sh without the -g parameter will by default run all of
> the tests for a given architecture. This patch series will add a test which
> has the ability to bring down the host and thus it might be nice if we
> double check that the user actually wants to run that test instead of
> them unknowingly bringing down a machine they might not want to.
> 
> In order to do this add the option for a tests' group parameter in
> unittests.cfg to be set as "nodefault" on order to indicate that
> it shouldn't be run be default. Modify runtime.bash such that if one of
> these tests is encountered a message will be printed to the user to
> indicate that the task was skipped and with instructions on how to run
> the test on it's own.
> 
> This allows a user to confirm that they want to run a test which has been
> marked as not to be run by default for whatever reason by the creator.
> 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/runtime.bash  | 10 ++++++++++
>  x86/unittests.cfg     |  3 +++
>  4 files changed, 19 insertions(+)
> 
> I was going to have the long error message gated behind
> [ $verbose == "yes" ] but this means that when that test is run standalone
> it will skip without any obvious indication as to why.
> Thus IMO it's better to have the message printed regardless.
> 
> 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/runtime.bash b/scripts/runtime.bash
> index 0503cf0..6bf28fb 100644
> --- a/scripts/runtime.bash
> +++ b/scripts/runtime.bash
> @@ -52,6 +52,16 @@ function run()
>          return
>      fi
>  
> +    if grep -q "nodefault" <<<$groups && ! grep -qw "$only_group" <<<$groups; then

I think we want if [ -z "$only_group" ] && grep -qw "nodefault" <<<$groups
The condition above already checks that if $only_group is given that it's
in $groups. Also, please add -w to the grep in that condition above.

> +        echo -e "`SKIP` $testname\n" \
> +            "Test $testname marked as not to be run by default,\n" \
> +            "please ensure that you actually want to run this test\n" \
> +            "To run this using run_tests.sh append \"-g $groups\"\n" \
> +            "To run this standalone set the only_group parameter\n" \
> +            "\"only_group=$groups tests/$testname\""

We prefer one line summaries be output here. How about just adding
"(manual run only - host may crash)" or some such, to the SKIP line.
As for the 'only_group=$groups tests/$testname' standalone instructions.
I think mkstandalone should be modified to check for nodefault and
output a message saying only continue if you're sure, and then wait for
input from the user to continue.

> +        return;
> +    fi
> +
>      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
>          echo "`SKIP` $1 ($arch only)"
>          return 2
> diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> index 60747cf..4a1f74e 100644
> --- a/x86/unittests.cfg
> +++ b/x86/unittests.cfg
> @@ -12,6 +12,9 @@
>  #					# specific to only one.
>  # groups = <group_name1> <group_name2> ...	# Used to identify test cases
>  #						# with run_tests -g ...
> +#						# Specify group_name=nodefault
> +#						# to have test not run by
> +#						# default
>  # accel = kvm|tcg		# Optionally specify if test must run with
>  #				# kvm or tcg. If not specified, then kvm will
>  #				# be used when available.
> -- 
> 2.5.5

Thanks,
drew

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

[Suraj Jitindar Singh]

On Fri, 2016-08-05 at 10:18 +0200, Andrew Jones wrote:
> On Fri, Aug 05, 2016 at 05:33:10PM +1000, Suraj Jitindar Singh wrote:
> > 
> > Invoking run_tests.sh without the -g parameter will by default run
> > all of
> > the tests for a given architecture. This patch series will add a
> > test which
> > has the ability to bring down the host and thus it might be nice if
> > we
> > double check that the user actually wants to run that test instead
> > of
> > them unknowingly bringing down a machine they might not want to.
> > 
> > In order to do this add the option for a tests' group parameter in
> > unittests.cfg to be set as "nodefault" on order to indicate that
> > it shouldn't be run be default. Modify runtime.bash such that if
> > one of
> > these tests is encountered a message will be printed to the user to
> > indicate that the task was skipped and with instructions on how to
> > run
> > the test on it's own.
> > 
> > This allows a user to confirm that they want to run a test which
> > has been
> > marked as not to be run by default for whatever reason by the
> > creator.
> > 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/runtime.bash  | 10 ++++++++++
> >  x86/unittests.cfg     |  3 +++
> >  4 files changed, 19 insertions(+)
> > 
> > I was going to have the long error message gated behind
> > [ $verbose == "yes" ] but this means that when that test is run
> > standalone
> > it will skip without any obvious indication as to why.
> > Thus IMO it's better to have the message printed regardless.
> > 
> > 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/runtime.bash b/scripts/runtime.bash
> > index 0503cf0..6bf28fb 100644
> > --- a/scripts/runtime.bash
> > +++ b/scripts/runtime.bash
> > @@ -52,6 +52,16 @@ function run()
> >          return
> >      fi
> >  
> > +    if grep -q "nodefault" <<<$groups && ! grep -qw "$only_group"
> > <<<$groups; then
> I think we want if [ -z "$only_group" ] && grep -qw "nodefault"
> <<<$groups
> The condition above already checks that if $only_group is given that
> it's
> in $groups. Also, please add -w to the grep in that condition above.
That would make more sense, I'll change this.
> 
> > 
> > +        echo -e "`SKIP` $testname\n" \
> > +            "Test $testname marked as not to be run by default,\n"
> > \
> > +            "please ensure that you actually want to run this
> > test\n" \
> > +            "To run this using run_tests.sh append \"-g
> > $groups\"\n" \
> > +            "To run this standalone set the only_group
> > parameter\n" \
> > +            "\"only_group=$groups tests/$testname\""
> We prefer one line summaries be output here. How about just adding
> "(manual run only - host may crash)" or some such, to the SKIP line.
Ok, I'll make it something like "test marked manual run only" as
this
may have been done for reasons other than the test can bring
down the
host.
> As for the 'only_group=$groups tests/$testname' standalone
> instructions.
> I think mkstandalone should be modified to check for nodefault and
> output a message saying only continue if you're sure, and then wait
> for
> input from the user to continue.
That's a more logical option, this was just the easiest. I'll look
into doing something like this.
When you say mkstandalone should be modified do you mean when
the user runs "make standalone" to build the tests they should be
prompted or when they actually invoke the test they are asked
to confirm that they actually want to run it? Having the
confirmation step on actual test invokation makes more sense IMO.
> 
> > 
> > +        return;
> > +    fi
> > +
> >      if [ -n "$arch" ] && [ "$arch" != "$ARCH" ]; then
> >          echo "`SKIP` $1 ($arch only)"
> >          return 2
> > diff --git a/x86/unittests.cfg b/x86/unittests.cfg
> > index 60747cf..4a1f74e 100644
> > --- a/x86/unittests.cfg
> > +++ b/x86/unittests.cfg
> > @@ -12,6 +12,9 @@
> >  #					# specific to only one.
> >  # groups = <group_name1> <group_name2> ...	# Used to
> > identify test cases
> >  #						# with run_tests
> > -g ...
> > +#						# Specify
> > group_name=nodefault
> > +#						# to have test
> > not run by
> > +#						# default
> >  # accel = kvm|tcg		# Optionally specify if test must
> > run with
> >  #				# kvm or tcg. If not specified,
> > then kvm will
> >  #				# be used when available.

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

[Andrew Jones]

On Mon, Aug 08, 2016 at 01:47:07PM +1000, Suraj Jitindar Singh wrote:
> On Fri, 2016-08-05 at 10:18 +0200, Andrew Jones wrote:
> > On Fri, Aug 05, 2016 at 05:33:10PM +1000, Suraj Jitindar Singh wrote:
> > > 
> > > Invoking run_tests.sh without the -g parameter will by default run
> > > all of
> > > the tests for a given architecture. This patch series will add a
> > > test which
> > > has the ability to bring down the host and thus it might be nice if
> > > we
> > > double check that the user actually wants to run that test instead
> > > of
> > > them unknowingly bringing down a machine they might not want to.
> > > 
> > > In order to do this add the option for a tests' group parameter in
> > > unittests.cfg to be set as "nodefault" on order to indicate that
> > > it shouldn't be run be default. Modify runtime.bash such that if
> > > one of
> > > these tests is encountered a message will be printed to the user to
> > > indicate that the task was skipped and with instructions on how to
> > > run
> > > the test on it's own.
> > > 
> > > This allows a user to confirm that they want to run a test which
> > > has been
> > > marked as not to be run by default for whatever reason by the
> > > creator.
> > > 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/runtime.bash  | 10 ++++++++++
> > >  x86/unittests.cfg     |  3 +++
> > >  4 files changed, 19 insertions(+)
> > > 
> > > I was going to have the long error message gated behind
> > > [ $verbose == "yes" ] but this means that when that test is run
> > > standalone
> > > it will skip without any obvious indication as to why.
> > > Thus IMO it's better to have the message printed regardless.
> > > 
> > > 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/runtime.bash b/scripts/runtime.bash
> > > index 0503cf0..6bf28fb 100644
> > > --- a/scripts/runtime.bash
> > > +++ b/scripts/runtime.bash
> > > @@ -52,6 +52,16 @@ function run()
> > >          return
> > >      fi
> > >  
> > > +    if grep -q "nodefault" <<<$groups && ! grep -qw "$only_group"
> > > <<<$groups; then
> > I think we want if [ -z "$only_group" ] && grep -qw "nodefault"
> > <<<$groups
> > The condition above already checks that if $only_group is given that
> > it's
> > in $groups. Also, please add -w to the grep in that condition above.
> That would make more sense, I'll change this.
> > 
> > > 
> > > +        echo -e "`SKIP` $testname\n" \
> > > +            "Test $testname marked as not to be run by default,\n"
> > > \
> > > +            "please ensure that you actually want to run this
> > > test\n" \
> > > +            "To run this using run_tests.sh append \"-g
> > > $groups\"\n" \
> > > +            "To run this standalone set the only_group
> > > parameter\n" \
> > > +            "\"only_group=$groups tests/$testname\""
> > We prefer one line summaries be output here. How about just adding
> > "(manual run only - host may crash)" or some such, to the SKIP line.
> Ok, I'll make it something like "test marked manual run only" as
> this
> may have been done for reasons other than the test can bring
> down the
> host.
> > As for the 'only_group=$groups tests/$testname' standalone
> > instructions.
> > I think mkstandalone should be modified to check for nodefault and
> > output a message saying only continue if you're sure, and then wait
> > for
> > input from the user to continue.
> That's a more logical option, this was just the easiest. I'll look
> into doing something like this.
> When you say mkstandalone should be modified do you mean when
> the user runs "make standalone" to build the tests they should be
> prompted or when they actually invoke the test they are asked
> to confirm that they actually want to run it? Having the
> confirmation step on actual test invokation makes more sense IMO.

Sorry for the slow response. I've been on vacation. Yes, I meant the
later, when the user runs the test she'll get prompted to continue
for 'nodefault' type tests.

Thanks,
drew

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