| Message ID | 1470382393-24209-4-git-send-email-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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
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