* Re: [PATCH v4 1/7] introduce time managment in xtf [not found] <20180410191701.17203-1-semelpaul@gmail.com> @ 2018-04-13 12:05 ` Roger Pau Monné 2018-04-16 10:48 ` Paul Semel [not found] ` <20180410191701.17203-2-semelpaul@gmail.com> ` (5 subsequent siblings) 6 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 12:05 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:16:55PM +0200, Paul Semel wrote: > this file is introduce to be able to implement an inter domain > communication protocol over xenstore. For synchronization purpose, we do > really want to be able to "control" time > > common/time.c: since_boot_time gets the time in nanoseconds from the > moment the VM has booted > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - moved rdtsc to arch/x86/include/arch/lib.h > - added a rdtsc_ordered implementation to serialize rdtsc > - simplified since_boot_time function > - still need to have Andrew's scale_delta version > > arch/x86/include/arch/lib.h | 18 ++++++++++ > build/files.mk | 1 + > common/time.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ > include/xtf/time.h | 24 ++++++++++++++ > 4 files changed, 124 insertions(+) > create mode 100644 common/time.c > create mode 100644 include/xtf/time.h > > diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h > index 0045902..510cdb1 100644 > --- a/arch/x86/include/arch/lib.h > +++ b/arch/x86/include/arch/lib.h > @@ -6,6 +6,7 @@ > #include <xen/arch-x86/xen.h> > #include <arch/desc.h> > #include <arch/msr.h> > +#include <arch/barrier.h> This include list is sorted alphabetically. > > static inline void cpuid(uint32_t leaf, > uint32_t *eax, uint32_t *ebx, > @@ -374,6 +375,23 @@ static inline void write_xcr0(uint64_t xcr0) > xsetbv(0, xcr0); > } > > +static inline uint64_t rdtsc(void) > +{ > + uint32_t lo, hi; > + > + asm volatile("rdtsc": "=a"(lo), "=d"(hi)); > + > + return ((uint64_t)hi << 32) | lo; > +} > + > +static inline uint64_t rdtsc_ordered(void) > +{ > + rmb(); > + mb(); > + > + return rdtsc(); > +} I would likely just add a single function like: static inline uint64_t rdtsc_ordered(void) { uint32_t lo, hi; asm volatile("lfence; mfence; rdtsc" : "=a" (lo), "=d" (hi)); return ((uint64_t)hi << 32) | lo; } Because there's no external caller of rdtsc, but I will leave that to Andrew to decide. In any case this should work fine. > + > #endif /* XTF_X86_LIB_H */ > > /* > diff --git a/build/files.mk b/build/files.mk > index 46b42d6..55ed1ca 100644 > --- a/build/files.mk > +++ b/build/files.mk > @@ -16,6 +16,7 @@ obj-perarch += $(ROOT)/common/libc/vsnprintf.o > obj-perarch += $(ROOT)/common/report.o > obj-perarch += $(ROOT)/common/setup.o > obj-perarch += $(ROOT)/common/xenbus.o > +obj-perarch += $(ROOT)/common/time.o > > obj-perenv += $(ROOT)/arch/x86/decode.o > obj-perenv += $(ROOT)/arch/x86/desc.o > diff --git a/common/time.c b/common/time.c > new file mode 100644 > index 0000000..79abc7e > --- /dev/null > +++ b/common/time.c > @@ -0,0 +1,81 @@ > +#include <xtf/types.h> > +#include <xtf/traps.h> > +#include <xtf/time.h> Alphabetic order please. > +#include <arch/barrier.h> > +#include <arch/lib.h> > + > +/* This function was taken from mini-os source code */ > +/* It returns ((delta << shift) * mul_frac) >> 32 */ Comment has wrong style, please check CODING_STYLE. > +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift) > +{ > + uint64_t product; > +#ifdef __i386__ > + uint32_t tmp1, tmp2; > +#endif > + > + if ( shift < 0 ) > + delta >>= -shift; > + else > + delta <<= shift; > + > +#ifdef __i386__ > + __asm__ ( > + "mul %5 ; " > + "mov %4,%%eax ; " > + "mov %%edx,%4 ; " > + "mul %5 ; " > + "add %4,%%eax ; " > + "xor %5,%5 ; " > + "adc %5,%%edx ; " > + : "=A" (product), "=r" (tmp1), "=r" (tmp2) > + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) ); This line is too long. > +#else > + __asm__ ( > + "mul %%rdx ; shrd $32,%%rdx,%%rax" > + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) ); Not sure whether you need to add a ': "d"' clobber here, since the d register is used but it's not in the list of output operands. > +#endif > + > + return product; > +} > + > + > +uint64_t since_boot_time(void) > +{ > + uint32_t ver1, ver2; > + uint64_t tsc_timestamp, system_time, tsc; > + uint32_t tsc_to_system_mul; > + int8_t tsc_shift; > + > + do > + { > + ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); > + smp_rmb(); > + > + system_time = shared_info.vcpu_info[0].time.system_time; > + tsc_timestamp = shared_info.vcpu_info[0].time.tsc_timestamp; > + tsc_to_system_mul = shared_info.vcpu_info[0].time.tsc_to_system_mul; > + tsc_shift = shared_info.vcpu_info[0].time.tsc_shift; > + tsc = rdtsc_ordered(); > + smp_rmb(); I don't think you need the barrier here if you use rdtsc_ordered, but I would like confirmation from someone else. > + > + ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); > + } while ( ver2 & 1 || ver1 != ver2 ); > + > + > + system_time += scale_delta(tsc - tsc_timestamp, > + tsc_to_system_mul, > + tsc_shift); > + > + return system_time; > +} > + > +/* > + * Local variables: > + * mode: C > + * c-file-style: "BSD" > + * c-basic-offset: 4 > + * tab-width: 4 > + * indent-tabs-mode: nil > + * End: > + */ > diff --git a/include/xtf/time.h b/include/xtf/time.h > new file mode 100644 > index 0000000..8180e07 > --- /dev/null > +++ b/include/xtf/time.h > @@ -0,0 +1,24 @@ > +/** > + * @file include/xtf/time.h > + * > + * Time management > + */ > +#ifndef XTF_TIME_H > +# define XTF_TIME_H > + > +#include <xtf/types.h> > + > +/* Time from boot in nanoseconds */ > +uint64_t since_boot_time(void); since_boot_time looks like a weird name, I would probably name this hypervisor_uptime or system_time, but I'm not specially thrilled anyway. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/7] introduce time managment in xtf 2018-04-13 12:05 ` [PATCH v4 1/7] introduce time managment in xtf Roger Pau Monné @ 2018-04-16 10:48 ` Paul Semel 2018-04-17 7:45 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Paul Semel @ 2018-04-16 10:48 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel On 04/13/2018 02:05 PM, Roger Pau Monné wrote: >> this file is introduce to be able to implement an inter domain >> communication protocol over xenstore. For synchronization purpose, we do >> really want to be able to "control" time >> >> common/time.c: since_boot_time gets the time in nanoseconds from the >> moment the VM has booted >> >> Signed-off-by: Paul Semel <phentex@amazon.de> >> --- >> >> Notes: >> v4: >> - moved rdtsc to arch/x86/include/arch/lib.h >> - added a rdtsc_ordered implementation to serialize rdtsc >> - simplified since_boot_time function >> - still need to have Andrew's scale_delta version >> >> arch/x86/include/arch/lib.h | 18 ++++++++++ >> build/files.mk | 1 + >> common/time.c | 81 +++++++++++++++++++++++++++++++++++++++++++++ >> include/xtf/time.h | 24 ++++++++++++++ >> 4 files changed, 124 insertions(+) >> create mode 100644 common/time.c >> create mode 100644 include/xtf/time.h >> >> diff --git a/arch/x86/include/arch/lib.h b/arch/x86/include/arch/lib.h >> index 0045902..510cdb1 100644 >> --- a/arch/x86/include/arch/lib.h >> +++ b/arch/x86/include/arch/lib.h >> @@ -6,6 +6,7 @@ >> #include <xen/arch-x86/xen.h> >> #include <arch/desc.h> >> #include <arch/msr.h> >> +#include <arch/barrier.h> > > This include list is sorted alphabetically. > >> >> static inline void cpuid(uint32_t leaf, >> uint32_t *eax, uint32_t *ebx, >> @@ -374,6 +375,23 @@ static inline void write_xcr0(uint64_t xcr0) >> xsetbv(0, xcr0); >> } >> >> +static inline uint64_t rdtsc(void) >> +{ >> + uint32_t lo, hi; >> + >> + asm volatile("rdtsc": "=a"(lo), "=d"(hi)); >> + >> + return ((uint64_t)hi << 32) | lo; >> +} >> + >> +static inline uint64_t rdtsc_ordered(void) >> +{ >> + rmb(); >> + mb(); >> + >> + return rdtsc(); >> +} > > I would likely just add a single function like: > > static inline uint64_t rdtsc_ordered(void) > { > uint32_t lo, hi; > > asm volatile("lfence; mfence; rdtsc" : "=a" (lo), "=d" (hi)); > > return ((uint64_t)hi << 32) | lo; > } > > Because there's no external caller of rdtsc, but I will leave that to > Andrew to decide. In any case this should work fine. > I think you're right for this one. What do you think about it @Andrew ? >> + >> #endif /* XTF_X86_LIB_H */ >> >> /* >> diff --git a/build/files.mk b/build/files.mk >> index 46b42d6..55ed1ca 100644 >> --- a/build/files.mk >> +++ b/build/files.mk >> @@ -16,6 +16,7 @@ obj-perarch += $(ROOT)/common/libc/vsnprintf.o >> obj-perarch += $(ROOT)/common/report.o >> obj-perarch += $(ROOT)/common/setup.o >> obj-perarch += $(ROOT)/common/xenbus.o >> +obj-perarch += $(ROOT)/common/time.o >> >> obj-perenv += $(ROOT)/arch/x86/decode.o >> obj-perenv += $(ROOT)/arch/x86/desc.o >> diff --git a/common/time.c b/common/time.c >> new file mode 100644 >> index 0000000..79abc7e >> --- /dev/null >> +++ b/common/time.c >> @@ -0,0 +1,81 @@ >> +#include <xtf/types.h> >> +#include <xtf/traps.h> >> +#include <xtf/time.h> > > Alphabetic order please. > >> +#include <arch/barrier.h> >> +#include <arch/lib.h> >> + >> +/* This function was taken from mini-os source code */ >> +/* It returns ((delta << shift) * mul_frac) >> 32 */ > > Comment has wrong style, please check CODING_STYLE. > >> +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift) >> +{ >> + uint64_t product; >> +#ifdef __i386__ >> + uint32_t tmp1, tmp2; >> +#endif >> + >> + if ( shift < 0 ) >> + delta >>= -shift; >> + else >> + delta <<= shift; >> + >> +#ifdef __i386__ >> + __asm__ ( >> + "mul %5 ; " >> + "mov %4,%%eax ; " >> + "mov %%edx,%4 ; " >> + "mul %5 ; " >> + "add %4,%%eax ; " >> + "xor %5,%5 ; " >> + "adc %5,%%edx ; " >> + : "=A" (product), "=r" (tmp1), "=r" (tmp2) >> + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) ); > > This line is too long. > >> +#else >> + __asm__ ( >> + "mul %%rdx ; shrd $32,%%rdx,%%rax" >> + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) ); > > Not sure whether you need to add a ': "d"' clobber here, since the d > register is used but it's not in the list of output operands. > >> +#endif >> + >> + return product; >> +} >> + Actually, I'm not sure that I have to make that much changes to this function, as @Andrew wanted to use another version of it as far as I recall. >> + >> +uint64_t since_boot_time(void) >> +{ >> + uint32_t ver1, ver2; >> + uint64_t tsc_timestamp, system_time, tsc; >> + uint32_t tsc_to_system_mul; >> + int8_t tsc_shift; >> + >> + do >> + { >> + ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); >> + smp_rmb(); >> + >> + system_time = shared_info.vcpu_info[0].time.system_time; >> + tsc_timestamp = shared_info.vcpu_info[0].time.tsc_timestamp; >> + tsc_to_system_mul = shared_info.vcpu_info[0].time.tsc_to_system_mul; >> + tsc_shift = shared_info.vcpu_info[0].time.tsc_shift; >> + tsc = rdtsc_ordered(); >> + smp_rmb(); > > I don't think you need the barrier here if you use rdtsc_ordered, but > I would like confirmation from someone else. > >> + >> + ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version); >> + } while ( ver2 & 1 || ver1 != ver2 ); >> + >> + >> + system_time += scale_delta(tsc - tsc_timestamp, >> + tsc_to_system_mul, >> + tsc_shift); >> + >> + return system_time; >> +} >> + >> +/* >> + * Local variables: >> + * mode: C >> + * c-file-style: "BSD" >> + * c-basic-offset: 4 >> + * tab-width: 4 >> + * indent-tabs-mode: nil >> + * End: >> + */ >> diff --git a/include/xtf/time.h b/include/xtf/time.h >> new file mode 100644 >> index 0000000..8180e07 >> --- /dev/null >> +++ b/include/xtf/time.h >> @@ -0,0 +1,24 @@ >> +/** >> + * @file include/xtf/time.h >> + * >> + * Time management >> + */ >> +#ifndef XTF_TIME_H >> +# define XTF_TIME_H >> + >> +#include <xtf/types.h> >> + >> +/* Time from boot in nanoseconds */ >> +uint64_t since_boot_time(void); > > since_boot_time looks like a weird name, I would probably name this > hypervisor_uptime or system_time, but I'm not specially thrilled > anyway. Sure. since_boot_time was really awful.. Thanks, -- Paul Semel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 1/7] introduce time managment in xtf 2018-04-16 10:48 ` Paul Semel @ 2018-04-17 7:45 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-17 7:45 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel On Mon, Apr 16, 2018 at 12:48:49PM +0200, Paul Semel wrote: > On 04/13/2018 02:05 PM, Roger Pau Monné wrote: > > > +static inline uint64_t scale_delta(uint64_t delta, uint32_t mul_frac, int shift) > > > +{ > > > + uint64_t product; > > > +#ifdef __i386__ > > > + uint32_t tmp1, tmp2; > > > +#endif > > > + > > > + if ( shift < 0 ) > > > + delta >>= -shift; > > > + else > > > + delta <<= shift; > > > + > > > +#ifdef __i386__ > > > + __asm__ ( > > > + "mul %5 ; " > > > + "mov %4,%%eax ; " > > > + "mov %%edx,%4 ; " > > > + "mul %5 ; " > > > + "add %4,%%eax ; " > > > + "xor %5,%5 ; " > > > + "adc %5,%%edx ; " > > > + : "=A" (product), "=r" (tmp1), "=r" (tmp2) > > > + : "a" ((uint32_t)delta), "1" ((uint32_t)(delta >> 32)), "2" (mul_frac) ); > > > > This line is too long. > > > > > +#else > > > + __asm__ ( > > > + "mul %%rdx ; shrd $32,%%rdx,%%rax" > > > + : "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) ); > > > > Not sure whether you need to add a ': "d"' clobber here, since the d > > register is used but it's not in the list of output operands. > > > > > +#endif > > > + > > > + return product; > > > +} > > > + > > Actually, I'm not sure that I have to make that much changes to this > function, as @Andrew wanted to use another version of it as far as I recall. IMO if there are known issues with this function they need to be sorted out before committing. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-2-semelpaul@gmail.com>]
* Re: [PATCH v4 2/7] add current_time function to time manager [not found] ` <20180410191701.17203-2-semelpaul@gmail.com> @ 2018-04-13 13:37 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:37 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:16:56PM +0200, Paul Semel wrote: > this function returns the "epoch" time > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 39 +++++++++++++++++++++++++++++++++++++++ > include/xtf/time.h | 5 +++++ > 2 files changed, 44 insertions(+) > > diff --git a/common/time.c b/common/time.c > index 79abc7e..c1b7cd1 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -4,6 +4,7 @@ > > #include <arch/barrier.h> > #include <arch/lib.h> > +#include <arch/div.h> Sorting > > /* This function was taken from mini-os source code */ > /* It returns ((delta << shift) * mul_frac) >> 32 */ > @@ -70,6 +71,44 @@ uint64_t since_boot_time(void) > return system_time; > } > > +static void get_time_info(uint64_t *boot_time, uint64_t *sec, uint32_t *nsec) > +{ > + uint32_t ver1, ver2; > + do { > + ver1 = ACCESS_ONCE(shared_info.wc_version); > + smp_rmb(); > + *boot_time = since_boot_time(); Why are you reading the uptime in the middle of the loop? You can read this out of the loop, or in a different function. > +#if defined(__i386__) > + *sec = (uint64_t)ACCESS_ONCE(shared_info.wc_sec); > +#else > + *sec = ((uint64_t)ACCESS_ONCE(shared_info.wc_sec_hi) << 32) > + | ACCESS_ONCE(shared_info.wc_sec); > +#endif I think this should be: *sec = shared_info.wc_sec; #ifdef __i386__ *sec |= shared_info.arch.wc_sec_hi << 32; #else *sec |= shared_info.wc_sec_hi << 32; #endif > + *nsec = (uint64_t)ACCESS_ONCE(shared_info.wc_nsec); > + smp_rmb(); > + ver2 = ACCESS_ONCE(shared_info.wc_version); > + smp_rmb(); This AFAICT this has the same issues as the code used in patch 1 to access the vcpu_time_info data. You only need the ACCESS_ONCE in order to read ver2, the rest can be dropped. > + } while ( (ver1 & 1) != 0 && ver1 != ver2 ); > +} > + > +/* This function return the epoch time (number of seconds elapsed > + * since Juanary 1, 1970) */ > +uint64_t current_time(void) > +{ > + uint32_t nsec; > + uint64_t boot_time, sec; > + > + get_time_info(&boot_time, &sec, &nsec); I think it would make more sense to: 1. Call since_boot_time hypervisor_uptime. 2. Call get_time_info hypervisor_wallclock. In order to get the current time here you call both functions and then add the result, like you do below. > + > +#if defined(__i386__) > + divmod64(&boot_time, SEC_TO_NSEC(1)); > +#else > + boot_time /= SEC_TO_NSEC(1); > +#endif Please use devmod64 unconditionally. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-3-semelpaul@gmail.com>]
* Re: [PATCH v4 3/7] add gettimeofday function to time managment [not found] ` <20180410191701.17203-3-semelpaul@gmail.com> @ 2018-04-13 13:39 ` Roger Pau Monné 2018-04-16 10:16 ` Paul Semel 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:39 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: > this function acts as the POSIX gettimeofday function > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 30 ++++++++++++++++++++++++++++++ > include/xtf/time.h | 8 ++++++++ > 2 files changed, 38 insertions(+) > > diff --git a/common/time.c b/common/time.c > index c1b7cd1..8489f3b 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -1,6 +1,7 @@ > #include <xtf/types.h> > #include <xtf/traps.h> > #include <xtf/time.h> > +#include <xen/errno.h> Sorting. > > #include <arch/barrier.h> > #include <arch/lib.h> > @@ -109,6 +110,35 @@ uint64_t current_time(void) > return sec + boot_time; > } > > +/* The POSIX gettimeofday syscall normally takes a second argument, which is > + * the timezone (struct timezone). However, it sould be NULL because linux > + * doesn't use it anymore. So we need for us to add it in this function > + */ > +int gettimeofday(struct timeval *tp, void *restrict tzp) > +{ > + uint64_t boot_time, sec; > + uint32_t mod, nsec; > + > + if ( tzp != NULL ) > + return -EOPNOTSUPP; > + > + if ( tp == NULL ) > + return -EINVAL; > + > + get_time_info(&boot_time, &sec, &nsec); Why are you using get_time_info here? Shouldn't you use the current_time function introduced in the previous patch? Or else I don't see the need to introduce current_time in the previous patch. > +#if defined(__i386__) > + mod = divmod64(&boot_time, SEC_TO_NSEC(1)); > +#else > + mod = boot_time % SEC_TO_NSEC(1); > + boot_time /= SEC_TO_NSEC(1); > +#endif Please use divmod64 unconditionally. > + > + tp->sec = sec + boot_time; > + tp->nsec = nsec + mod; > + return 0; > +} > + > /* > * Local variables: > * mode: C > diff --git a/include/xtf/time.h b/include/xtf/time.h > index e33dc8a..ce4d6db 100644 > --- a/include/xtf/time.h > +++ b/include/xtf/time.h > @@ -8,6 +8,12 @@ > > #include <xtf/types.h> > > +struct timeval { > + uint64_t sec; > + uint64_t nsec; > +}; > + > + Extra newline. Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] add gettimeofday function to time managment 2018-04-13 13:39 ` [PATCH v4 3/7] add gettimeofday function to time managment Roger Pau Monné @ 2018-04-16 10:16 ` Paul Semel 2018-04-17 7:41 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Paul Semel @ 2018-04-16 10:16 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel Hi ! Thanks a lot for reviewing ! On 04/13/2018 03:39 PM, Roger Pau Monné wrote: > On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: >> this function acts as the POSIX gettimeofday function >> >> Signed-off-by: Paul Semel <phentex@amazon.de> >> --- >> >> Notes: >> v4: >> - new patch version >> >> common/time.c | 30 ++++++++++++++++++++++++++++++ >> include/xtf/time.h | 8 ++++++++ >> 2 files changed, 38 insertions(+) >> >> diff --git a/common/time.c b/common/time.c >> index c1b7cd1..8489f3b 100644 >> --- a/common/time.c >> +++ b/common/time.c >> @@ -1,6 +1,7 @@ >> #include <xtf/types.h> >> #include <xtf/traps.h> >> #include <xtf/time.h> >> +#include <xen/errno.h> > > Sorting. > >> >> #include <arch/barrier.h> >> #include <arch/lib.h> >> @@ -109,6 +110,35 @@ uint64_t current_time(void) >> return sec + boot_time; >> } >> >> +/* The POSIX gettimeofday syscall normally takes a second argument, which is >> + * the timezone (struct timezone). However, it sould be NULL because linux >> + * doesn't use it anymore. So we need for us to add it in this function >> + */ >> +int gettimeofday(struct timeval *tp, void *restrict tzp) >> +{ >> + uint64_t boot_time, sec; >> + uint32_t mod, nsec; >> + >> + if ( tzp != NULL ) >> + return -EOPNOTSUPP; >> + >> + if ( tp == NULL ) >> + return -EINVAL; >> + >> + get_time_info(&boot_time, &sec, &nsec); > > Why are you using get_time_info here? Shouldn't you use the > current_time function introduced in the previous patch? > > Or else I don't see the need to introduce current_time in the previous > patch. > Actually, I can't use *only* the current_time function here, because I won't be able to get the nanoseconds if so. Anyway, in the last patch, I am using current_time function for the NOW() macro, which I think is really helpful. Do you think I should drop all of those ? >> +#if defined(__i386__) >> + mod = divmod64(&boot_time, SEC_TO_NSEC(1)); >> +#else >> + mod = boot_time % SEC_TO_NSEC(1); >> + boot_time /= SEC_TO_NSEC(1); >> +#endif > > Please use divmod64 unconditionally. > >> + >> + tp->sec = sec + boot_time; >> + tp->nsec = nsec + mod; >> + return 0; >> +} >> + >> /* >> * Local variables: >> * mode: C >> diff --git a/include/xtf/time.h b/include/xtf/time.h >> index e33dc8a..ce4d6db 100644 >> --- a/include/xtf/time.h >> +++ b/include/xtf/time.h >> @@ -8,6 +8,12 @@ >> >> #include <xtf/types.h> >> >> +struct timeval { >> + uint64_t sec; >> + uint64_t nsec; >> +}; >> + >> + > > Extra newline. Thanks, -- Paul _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 3/7] add gettimeofday function to time managment 2018-04-16 10:16 ` Paul Semel @ 2018-04-17 7:41 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-17 7:41 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel On Mon, Apr 16, 2018 at 12:16:18PM +0200, Paul Semel wrote: > Hi ! > > Thanks a lot for reviewing ! > > On 04/13/2018 03:39 PM, Roger Pau Monné wrote: > > On Tue, Apr 10, 2018 at 09:16:57PM +0200, Paul Semel wrote: > > > this function acts as the POSIX gettimeofday function > > > > > > Signed-off-by: Paul Semel <phentex@amazon.de> > > > --- > > > > > > Notes: > > > v4: > > > - new patch version > > > > > > common/time.c | 30 ++++++++++++++++++++++++++++++ > > > include/xtf/time.h | 8 ++++++++ > > > 2 files changed, 38 insertions(+) > > > > > > diff --git a/common/time.c b/common/time.c > > > index c1b7cd1..8489f3b 100644 > > > --- a/common/time.c > > > +++ b/common/time.c > > > @@ -1,6 +1,7 @@ > > > #include <xtf/types.h> > > > #include <xtf/traps.h> > > > #include <xtf/time.h> > > > +#include <xen/errno.h> > > > > Sorting. > > > > > #include <arch/barrier.h> > > > #include <arch/lib.h> > > > @@ -109,6 +110,35 @@ uint64_t current_time(void) > > > return sec + boot_time; > > > } > > > +/* The POSIX gettimeofday syscall normally takes a second argument, which is > > > + * the timezone (struct timezone). However, it sould be NULL because linux > > > + * doesn't use it anymore. So we need for us to add it in this function > > > + */ > > > +int gettimeofday(struct timeval *tp, void *restrict tzp) > > > +{ > > > + uint64_t boot_time, sec; > > > + uint32_t mod, nsec; > > > + > > > + if ( tzp != NULL ) > > > + return -EOPNOTSUPP; > > > + > > > + if ( tp == NULL ) > > > + return -EINVAL; > > > + > > > + get_time_info(&boot_time, &sec, &nsec); > > > > Why are you using get_time_info here? Shouldn't you use the > > current_time function introduced in the previous patch? > > > > Or else I don't see the need to introduce current_time in the previous > > patch. > > > > Actually, I can't use *only* the current_time function here, because I won't > be able to get the nanoseconds if so. > > Anyway, in the last patch, I am using current_time function for the NOW() > macro, which I think is really helpful. > > Do you think I should drop all of those ? Without having any users of those functions it's hard to tell which ones we should keep and which ones should be removed. IMO, I would keep gettimeofday in order to get the current time. Then I would also keep the m/u/sleep functions and add a small selftest in test/selftest/main.c that makes use of those functions, for example something as simple as: s = gettimeofday msleep(1) if ( s + 1ms < gettimeofday ) xtf_failure("Fail: error in time keeping functions\n") Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-4-semelpaul@gmail.com>]
* Re: [PATCH v4 4/7] add nspin_sleep function to time manager [not found] ` <20180410191701.17203-4-semelpaul@gmail.com> @ 2018-04-13 13:51 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:51 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:16:58PM +0200, Paul Semel wrote: > this function spin sleeps for t nanoseconds > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/common/time.c b/common/time.c > index 8489f3b..232e134 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -139,6 +139,18 @@ int gettimeofday(struct timeval *tp, void *restrict tzp) > return 0; > } > > +static inline void nspin_sleep(uint64_t t) > +{ > + uint64_t curr = since_boot_time(); > + uint64_t end = curr + t; > + > + if ( end < curr ) > + panic("end value overflows counter\n"); > + > + while ( since_boot_time() < end ) > + asm volatile ("pause"); You likely want to add a pause helper to arch/x86/include/arch/lib.h in order to avoid open-coding the asm instruction in common code. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-5-semelpaul@gmail.com>]
* Re: [PATCH v4 5/7] add spin_sleep function to time manager [not found] ` <20180410191701.17203-5-semelpaul@gmail.com> @ 2018-04-13 13:52 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:52 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:16:59PM +0200, Paul Semel wrote: > this function uses nspin_sleep to spin sleep for t seconds IMO you can squash this into the previous patch. > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/common/time.c b/common/time.c > index 232e134..87db124 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -151,6 +151,12 @@ static inline void nspin_sleep(uint64_t t) > asm volatile ("pause"); > } > > +static inline void spin_sleep(uint64_t t) > +{ > + uint64_t nsec = SEC_TO_NSEC(t); Newline. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-6-semelpaul@gmail.com>]
* Re: [PATCH v4 6/7] add mspin_sleep function to time manager [not found] ` <20180410191701.17203-6-semelpaul@gmail.com> @ 2018-04-13 13:53 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:53 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:17:00PM +0200, Paul Semel wrote: > this function uses mspin_sleep to spin sleep for t milliseconds > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 6 ++++++ > include/xtf/time.h | 1 + > 2 files changed, 7 insertions(+) > > diff --git a/common/time.c b/common/time.c > index 87db124..7515eb0 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -157,6 +157,12 @@ static inline void spin_sleep(uint64_t t) > nspin_sleep(nsec); > } > > +static inline void mspin_sleep(uint64_t t) > +{ > + uint64_t nsec = MSEC_TO_NSEC(t); Newline. And IMO can be squashed into the previous patch. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
[parent not found: <20180410191701.17203-7-semelpaul@gmail.com>]
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager [not found] ` <20180410191701.17203-7-semelpaul@gmail.com> @ 2018-04-13 13:55 ` Roger Pau Monné 2018-04-16 10:45 ` Paul Semel 0 siblings, 1 reply; 13+ messages in thread From: Roger Pau Monné @ 2018-04-13 13:55 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, Paul Semel, andrew.cooper3, wipawel On Tue, Apr 10, 2018 at 09:17:01PM +0200, Paul Semel wrote: > those are helpful macro to use the time manager correctly > > Signed-off-by: Paul Semel <phentex@amazon.de> > --- > > Notes: > v4: > - new patch version > > common/time.c | 10 ++++++++++ > include/xtf/time.h | 12 ++++++++++++ > 2 files changed, 22 insertions(+) > > diff --git a/common/time.c b/common/time.c > index 7515eb0..e2779b9 100644 > --- a/common/time.c > +++ b/common/time.c > @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t) > nspin_sleep(nsec); > } > > +void sleep(uint64_t t) > +{ > + spin_sleep(t); > +} > + > +void msleep(uint64_t t) > +{ > + mspin_sleep(t); Why can you just call mspin_sleep msleep directly? The same applies to spin_sleep. Also I was expecting to see some kind of test appear at the end of the series. You are basically adding a bunch of dead code, since there's no user of any of the newly introduced functions. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager 2018-04-13 13:55 ` [PATCH v4 7/7] add sleep, msleep and NOW() macros " Roger Pau Monné @ 2018-04-16 10:45 ` Paul Semel 2018-04-17 7:42 ` Roger Pau Monné 0 siblings, 1 reply; 13+ messages in thread From: Paul Semel @ 2018-04-16 10:45 UTC (permalink / raw) To: Roger Pau Monné; +Cc: xen-devel, andrew.cooper3, wipawel On 04/13/2018 03:55 PM, Roger Pau Monné wrote: >> those are helpful macro to use the time manager correctly >> >> Signed-off-by: Paul Semel <phentex@amazon.de> >> --- >> >> Notes: >> v4: >> - new patch version >> >> common/time.c | 10 ++++++++++ >> include/xtf/time.h | 12 ++++++++++++ >> 2 files changed, 22 insertions(+) >> >> diff --git a/common/time.c b/common/time.c >> index 7515eb0..e2779b9 100644 >> --- a/common/time.c >> +++ b/common/time.c >> @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t) >> nspin_sleep(nsec); >> } >> >> +void sleep(uint64_t t) >> +{ >> + spin_sleep(t); >> +} >> + >> +void msleep(uint64_t t) >> +{ >> + mspin_sleep(t); > > Why can you just call mspin_sleep msleep directly? > > The same applies to spin_sleep. > > Also I was expecting to see some kind of test appear at the end of the > series. You are basically adding a bunch of dead code, since there's > no user of any of the newly introduced functions. Actually, I won't be able to add a real XSA test using this feature. Anyway, I do think that having the sleep functions will be really useful in the future. Anyway, I was thinking about adding a test that is calling the gettimeofday function or something similar. What do you think about it ? -- Paul Semel _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v4 7/7] add sleep, msleep and NOW() macros to time manager 2018-04-16 10:45 ` Paul Semel @ 2018-04-17 7:42 ` Roger Pau Monné 0 siblings, 0 replies; 13+ messages in thread From: Roger Pau Monné @ 2018-04-17 7:42 UTC (permalink / raw) To: Paul Semel; +Cc: xen-devel, andrew.cooper3, wipawel On Mon, Apr 16, 2018 at 12:45:17PM +0200, Paul Semel wrote: > On 04/13/2018 03:55 PM, Roger Pau Monné wrote: > > > those are helpful macro to use the time manager correctly > > > > > > Signed-off-by: Paul Semel <phentex@amazon.de> > > > --- > > > > > > Notes: > > > v4: > > > - new patch version > > > > > > common/time.c | 10 ++++++++++ > > > include/xtf/time.h | 12 ++++++++++++ > > > 2 files changed, 22 insertions(+) > > > > > > diff --git a/common/time.c b/common/time.c > > > index 7515eb0..e2779b9 100644 > > > --- a/common/time.c > > > +++ b/common/time.c > > > @@ -163,6 +163,16 @@ static inline void mspin_sleep(uint64_t t) > > > nspin_sleep(nsec); > > > } > > > +void sleep(uint64_t t) > > > +{ > > > + spin_sleep(t); > > > +} > > > + > > > +void msleep(uint64_t t) > > > +{ > > > + mspin_sleep(t); > > > > Why can you just call mspin_sleep msleep directly? > > > > The same applies to spin_sleep. > > > > Also I was expecting to see some kind of test appear at the end of the > > series. You are basically adding a bunch of dead code, since there's > > no user of any of the newly introduced functions. > > Actually, I won't be able to add a real XSA test using this feature. Anyway, > I do think that having the sleep functions will be really useful in the > future. > Anyway, I was thinking about adding a test that is calling the gettimeofday > function or something similar. > > What do you think about it ? Yes, see my last reply to 3/7. Thanks, Roger. _______________________________________________ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2018-04-17 7:45 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20180410191701.17203-1-semelpaul@gmail.com> 2018-04-13 12:05 ` [PATCH v4 1/7] introduce time managment in xtf Roger Pau Monné 2018-04-16 10:48 ` Paul Semel 2018-04-17 7:45 ` Roger Pau Monné [not found] ` <20180410191701.17203-2-semelpaul@gmail.com> 2018-04-13 13:37 ` [PATCH v4 2/7] add current_time function to time manager Roger Pau Monné [not found] ` <20180410191701.17203-3-semelpaul@gmail.com> 2018-04-13 13:39 ` [PATCH v4 3/7] add gettimeofday function to time managment Roger Pau Monné 2018-04-16 10:16 ` Paul Semel 2018-04-17 7:41 ` Roger Pau Monné [not found] ` <20180410191701.17203-4-semelpaul@gmail.com> 2018-04-13 13:51 ` [PATCH v4 4/7] add nspin_sleep function to time manager Roger Pau Monné [not found] ` <20180410191701.17203-5-semelpaul@gmail.com> 2018-04-13 13:52 ` [PATCH v4 5/7] add spin_sleep " Roger Pau Monné [not found] ` <20180410191701.17203-6-semelpaul@gmail.com> 2018-04-13 13:53 ` [PATCH v4 6/7] add mspin_sleep " Roger Pau Monné [not found] ` <20180410191701.17203-7-semelpaul@gmail.com> 2018-04-13 13:55 ` [PATCH v4 7/7] add sleep, msleep and NOW() macros " Roger Pau Monné 2018-04-16 10:45 ` Paul Semel 2018-04-17 7:42 ` Roger Pau Monné
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.