Re: [kvm-unit-tests PATCH V4 4/5] lib/powerpc: Implement generic delay function for use in unit tests

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

On Wed, 2016-08-17 at 15:04 +0200, Andrew Jones wrote:
> On Wed, Aug 17, 2016 at 04:48:57PM +1000, Suraj Jitindar Singh wrote:
> > 
> > It would be nice if we had a generic delay function which could be
> > used
> > in unit tests, add one.
> > 
> > Add the variable tb_hz used to store the time base frequency which
> > is read
> > from the device tree on setup.
> > 
> > Add functions mdelay, udelay and delay in processor.c to delay for
> > a given
> > number of milliseconds, microseconds and time base ticks
> > respectively.
> > 
> > Signed-off-by: Suraj Jitindar Singh <sjitindarsingh@gmail.com>
> > ---
> > 
> > Change Log:
> > 
> > V2 -> V3:
> > 	- Add patch to series
> > V3 -> V4:
> > 	- Reword sleep->delay
> > 	- Move cpu_relax to asm-generic/barrier.h
> > 	- Add assert to catch when delay fns called with too large
> > values
> > ---
> >  lib/asm-generic/barrier.h   |  4 ++++
> >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++
> >  lib/powerpc/asm/setup.h     |  2 ++
> >  lib/powerpc/processor.c     | 20 ++++++++++++++++++++
> >  lib/powerpc/setup.c         |  7 +++++++
> >  5 files changed, 52 insertions(+)
> > 
> > diff --git a/lib/asm-generic/barrier.h b/lib/asm-generic/barrier.h
> > index 12ae782..6a990ff 100644
> > --- a/lib/asm-generic/barrier.h
> > +++ b/lib/asm-generic/barrier.h
> > @@ -28,4 +28,8 @@
> >  #define smp_wmb()	wmb()
> >  #endif
> >  
> > +#ifndef cpu_relax
> > +#define cpu_relax()	asm volatile ("":::"memory")
> > +#endif
> > +
> >  #endif /* _ASM_BARRIER_H_ */
> > diff --git a/lib/powerpc/asm/processor.h
> > b/lib/powerpc/asm/processor.h
> > index 09692bd..ac001e1 100644
> > --- a/lib/powerpc/asm/processor.h
> > +++ b/lib/powerpc/asm/processor.h
> > @@ -1,6 +1,7 @@
> >  #ifndef _ASMPOWERPC_PROCESSOR_H_
> >  #define _ASMPOWERPC_PROCESSOR_H_
> >  
> > +#include <libcflat.h>
> >  #include <asm/ptrace.h>
> >  
> >  #ifndef __ASSEMBLY__
> > @@ -8,4 +9,22 @@ void handle_exception(int trap, void
> > (*func)(struct pt_regs *, void *), void *);
> >  void do_handle_exception(struct pt_regs *regs);
> >  #endif /* __ASSEMBLY__ */
> >  
> > +static inline uint64_t get_tb(void)
> > +{
> > +	uint64_t tb;
> > +
> > +	asm volatile ("mfspr %[tb],268" : [tb] "=r" (tb));
> > +
> > +	return tb;
> > +}
> > +
> > +extern void delay(uint64_t cycles);
> > +extern void udelay(uint64_t us);
> > +
> > +static inline void mdelay(uint64_t ms)
> > +{
> > +	while (ms--)
> > +		udelay(1000);
> > +}
> > +
> >  #endif /* _ASMPOWERPC_PROCESSOR_H_ */
> > diff --git a/lib/powerpc/asm/setup.h b/lib/powerpc/asm/setup.h
> > index b1e1e5a..23b4156 100644
> > --- a/lib/powerpc/asm/setup.h
> > +++ b/lib/powerpc/asm/setup.h
> > @@ -11,6 +11,8 @@
> >  extern u32 cpus[NR_CPUS];
> >  extern int nr_cpus;
> >  
> > +extern uint64_t tb_hz;
> > +
> >  #define NR_MEM_REGIONS		8
> >  #define MR_F_PRIMARY		(1U << 0)
> >  struct mem_region {
> > diff --git a/lib/powerpc/processor.c b/lib/powerpc/processor.c
> > index a78bc3c..a28f2f0 100644
> > --- a/lib/powerpc/processor.c
> > +++ b/lib/powerpc/processor.c
> > @@ -5,6 +5,8 @@
> >  #include <libcflat.h>
> >  #include <asm/processor.h>
> >  #include <asm/ptrace.h>
> > +#include <asm/setup.h>
> > +#include <asm/barrier.h>
> >  
> >  static struct {
> >  	void (*func)(struct pt_regs *, void *data);
> > @@ -36,3 +38,21 @@ void do_handle_exception(struct pt_regs *regs)
> >  	printf("unhandled cpu exception 0x%lx\n", regs->trap);
> >  	abort();
> >  }
> > +
> > +void delay(uint64_t cycles)
> > +{
> > +	uint64_t start = get_tb();
> > +	/*
> > +	 * Pretty unlikely unless your server has been on for, or
> > you want to
> > +	 * delay for, over 1000 years, but still.
> > +	 */
> > +	assert(cycles < (UINT64_MAX - start));
> > +	while ((get_tb() - start) < cycles)
> I don't think the above assert is necessary. As long as the
> subtraction
> (get_tb() - start) produces a uint64_t, then the condition should
> always
> work - per C99. Maybe it should be written as (uint64_t)(get_tb() -
> start)
> to be 100% correct though.
This is to catch the case where the caller passes a ridiculously large
cycles value (e.g. UINT64_MAX - 1) and/or start is sufficiently large
that get_tb() - start will never be >= to cycles because the time-base
counter will overflow and wrap around before that ever becomes true.
This is super unlikely but will avoid an infinite loop in the event
someone does it.
> 
> > 
> > +		cpu_relax();
> > +}
> > +
> > +void udelay(uint64_t us)
> > +{
> > +	assert(us < (UINT64_MAX / tb_hz));
> Same comment here. I'm pretty sure (wrap around wraps my head, so I
> could be wrong) that the main concern is maintaining unsigned integer
> subtraction, which the C99 guarantees to wrap modulo 2^N, N being the
> number of bits of the unsigned integer.
This is to catch when the caller tries to sleep for > 36000000000us (10
hrs), which I realise is highly unlikely. But in this case us * tb_hz
will be too big to store in a u64. Thus this won't delay for the
intended time, hence the assert.
> 
> > 
> > +	delay((us * tb_hz) / 1000000);
> > +}
> > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c
> > index e3d2afa..65bedf5 100644
> > --- a/lib/powerpc/setup.c
> > +++ b/lib/powerpc/setup.c
> > @@ -24,6 +24,7 @@ extern void setup_args_progname(const char
> > *args);
> >  
> >  u32 cpus[NR_CPUS] = { [0 ... NR_CPUS-1] = (~0U) };
> >  int nr_cpus;
> > +uint64_t tb_hz;
> >  
> >  struct mem_region mem_regions[NR_MEM_REGIONS];
> >  phys_addr_t __physical_start, __physical_end;
> > @@ -72,6 +73,12 @@ static void cpu_set(int fdtnode, u32 regval,
> > void *info)
> >  		data = (u32 *)prop->data;
> >  		params->dcache_bytes = fdt32_to_cpu(*data);
> >  
> > +		prop = fdt_get_property(dt_fdt(), fdtnode,
> > +					"timebase-frequency",
> > NULL);
> > +		assert(prop != NULL);
> > +		data = (u32 *)prop->data;
> > +		tb_hz = fdt32_to_cpu(*data);
> Arguably the dance I do with cpu_set_params to pass values back to
> cpu_init, where they simply get assigned to globals, is pointless.
> It's
> trying to maintain encapsulation (which I violate for nr_cpus
> anyway...)
> That said, I'd like to see tb_hz either integrate with the
> cpu_set_params
> pattern, or a cleanup patch come before this one, which removes
> cpu_set_params, allowing icache/dcache_bytes setting to match the
> tb_hz
> pattern. I won't hold this series up on that though.
Yeah I see whats happening here, I'll make it match what is done for
i/dcache.
> 
> > 
> > +
> >  		read_common_info = true;
> >  	}
> >  }
Thanks