| Message ID | 20190515002801.20517-1-sjitindarsingh@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [kvm-unit-tests,v2,1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp | expand |
On 15/05/2019 02:28, Suraj Jitindar Singh wrote: > Currently the handler for a decrementer exception will simply reload the > maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again. > This means that if a vcpu cedes, it will be ~4 seconds between wakeups. > > The h_cede_tm test is testing a known breakage when a guest cedes while > suspended. To be sure we cede 500 times to check for the bug. However > since it takes ~4 seconds to be woken up once we've ceded, we only get > through ~20 iterations before we reach the 90 seconds timeout and the > test appears to fail. > > Add an option when registering the decrementer handler to specify the > value which should be reloaded by the handler, allowing the timeout to be > chosen. > > Modify the spr test to use the max timeout to preserve existing > behaviour. > Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform > 500 iterations before hitting the 90 second time limit for a test. > > This means the h_cede_tm test now succeeds rather than timing out. > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > --- > > V1 -> V2: > - Make decr variables static > - Load intial decr value in tm test to ensure known value present > --- > lib/powerpc/handlers.c | 7 ++++--- > powerpc/sprs.c | 5 +++-- > powerpc/tm.c | 4 +++- > 3 files changed, 10 insertions(+), 6 deletions(-) > > diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c > index be8226a..c8721e0 100644 > --- a/lib/powerpc/handlers.c > +++ b/lib/powerpc/handlers.c > @@ -12,11 +12,12 @@ > > /* > * Generic handler for decrementer exceptions (0x900) > - * Just reset the decrementer back to its maximum value (0x7FFFFFFF) > + * Just reset the decrementer back to the value specified when registering the > + * handler > */ > -void dec_except_handler(struct pt_regs *regs __unused, void *data __unused) > +void dec_except_handler(struct pt_regs *regs __unused, void *data) > { > - uint32_t dec = 0x7FFFFFFF; > + uint64_t dec = *((uint64_t *) data); > > asm volatile ("mtdec %0" : : "r" (dec)); > } > diff --git a/powerpc/sprs.c b/powerpc/sprs.c > index 6744bd8..0e2e1c9 100644 > --- a/powerpc/sprs.c > +++ b/powerpc/sprs.c > @@ -253,6 +253,7 @@ int main(int argc, char **argv) > 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL, > -1ULL, > }; > + static uint64_t decr = 0x7FFFFFFF; /* Max value */ > > for (i = 1; i < argc; i++) { > if (!strcmp(argv[i], "-w")) { > @@ -288,8 +289,8 @@ int main(int argc, char **argv) > (void) getchar(); > } else { > puts("Sleeping...\n"); > - handle_exception(0x900, &dec_except_handler, NULL); > - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); > + handle_exception(0x900, &dec_except_handler, &decr); > + asm volatile ("mtdec %0" : : "r" (decr)); why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF? > hcall(H_CEDE); > } > > diff --git a/powerpc/tm.c b/powerpc/tm.c > index bd56baa..c588985 100644 > --- a/powerpc/tm.c > +++ b/powerpc/tm.c > @@ -95,11 +95,13 @@ static bool enable_tm(void) > static void test_h_cede_tm(int argc, char **argv) > { > int i; > + static uint64_t decr = 0x3FFFFF; /* ~10ms */ > > if (argc > 2) > report_abort("Unsupported argument: '%s'", argv[2]); > > - handle_exception(0x900, &dec_except_handler, NULL); > + handle_exception(0x900, &dec_except_handler, &decr); > + asm volatile ("mtdec %0" : : "r" (decr)); > > if (!start_all_cpus(halt, 0)) > report_abort("Failed to start secondary cpus"); > Thanks, Laurent
On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote: > On 15/05/2019 02:28, Suraj Jitindar Singh wrote: > > Currently the handler for a decrementer exception will simply > > reload the > > maximum value (0x7FFFFFFF), which will take ~4 seconds to expire > > again. > > This means that if a vcpu cedes, it will be ~4 seconds between > > wakeups. > > > > The h_cede_tm test is testing a known breakage when a guest cedes > > while > > suspended. To be sure we cede 500 times to check for the bug. > > However > > since it takes ~4 seconds to be woken up once we've ceded, we only > > get > > through ~20 iterations before we reach the 90 seconds timeout and > > the > > test appears to fail. > > > > Add an option when registering the decrementer handler to specify > > the > > value which should be reloaded by the handler, allowing the timeout > > to be > > chosen. > > > > Modify the spr test to use the max timeout to preserve existing > > behaviour. > > Modify the h_cede_tm test to use a 10ms timeout to ensure we can > > perform > > 500 iterations before hitting the 90 second time limit for a test. > > > > This means the h_cede_tm test now succeeds rather than timing out. > > > > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > > > --- > > > > V1 -> V2: > > - Make decr variables static > > - Load intial decr value in tm test to ensure known value present > > --- > > lib/powerpc/handlers.c | 7 ++++--- > > powerpc/sprs.c | 5 +++-- > > powerpc/tm.c | 4 +++- > > 3 files changed, 10 insertions(+), 6 deletions(-) > > > > diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c > > index be8226a..c8721e0 100644 > > --- a/lib/powerpc/handlers.c > > +++ b/lib/powerpc/handlers.c > > @@ -12,11 +12,12 @@ > > > > /* > > * Generic handler for decrementer exceptions (0x900) > > - * Just reset the decrementer back to its maximum value > > (0x7FFFFFFF) > > + * Just reset the decrementer back to the value specified when > > registering the > > + * handler > > */ > > -void dec_except_handler(struct pt_regs *regs __unused, void *data > > __unused) > > +void dec_except_handler(struct pt_regs *regs __unused, void *data) > > { > > - uint32_t dec = 0x7FFFFFFF; > > + uint64_t dec = *((uint64_t *) data); > > > > asm volatile ("mtdec %0" : : "r" (dec)); > > } > > diff --git a/powerpc/sprs.c b/powerpc/sprs.c > > index 6744bd8..0e2e1c9 100644 > > --- a/powerpc/sprs.c > > +++ b/powerpc/sprs.c > > @@ -253,6 +253,7 @@ int main(int argc, char **argv) > > 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL, > > -1ULL, > > }; > > + static uint64_t decr = 0x7FFFFFFF; /* Max value */ > > > > for (i = 1; i < argc; i++) { > > if (!strcmp(argv[i], "-w")) { > > @@ -288,8 +289,8 @@ int main(int argc, char **argv) > > (void) getchar(); > > } else { > > puts("Sleeping...\n"); > > - handle_exception(0x900, &dec_except_handler, > > NULL); > > - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); > > + handle_exception(0x900, &dec_except_handler, > > &decr); > > + asm volatile ("mtdec %0" : : "r" (decr)); > > why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF? Oh yeah, my mistake. I mis-read and thought they were the same. Is there any reason it was 0x3FFFFFFF? Can this be fixed up when applying or should I resend? > > > hcall(H_CEDE); > > } > > > > diff --git a/powerpc/tm.c b/powerpc/tm.c > > index bd56baa..c588985 100644 > > --- a/powerpc/tm.c > > +++ b/powerpc/tm.c > > @@ -95,11 +95,13 @@ static bool enable_tm(void) > > static void test_h_cede_tm(int argc, char **argv) > > { > > int i; > > + static uint64_t decr = 0x3FFFFF; /* ~10ms */ > > > > if (argc > 2) > > report_abort("Unsupported argument: '%s'", > > argv[2]); > > > > - handle_exception(0x900, &dec_except_handler, NULL); > > + handle_exception(0x900, &dec_except_handler, &decr); > > + asm volatile ("mtdec %0" : : "r" (decr)); > > > > if (!start_all_cpus(halt, 0)) > > report_abort("Failed to start secondary cpus"); > > > > Thanks, > Laurent
On 16/05/2019 01.27, Suraj Jitindar Singh wrote: > On Wed, 2019-05-15 at 18:22 +0200, Laurent Vivier wrote: >> On 15/05/2019 02:28, Suraj Jitindar Singh wrote: >>> Currently the handler for a decrementer exception will simply >>> reload the >>> maximum value (0x7FFFFFFF), which will take ~4 seconds to expire >>> again. >>> This means that if a vcpu cedes, it will be ~4 seconds between >>> wakeups. >>> >>> The h_cede_tm test is testing a known breakage when a guest cedes >>> while >>> suspended. To be sure we cede 500 times to check for the bug. >>> However >>> since it takes ~4 seconds to be woken up once we've ceded, we only >>> get >>> through ~20 iterations before we reach the 90 seconds timeout and >>> the >>> test appears to fail. >>> >>> Add an option when registering the decrementer handler to specify >>> the >>> value which should be reloaded by the handler, allowing the timeout >>> to be >>> chosen. >>> >>> Modify the spr test to use the max timeout to preserve existing >>> behaviour. >>> Modify the h_cede_tm test to use a 10ms timeout to ensure we can >>> perform >>> 500 iterations before hitting the 90 second time limit for a test. >>> >>> This means the h_cede_tm test now succeeds rather than timing out. >>> >>> Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> >>> >>> --- >>> >>> V1 -> V2: >>> - Make decr variables static >>> - Load intial decr value in tm test to ensure known value present >>> --- >>> lib/powerpc/handlers.c | 7 ++++--- >>> powerpc/sprs.c | 5 +++-- >>> powerpc/tm.c | 4 +++- >>> 3 files changed, 10 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c >>> index be8226a..c8721e0 100644 >>> --- a/lib/powerpc/handlers.c >>> +++ b/lib/powerpc/handlers.c >>> @@ -12,11 +12,12 @@ >>> >>> /* >>> * Generic handler for decrementer exceptions (0x900) >>> - * Just reset the decrementer back to its maximum value >>> (0x7FFFFFFF) >>> + * Just reset the decrementer back to the value specified when >>> registering the >>> + * handler >>> */ >>> -void dec_except_handler(struct pt_regs *regs __unused, void *data >>> __unused) >>> +void dec_except_handler(struct pt_regs *regs __unused, void *data) >>> { >>> - uint32_t dec = 0x7FFFFFFF; >>> + uint64_t dec = *((uint64_t *) data); >>> >>> asm volatile ("mtdec %0" : : "r" (dec)); >>> } >>> diff --git a/powerpc/sprs.c b/powerpc/sprs.c >>> index 6744bd8..0e2e1c9 100644 >>> --- a/powerpc/sprs.c >>> +++ b/powerpc/sprs.c >>> @@ -253,6 +253,7 @@ int main(int argc, char **argv) >>> 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL, >>> -1ULL, >>> }; >>> + static uint64_t decr = 0x7FFFFFFF; /* Max value */ >>> >>> for (i = 1; i < argc; i++) { >>> if (!strcmp(argv[i], "-w")) { >>> @@ -288,8 +289,8 @@ int main(int argc, char **argv) >>> (void) getchar(); >>> } else { >>> puts("Sleeping...\n"); >>> - handle_exception(0x900, &dec_except_handler, >>> NULL); >>> - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); >>> + handle_exception(0x900, &dec_except_handler, >>> &decr); >>> + asm volatile ("mtdec %0" : : "r" (decr)); >> >> why do you replace the 0x3FFFFFFF by decr which is 0x7FFFFFFF? > > Oh yeah, my mistake. I mis-read and thought they were the same. Is > there any reason it was 0x3FFFFFFF? Can this be fixed up when applying > or should I resend? Should be fine to fix this when the patch gets picked up. With the value fixed: Reviewed-by: Thomas Huth <thuth@redhat.com>
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c index be8226a..c8721e0 100644 --- a/lib/powerpc/handlers.c +++ b/lib/powerpc/handlers.c @@ -12,11 +12,12 @@ /* * Generic handler for decrementer exceptions (0x900) - * Just reset the decrementer back to its maximum value (0x7FFFFFFF) + * Just reset the decrementer back to the value specified when registering the + * handler */ -void dec_except_handler(struct pt_regs *regs __unused, void *data __unused) +void dec_except_handler(struct pt_regs *regs __unused, void *data) { - uint32_t dec = 0x7FFFFFFF; + uint64_t dec = *((uint64_t *) data); asm volatile ("mtdec %0" : : "r" (dec)); } diff --git a/powerpc/sprs.c b/powerpc/sprs.c index 6744bd8..0e2e1c9 100644 --- a/powerpc/sprs.c +++ b/powerpc/sprs.c @@ -253,6 +253,7 @@ int main(int argc, char **argv) 0x1234567890ABCDEFULL, 0xFEDCBA0987654321ULL, -1ULL, }; + static uint64_t decr = 0x7FFFFFFF; /* Max value */ for (i = 1; i < argc; i++) { if (!strcmp(argv[i], "-w")) { @@ -288,8 +289,8 @@ int main(int argc, char **argv) (void) getchar(); } else { puts("Sleeping...\n"); - handle_exception(0x900, &dec_except_handler, NULL); - asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); + handle_exception(0x900, &dec_except_handler, &decr); + asm volatile ("mtdec %0" : : "r" (decr)); hcall(H_CEDE); } diff --git a/powerpc/tm.c b/powerpc/tm.c index bd56baa..c588985 100644 --- a/powerpc/tm.c +++ b/powerpc/tm.c @@ -95,11 +95,13 @@ static bool enable_tm(void) static void test_h_cede_tm(int argc, char **argv) { int i; + static uint64_t decr = 0x3FFFFF; /* ~10ms */ if (argc > 2) report_abort("Unsupported argument: '%s'", argv[2]); - handle_exception(0x900, &dec_except_handler, NULL); + handle_exception(0x900, &dec_except_handler, &decr); + asm volatile ("mtdec %0" : : "r" (decr)); if (!start_all_cpus(halt, 0)) report_abort("Failed to start secondary cpus");
Currently the handler for a decrementer exception will simply reload the maximum value (0x7FFFFFFF), which will take ~4 seconds to expire again. This means that if a vcpu cedes, it will be ~4 seconds between wakeups. The h_cede_tm test is testing a known breakage when a guest cedes while suspended. To be sure we cede 500 times to check for the bug. However since it takes ~4 seconds to be woken up once we've ceded, we only get through ~20 iterations before we reach the 90 seconds timeout and the test appears to fail. Add an option when registering the decrementer handler to specify the value which should be reloaded by the handler, allowing the timeout to be chosen. Modify the spr test to use the max timeout to preserve existing behaviour. Modify the h_cede_tm test to use a 10ms timeout to ensure we can perform 500 iterations before hitting the 90 second time limit for a test. This means the h_cede_tm test now succeeds rather than timing out. Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com> --- V1 -> V2: - Make decr variables static - Load intial decr value in tm test to ensure known value present --- lib/powerpc/handlers.c | 7 ++++--- powerpc/sprs.c | 5 +++-- powerpc/tm.c | 4 +++- 3 files changed, 10 insertions(+), 6 deletions(-)