From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Date: Wed, 17 Aug 2016 05:02:19 +0000 Subject: Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit test Message-Id: <1471410139.4989.25.camel@gmail.com> List-Id: References: <1471331895-29887-1-git-send-email-sjitindarsingh@gmail.com> <1471331895-29887-4-git-send-email-sjitindarsingh@gmail.com> <53f79046-582d-3c1b-3d45-104984c75154@redhat.com> In-Reply-To: <53f79046-582d-3c1b-3d45-104984c75154@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: Thomas Huth , kvm@vger.kernel.org Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, drjones@redhat.com On Tue, 2016-08-16 at 14:54 +0200, Thomas Huth wrote: > On 16.08.2016 09:18, Suraj Jitindar Singh wrote: > > > > It would be nice if we had a generic sleep 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 msleep, usleep and sleep in processor.c to sleep for > > a given > > number of milliseconds, microseconds and time base ticks > > respectively. > You finally called them mdelay, udelay and delay instead, so in case > you > respin, please adjust the commit message. Woops, I'll fix those > > > > > Signed-off-by: Suraj Jitindar Singh > > --- > > > > Change Log: > > > > V2 -> V3: > > - Add patch to series > > > > Signed-off-by: Suraj Jitindar Singh > > --- > >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++ > >  lib/powerpc/asm/setup.h     |  2 ++ > >  lib/powerpc/processor.c     | 13 +++++++++++++ > >  lib/powerpc/setup.c         | 12 ++++++++++++ > >  4 files changed, 46 insertions(+) > > > > diff --git a/lib/powerpc/asm/processor.h > > b/lib/powerpc/asm/processor.h > > index 09692bd..9554e08 100644 > > --- a/lib/powerpc/asm/processor.h > > +++ b/lib/powerpc/asm/processor.h > > @@ -1,11 +1,30 @@ > >  #ifndef _ASMPOWERPC_PROCESSOR_H_ > >  #define _ASMPOWERPC_PROCESSOR_H_ > >   > > +#include > >  #include > >   > > +#define cpu_relax() asm volatile ("" : : : "memory") > I'd maybe add a "nop" instruction in there ... just in case? We don't put the nop in anywhere else or in the kernel. I'm gonna leave this. > > > > >  #ifndef __ASSEMBLY__ > >  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..c84bae5 100644 > > --- a/lib/powerpc/processor.c > > +++ b/lib/powerpc/processor.c > > @@ -5,6 +5,7 @@ > >  #include > >  #include > >  #include > > +#include > >   > >  static struct { > >   void (*func)(struct pt_regs *, void *data); > > @@ -36,3 +37,15 @@ 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(); > > + while((get_tb() - start) < cycles) > > + cpu_relax(); > Shouldn't there be some kind of wrap-around detection, like you had > it > in v2 for the decrementer based sleep function? Thanks for catching that, highly unlikely this will overflow (64-bit for timebase vs 31-bit for decrementer) but probably worth adding a check to be sure. > > > > > +} > > + > > +void udelay(uint64_t us) > > +{ > > + delay((us * tb_hz) / 1000000); > > +} > > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c > > index e3d2afa..e7fa525 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; > > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, > > void *info) > >   } > >   cpus[cpu] = regval; > >   > > + /* set timebase frequency */ > > + if (!tb_hz) { > > + const struct fdt_property *prop; > > + u32 *data; > > + prop = fdt_get_property(dt_fdt(), fdtnode, > > + "timebase-frequency", > > NULL); > > + assert(prop != NULL); > > + data = (u32 *)prop->data; > > + tb_hz = fdt32_to_cpu(*data); > You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get > rid > of the "data" variable. Moved this into the code block below were those are both already defined. > > > > > + } > > + > >   /* set exception stack address for this CPU (in SPGR0) */ > >   > >   asm volatile ("mtsprg0 %[addr]" :: > > >  Thomas > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suraj Jitindar Singh Subject: Re: [kvm-unit-tests PATCH V3 4/5] lib/powerpc: Implement generic sleep function for use in unit tests Date: Wed, 17 Aug 2016 15:02:19 +1000 Message-ID: <1471410139.4989.25.camel@gmail.com> References: <1471331895-29887-1-git-send-email-sjitindarsingh@gmail.com> <1471331895-29887-4-git-send-email-sjitindarsingh@gmail.com> <53f79046-582d-3c1b-3d45-104984c75154@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 8bit Cc: pbonzini@redhat.com, rkrcmar@redhat.com, kvm-ppc@vger.kernel.org, lvivier@redhat.com, drjones@redhat.com To: Thomas Huth , kvm@vger.kernel.org Return-path: Received: from mail-pa0-f68.google.com ([209.85.220.68]:36545 "EHLO mail-pa0-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750853AbcHQFQF (ORCPT ); Wed, 17 Aug 2016 01:16:05 -0400 In-Reply-To: <53f79046-582d-3c1b-3d45-104984c75154@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, 2016-08-16 at 14:54 +0200, Thomas Huth wrote: > On 16.08.2016 09:18, Suraj Jitindar Singh wrote: > > > > It would be nice if we had a generic sleep 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 msleep, usleep and sleep in processor.c to sleep for > > a given > > number of milliseconds, microseconds and time base ticks > > respectively. > You finally called them mdelay, udelay and delay instead, so in case > you > respin, please adjust the commit message. Woops, I'll fix those > > > > > Signed-off-by: Suraj Jitindar Singh > > --- > > > > Change Log: > > > > V2 -> V3: > > - Add patch to series > > > > Signed-off-by: Suraj Jitindar Singh > > --- > >  lib/powerpc/asm/processor.h | 19 +++++++++++++++++++ > >  lib/powerpc/asm/setup.h     |  2 ++ > >  lib/powerpc/processor.c     | 13 +++++++++++++ > >  lib/powerpc/setup.c         | 12 ++++++++++++ > >  4 files changed, 46 insertions(+) > > > > diff --git a/lib/powerpc/asm/processor.h > > b/lib/powerpc/asm/processor.h > > index 09692bd..9554e08 100644 > > --- a/lib/powerpc/asm/processor.h > > +++ b/lib/powerpc/asm/processor.h > > @@ -1,11 +1,30 @@ > >  #ifndef _ASMPOWERPC_PROCESSOR_H_ > >  #define _ASMPOWERPC_PROCESSOR_H_ > >   > > +#include > >  #include > >   > > +#define cpu_relax() asm volatile ("" : : : "memory") > I'd maybe add a "nop" instruction in there ... just in case? We don't put the nop in anywhere else or in the kernel. I'm gonna leave this. > > > > >  #ifndef __ASSEMBLY__ > >  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..c84bae5 100644 > > --- a/lib/powerpc/processor.c > > +++ b/lib/powerpc/processor.c > > @@ -5,6 +5,7 @@ > >  #include > >  #include > >  #include > > +#include > >   > >  static struct { > >   void (*func)(struct pt_regs *, void *data); > > @@ -36,3 +37,15 @@ 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(); > > + while((get_tb() - start) < cycles) > > + cpu_relax(); > Shouldn't there be some kind of wrap-around detection, like you had > it > in v2 for the decrementer based sleep function? Thanks for catching that, highly unlikely this will overflow (64-bit for timebase vs 31-bit for decrementer) but probably worth adding a check to be sure. > > > > > +} > > + > > +void udelay(uint64_t us) > > +{ > > + delay((us * tb_hz) / 1000000); > > +} > > diff --git a/lib/powerpc/setup.c b/lib/powerpc/setup.c > > index e3d2afa..e7fa525 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; > > @@ -51,6 +52,17 @@ static void cpu_set(int fdtnode, u32 regval, > > void *info) > >   } > >   cpus[cpu] = regval; > >   > > + /* set timebase frequency */ > > + if (!tb_hz) { > > + const struct fdt_property *prop; > > + u32 *data; > > + prop = fdt_get_property(dt_fdt(), fdtnode, > > + "timebase-frequency", > > NULL); > > + assert(prop != NULL); > > + data = (u32 *)prop->data; > > + tb_hz = fdt32_to_cpu(*data); > You could short-cut that to fdt32_to_cpu(*(u32 *)prop->data) and get > rid > of the "data" variable. Moved this into the code block below were those are both already defined. > > > > > + } > > + > >   /* set exception stack address for this CPU (in SPGR0) */ > >   > >   asm volatile ("mtsprg0 %[addr]" :: > > >  Thomas >