Re: [Xen-devel] [PATCH v4 1/7] introduce time managment in xtf

2018-04-17 Thread Roger Pau Monné 
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

Re: [Xen-devel] [PATCH v4 1/7] introduce time managment in xtf

2018-04-16 Thread Paul Semel 

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 
---

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 
  #include 
  #include 
+#include 


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 000..79abc7e
--- /dev/null
+++ b/common/time.c
@@ -0,0 +1,81 @@
+#include 
+#include 
+#include 


Alphabetic order please.


+#include 
+#include 
+
+/* 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,
+

Re: [Xen-devel] [PATCH v4 1/7] introduce time managment in xtf

2018-04-13 Thread Roger Pau Monné 
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 
> ---
> 
> 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 
>  #include 
>  #include 
> +#include 

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 000..79abc7e
> --- /dev/null
> +++ b/common/time.c
> @@ -0,0 +1,81 @@
> +#include 
> +#include 
> +#include 

Alphabetic order please.

> +#include 
> +#include 
> +
> +/* 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 -

[Xen-devel] [PATCH v4 1/7] introduce time managment in xtf

2018-04-10 Thread Paul Semel 
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 
---

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 
 #include 
 #include 
+#include 
 
 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();
+}
+
 #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 000..79abc7e
--- /dev/null
+++ b/common/time.c
@@ -0,0 +1,81 @@
+#include 
+#include 
+#include 
+
+#include 
+#include 
+
+/* This function was taken from mini-os source code */
+/* It returns ((delta << shift) * mul_frac) >> 32 */
+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) );
+#else
+__asm__ (
+"mul %%rdx ; shrd $32,%%rdx,%%rax"
+: "=a" (product) : "0" (delta), "d" ((uint64_t)mul_frac) );
+#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();
+
+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 000..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 
+
+/* Time from boot in nanoseconds */
+uint64_t since_boot_time(void);
+
+#endif /* XTF_TIME_H */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
-- 
2.16.1


___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel