| Message ID | 20190517130305.32123-2-lvivier@redhat.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [kvm-unit-tests,PULL,1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp | expand |
On 17/05/2019 15.03, Laurent Vivier wrote: > From: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > 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> > Reviewed-by: Thomas Huth <thuth@redhat.com> > Reviewed-by: Laurent Vivier <lvivier@redhat.com> > [lv: reset initial value to 0x3FFFFFFF] Looks like something went wrong here? There is still the 0x7FFFFFFF in the hunk below... > diff --git a/powerpc/sprs.c b/powerpc/sprs.c > index 6744bd8d8049..3c2d98c9ca99 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,7 +289,7 @@ int main(int argc, char **argv) > (void) getchar(); > } else { > puts("Sleeping...\n"); > - handle_exception(0x900, &dec_except_handler, NULL); > + handle_exception(0x900, &dec_except_handler, &decr); > asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); > hcall(H_CEDE); > } Thomas
On Fri, 2019-05-17 at 15:10 +0200, Thomas Huth wrote: > On 17/05/2019 15.03, Laurent Vivier wrote: > > From: Suraj Jitindar Singh <sjitindarsingh@gmail.com> > > > > 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> > > Reviewed-by: Thomas Huth <thuth@redhat.com> > > Reviewed-by: Laurent Vivier <lvivier@redhat.com> > > [lv: reset initial value to 0x3FFFFFFF] > > Looks like something went wrong here? There is still the 0x7FFFFFFF > in > the hunk below... No, I think this is correct. Max value is ox7FFFFFFF, but the initial value we load via mtdec is the original 0x3FFFFFFF. > > > diff --git a/powerpc/sprs.c b/powerpc/sprs.c > > index 6744bd8d8049..3c2d98c9ca99 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,7 +289,7 @@ int main(int argc, char **argv) > > (void) getchar(); > > } else { > > puts("Sleeping...\n"); > > - handle_exception(0x900, &dec_except_handler, > > NULL); > > + handle_exception(0x900, &dec_except_handler, > > &decr); > > asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); > > hcall(H_CEDE); > > } > > Thomas
On 20/05/2019 06:47, Suraj Jitindar Singh wrote: > On Fri, 2019-05-17 at 15:10 +0200, Thomas Huth wrote: >> On 17/05/2019 15.03, Laurent Vivier wrote: >>> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com> >>> >>> 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> >>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>> Reviewed-by: Laurent Vivier <lvivier@redhat.com> >>> [lv: reset initial value to 0x3FFFFFFF] >> >> Looks like something went wrong here? There is still the 0x7FFFFFFF >> in >> the hunk below... > > No, I think this is correct. > Max value is ox7FFFFFFF, but the initial value we load via mtdec is the > original 0x3FFFFFFF. Yes, that's it. Thanks, Laurent
On 20/05/2019 08.12, Laurent Vivier wrote: > On 20/05/2019 06:47, Suraj Jitindar Singh wrote: >> On Fri, 2019-05-17 at 15:10 +0200, Thomas Huth wrote: >>> On 17/05/2019 15.03, Laurent Vivier wrote: >>>> From: Suraj Jitindar Singh <sjitindarsingh@gmail.com> >>>> >>>> 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> >>>> Reviewed-by: Thomas Huth <thuth@redhat.com> >>>> Reviewed-by: Laurent Vivier <lvivier@redhat.com> >>>> [lv: reset initial value to 0x3FFFFFFF] >>> >>> Looks like something went wrong here? There is still the 0x7FFFFFFF >>> in >>> the hunk below... >> >> No, I think this is correct. >> Max value is ox7FFFFFFF, but the initial value we load via mtdec is the >> original 0x3FFFFFFF. > > Yes, that's it. Ok, since we're only calling H_CEDE once here, we should be fine, indeed. Sorry for the confusion. Thomas
diff --git a/lib/powerpc/handlers.c b/lib/powerpc/handlers.c index be8226a2e573..c8721e0a11ae 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 6744bd8d8049..3c2d98c9ca99 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,7 +289,7 @@ int main(int argc, char **argv) (void) getchar(); } else { puts("Sleeping...\n"); - handle_exception(0x900, &dec_except_handler, NULL); + handle_exception(0x900, &dec_except_handler, &decr); asm volatile ("mtdec %0" : : "r" (0x3FFFFFFF)); hcall(H_CEDE); } diff --git a/powerpc/tm.c b/powerpc/tm.c index bd56baa5b3d8..c588985352f4 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");