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

[Suraj Jitindar Singh <sjitindarsingh@gmail.com>]

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