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

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

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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
> 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html