Re: [kvm-unit-tests PATCH v2 1/2] powerpc: Allow for a custom decr value to be specified to load on decr excp

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

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