[PATCH v3 1/7] introduce time managment in xtf

[PATCH v3 1/7] introduce time managment in xtf

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

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>
---
 build/files.mk     |  1 +
 common/time.c      | 81 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 include/xtf/time.h | 31 +++++++++++++++++++++
 3 files changed, 113 insertions(+)
 create mode 100644 common/time.c
 create mode 100644 include/xtf/time.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..7859c21
--- /dev/null
+++ b/common/time.c
@@ -0,0 +1,81 @@
+#include <xtf/types.h>
+#include <xtf/traps.h>
+#include <xtf/time.h>
+
+#include <arch/barrier.h>
+
+/* 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)
+{
+    uint64_t tsc;
+    uint32_t ver1, ver2;
+    uint64_t system_time;
+    uint64_t old_tsc;
+
+    do
+    {
+        do
+        {
+            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
+            smp_rmb();
+        } while ( (ver1 & 1) == 1 );
+
+        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
+        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
+        smp_rmb();
+        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
+        smp_rmb();
+    } while ( ver1 != ver2 );
+
+    rdtsc(tsc);
+
+    system_time += scale_delta(tsc - old_tsc,
+                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
+                   ACCESS_ONCE(shared_info.vcpu_info[0].time.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..b88da63
--- /dev/null
+++ b/include/xtf/time.h
@@ -0,0 +1,31 @@
+/**
+ * @file include/xtf/time.h
+ *
+ * Time management
+ */
+#ifndef XTF_TIME_H
+# define XTF_TIME_H
+
+#include <xtf/types.h>
+
+#define rdtsc(tsc) {\
+    uint32_t lo, hi;\
+    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
+    tsc = ((uint64_t)hi << 32) | lo;\
+}
+
+
+/* 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

[PATCH v3 2/7] add current_time function to time manager

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

this function returns the "epoch" time

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 common/time.c      | 39 +++++++++++++++++++++++++++++++++++++++
 include/xtf/time.h |  4 ++++
 2 files changed, 43 insertions(+)

diff --git a/common/time.c b/common/time.c
index 7859c21..29b38ca 100644
--- a/common/time.c
+++ b/common/time.c
@@ -3,6 +3,7 @@
 #include <xtf/time.h>
 
 #include <arch/barrier.h>
+#include <arch/div.h>
 
 /* 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();
+#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
+        *nsec = (uint64_t)ACCESS_ONCE(shared_info.wc_nsec);
+        smp_rmb();
+        ver2 = ACCESS_ONCE(shared_info.wc_version);
+        smp_rmb();
+    } 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);
+
+#if defined(__i386__)
+    divmod64(&boot_time, SEC_TO_NSEC(1));
+#else
+    boot_time /= SEC_TO_NSEC(1);
+#endif
+
+    return sec + boot_time;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/include/xtf/time.h b/include/xtf/time.h
index b88da63..4d94958 100644
--- a/include/xtf/time.h
+++ b/include/xtf/time.h
@@ -14,10 +14,14 @@
     tsc = ((uint64_t)hi << 32) | lo;\
 }
 
+#define SEC_TO_NSEC(x) ((x) * 1000000000ul)
+
 
 /* Time from boot in nanoseconds */
 uint64_t since_boot_time(void);
 
+uint64_t current_time(void);
+
 #endif /* XTF_TIME_H */
 
 /*
-- 
2.16.1


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

[PATCH v3 3/7] add gettimeofday function to time managment

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

this function acts as the POSIX gettimeofday function

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 common/time.c      | 30 ++++++++++++++++++++++++++++++
 include/xtf/time.h |  7 +++++++
 2 files changed, 37 insertions(+)

diff --git a/common/time.c b/common/time.c
index 29b38ca..d0c9ed2 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>
 
 #include <arch/barrier.h>
 #include <arch/div.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);
+
+#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
+
+    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 4d94958..5ed88ad 100644
--- a/include/xtf/time.h
+++ b/include/xtf/time.h
@@ -8,6 +8,11 @@
 
 #include <xtf/types.h>
 
+struct timeval {
+    uint64_t sec;
+    uint64_t nsec;
+};
+
 #define rdtsc(tsc) {\
     uint32_t lo, hi;\
     __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
@@ -22,6 +27,8 @@ uint64_t since_boot_time(void);
 
 uint64_t current_time(void);
 
+int gettimeofday(struct timeval *tp, void *restrict tzp);
+
 #endif /* XTF_TIME_H */
 
 /*
-- 
2.16.1


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

[PATCH v3 4/7] add nspin_sleep function to time manager

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

this function spin sleeps for t nanoseconds

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 common/time.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/common/time.c b/common/time.c
index d0c9ed2..4770b9a 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");
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.16.1


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

[PATCH v3 5/7] add spin_sleep function to time manager

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

this function uses nspin_sleep to spin sleep for t seconds

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 common/time.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/common/time.c b/common/time.c
index 4770b9a..e744ab1 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);
+    nspin_sleep(nsec);
+}
+
 /*
  * Local variables:
  * mode: C
-- 
2.16.1


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

[PATCH v3 6/7] add mspin_sleep function to time manager

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

this function uses mspin_sleep to spin sleep for t milliseconds

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 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 e744ab1..9685591 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);
+    nspin_sleep(nsec);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/include/xtf/time.h b/include/xtf/time.h
index 5ed88ad..17fb561 100644
--- a/include/xtf/time.h
+++ b/include/xtf/time.h
@@ -20,6 +20,7 @@ struct timeval {
 }
 
 #define SEC_TO_NSEC(x) ((x) * 1000000000ul)
+#define MSEC_TO_NSEC(x) ((x) * 1000000ul)
 
 
 /* Time from boot in nanoseconds */
-- 
2.16.1


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

[PATCH v3 7/7] add sleep, msleep and NOW() macros to time manager

[Paul Semel]

From: Paul Semel <phentex@amazon.de>

those are helpful macro to use the time manager correctly

Signed-off-by: Paul Semel <phentex@amazon.de>
---
 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 9685591..714afb8 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);
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/include/xtf/time.h b/include/xtf/time.h
index 17fb561..ff431a2 100644
--- a/include/xtf/time.h
+++ b/include/xtf/time.h
@@ -28,8 +28,20 @@ uint64_t since_boot_time(void);
 
 uint64_t current_time(void);
 
+/* This function takes seconds in parameter */
+void sleep(uint64_t f);
+
+/* Be careful, this function takes milliseconds in parameter,
+ * not microseconds !
+ */
+void msleep(uint64_t f);
+
 int gettimeofday(struct timeval *tp, void *restrict tzp);
 
+
+/* This returns the current epoch time */
+#define NOW() current_time()
+
 #endif /* XTF_TIME_H */
 
 /*
-- 
2.16.1


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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Roger Pau Monné]

On Mon, Apr 09, 2018 at 04:35:37PM +0200, Paul Semel wrote:
> From: Paul Semel <phentex@amazon.de>
> 
> 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>
> ---

This seems to be missing a list of changes between v2 and v3. Please
add such a list when posting new versions.

> +uint64_t since_boot_time(void)
> +{
> +    uint64_t tsc;
> +    uint32_t ver1, ver2;
> +    uint64_t system_time;
> +    uint64_t old_tsc;
> +
> +    do
> +    {
> +        do
> +        {
> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> +            smp_rmb();
> +        } while ( (ver1 & 1) == 1 );
> +
> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
> +        smp_rmb();
> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> +        smp_rmb();
> +    } while ( ver1 != ver2 );

This is still overly complicated IMO, and you have not replied to my
question of whether doing the scale_delta below is OK.

AFAICT uou _cannot_ access any of the vcpu_time_info fields without
checking for the version (in order to avoid reading inconsistent data
during an update), yet below you read tsc_to_system_mul and
tsc_shift.

I've already pointed out the code at:

https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141

As a simpler reference implementation.

> +
> +    rdtsc(tsc);
> +
> +    system_time += scale_delta(tsc - old_tsc,
> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.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..b88da63
> --- /dev/null
> +++ b/include/xtf/time.h
> @@ -0,0 +1,31 @@
> +/**
> + * @file include/xtf/time.h
> + *
> + * Time management
> + */
> +#ifndef XTF_TIME_H
> +# define XTF_TIME_H
> +
> +#include <xtf/types.h>
> +
> +#define rdtsc(tsc) {\
> +    uint32_t lo, hi;\
> +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\

Please make sure you only send a new version after having fixed all
the comments, this is still missing the serialization requirements
mentioned in the review, and it's also the wrong file to place this
helper:

https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00546.html

Roger.

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Andrew Cooper]

On 09/04/18 15:35, Roger Pau Monné wrote:
> On Mon, Apr 09, 2018 at 04:35:37PM +0200, Paul Semel wrote:
>> From: Paul Semel <phentex@amazon.de>
>>
>> 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>
>> ---
> This seems to be missing a list of changes between v2 and v3. Please
> add such a list when posting new versions.
>
>> +uint64_t since_boot_time(void)
>> +{
>> +    uint64_t tsc;
>> +    uint32_t ver1, ver2;
>> +    uint64_t system_time;
>> +    uint64_t old_tsc;
>> +
>> +    do
>> +    {
>> +        do
>> +        {
>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +            smp_rmb();
>> +        } while ( (ver1 & 1) == 1 );
>> +
>> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>> +        smp_rmb();
>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +        smp_rmb();
>> +    } while ( ver1 != ver2 );
> This is still overly complicated IMO, and you have not replied to my
> question of whether doing the scale_delta below is OK.
>
> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> checking for the version (in order to avoid reading inconsistent data
> during an update), yet below you read tsc_to_system_mul and
> tsc_shift.
>
> I've already pointed out the code at:
>
> https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141
>
> As a simpler reference implementation.

There are real bugs in that implementation (wrong barriers, although
they are overly strong rather than overly week), and an inefficiency, in
that you don't want any extraneous calculation in between the two reads
of version.  The call to pvclock_get_nsec_offset(ti) wants to be outside
of the critical region, to reduce the chance of intersecting an update
and having repeat the loop again.

~Andrew

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Paul Semel]

On 04/09/2018 04:35 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>
>> ---
> 
> This seems to be missing a list of changes between v2 and v3. Please
> add such a list when posting new versions.
> 
>> +uint64_t since_boot_time(void)
>> +{
>> +    uint64_t tsc;
>> +    uint32_t ver1, ver2;
>> +    uint64_t system_time;
>> +    uint64_t old_tsc;
>> +
>> +    do
>> +    {
>> +        do
>> +        {
>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +            smp_rmb();
>> +        } while ( (ver1 & 1) == 1 );
>> +
>> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>> +        smp_rmb();
>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>> +        smp_rmb();
>> +    } while ( ver1 != ver2 );
> 
> This is still overly complicated IMO, and you have not replied to my
> question of whether doing the scale_delta below is OK.

About this scale_delta, we discussed with Andrew, and we are going to use 
another version of the function as far as I remember. That's why I am not taking 
care of it for the moment.
> 
> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> checking for the version (in order to avoid reading inconsistent data
> during an update), yet below you read tsc_to_system_mul and
> tsc_shift.
> 

I'm sorry, I am probably not getting your point here, because I am already 
checking for the version. I was actually checking for the wc_version too in the 
first version of those patches, but after chatting with Andrew, It appeared that 
it was not necessary..

> I've already pointed out the code at:
> 
> https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141
> 
> As a simpler reference implementation.
> 
>> +
>> +    rdtsc(tsc);
>> +
>> +    system_time += scale_delta(tsc - old_tsc,
>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.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..b88da63
>> --- /dev/null
>> +++ b/include/xtf/time.h
>> @@ -0,0 +1,31 @@
>> +/**
>> + * @file include/xtf/time.h
>> + *
>> + * Time management
>> + */
>> +#ifndef XTF_TIME_H
>> +# define XTF_TIME_H
>> +
>> +#include <xtf/types.h>
>> +
>> +#define rdtsc(tsc) {\
>> +    uint32_t lo, hi;\
>> +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
> 
> Please make sure you only send a new version after having fixed all
> the comments, this is still missing the serialization requirements
> mentioned in the review, and it's also the wrong file to place this
> helper:
> 

I am sorry, I was really convinced that this version didn't need revision 
anymore (and I still don't see what I should change).

> https://lists.xenproject.org/archives/html/xen-devel/2018-04/msg00546.html
> 
> Roger.
> 

Thanks,

-- 
Paul

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Roger Pau Monné]

On Mon, Apr 09, 2018 at 05:12:46PM +0200, Paul Semel wrote:
> On 04/09/2018 04:35 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>
> > > ---
> > 
> > This seems to be missing a list of changes between v2 and v3. Please
> > add such a list when posting new versions.
> > 
> > > +uint64_t since_boot_time(void)
> > > +{
> > > +    uint64_t tsc;
> > > +    uint32_t ver1, ver2;
> > > +    uint64_t system_time;
> > > +    uint64_t old_tsc;
> > > +
> > > +    do
> > > +    {
> > > +        do
> > > +        {
> > > +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > +            smp_rmb();
> > > +        } while ( (ver1 & 1) == 1 );
> > > +
> > > +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
> > > +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
> > > +        smp_rmb();
> > > +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > +        smp_rmb();
> > > +    } while ( ver1 != ver2 );
> > 
> > This is still overly complicated IMO, and you have not replied to my
> > question of whether doing the scale_delta below is OK.
> 
> About this scale_delta, we discussed with Andrew, and we are going to use
> another version of the function as far as I remember. That's why I am not
> taking care of it for the moment.

You should send that version then :).

> > 
> > AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> > checking for the version (in order to avoid reading inconsistent data
> > during an update), yet below you read tsc_to_system_mul and
> > tsc_shift.
> > 
> 
> I'm sorry, I am probably not getting your point here, because I am already
> checking for the version. I was actually checking for the wc_version too in
> the first version of those patches, but after chatting with Andrew, It
> appeared that it was not necessary..

AFAICT the following should work:

do
{
    ver1 = 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();
    /* NB: this barrier is probably not needed if rdtsc is serializing. */
    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,
                           time.tsc_shift);

I'm not sure the second barrier is actually needed, since
rdtsc_ordered should be serializing.

> > I've already pointed out the code at:
> > 
> > https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141
> > 
> > As a simpler reference implementation.
> > 
> > > +
> > > +    rdtsc(tsc);
> > > +
> > > +    system_time += scale_delta(tsc - old_tsc,
> > > +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
> > > +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.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..b88da63
> > > --- /dev/null
> > > +++ b/include/xtf/time.h
> > > @@ -0,0 +1,31 @@
> > > +/**
> > > + * @file include/xtf/time.h
> > > + *
> > > + * Time management
> > > + */
> > > +#ifndef XTF_TIME_H
> > > +# define XTF_TIME_H
> > > +
> > > +#include <xtf/types.h>
> > > +
> > > +#define rdtsc(tsc) {\
> > > +    uint32_t lo, hi;\
> > > +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
> > 
> > Please make sure you only send a new version after having fixed all
> > the comments, this is still missing the serialization requirements
> > mentioned in the review, and it's also the wrong file to place this
> > helper:
> > 
> 
> I am sorry, I was really convinced that this version didn't need revision
> anymore (and I still don't see what I should change).

rdtsc is not a serializing instruction, and as such there's no
guarantee it's not executed before or after any of it's preceding or
following instructions. In order to make it serializing you need to
add a lfence or mfence, or if you are not sure about the architecture
you likely need to add both, see:

https://marc.info/?l=xen-devel&m=151983511212795&w=2

Also, as said in the previous review, the rdtsc helper should be
placed in a x86 specific file because it's a x86 specific instruction.
IMO it should be placed in arch/x86/include/arch/lib.h with the rest
of the x86 specific instructions.

Hope this clarifies the changes.

Roger.

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Paul Semel]

On 04/10/2018 10:08 AM, 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>
>>>> ---
>>>
>>> This seems to be missing a list of changes between v2 and v3. Please
>>> add such a list when posting new versions.
>>>
>>>> +uint64_t since_boot_time(void)
>>>> +{
>>>> +    uint64_t tsc;
>>>> +    uint32_t ver1, ver2;
>>>> +    uint64_t system_time;
>>>> +    uint64_t old_tsc;
>>>> +
>>>> +    do
>>>> +    {
>>>> +        do
>>>> +        {
>>>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>> +            smp_rmb();
>>>> +        } while ( (ver1 & 1) == 1 );
>>>> +
>>>> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>>>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>>>> +        smp_rmb();
>>>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>> +        smp_rmb();
>>>> +    } while ( ver1 != ver2 );
>>>
>>> This is still overly complicated IMO, and you have not replied to my
>>> question of whether doing the scale_delta below is OK.
>>
>> About this scale_delta, we discussed with Andrew, and we are going to use
>> another version of the function as far as I remember. That's why I am not
>> taking care of it for the moment.
> 
> You should send that version then :).
> 
>>>
>>> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
>>> checking for the version (in order to avoid reading inconsistent data
>>> during an update), yet below you read tsc_to_system_mul and
>>> tsc_shift.
>>>
>>
>> I'm sorry, I am probably not getting your point here, because I am already
>> checking for the version. I was actually checking for the wc_version too in
>> the first version of those patches, but after chatting with Andrew, It
>> appeared that it was not necessary..
> 
> AFAICT the following should work:
> 
> do
> {
>      ver1 = 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();
>      /* NB: this barrier is probably not needed if rdtsc is serializing. */
>      smp_rmb();
> 
>      ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> } while ( ver2 & 1 || ver1 != ver2 );
> 

Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on 
every shared_info field accesses ?
As far as I understand, we need to do this as most as we can to avoid 
having completely broken data (or security issues). Am I missing something ?

> system_time += scale_delta(tsc - tsc_timestamp, tsc_to_system_mul,
>                             time.tsc_shift);
> 
> I'm not sure the second barrier is actually needed, since
> rdtsc_ordered should be serializing.
> 
>>> I've already pointed out the code at:
>>>
>>> https://github.com/freebsd/freebsd/blob/master/sys/x86/x86/pvclock.c#L141
>>>
>>> As a simpler reference implementation.
>>>
>>>> +
>>>> +    rdtsc(tsc);
>>>> +
>>>> +    system_time += scale_delta(tsc - old_tsc,
>>>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_to_system_mul),
>>>> +                   ACCESS_ONCE(shared_info.vcpu_info[0].time.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..b88da63
>>>> --- /dev/null
>>>> +++ b/include/xtf/time.h
>>>> @@ -0,0 +1,31 @@
>>>> +/**
>>>> + * @file include/xtf/time.h
>>>> + *
>>>> + * Time management
>>>> + */
>>>> +#ifndef XTF_TIME_H
>>>> +# define XTF_TIME_H
>>>> +
>>>> +#include <xtf/types.h>
>>>> +
>>>> +#define rdtsc(tsc) {\
>>>> +    uint32_t lo, hi;\
>>>> +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
>>>
>>> Please make sure you only send a new version after having fixed all
>>> the comments, this is still missing the serialization requirements
>>> mentioned in the review, and it's also the wrong file to place this
>>> helper:
>>>
>>
>> I am sorry, I was really convinced that this version didn't need revision
>> anymore (and I still don't see what I should change).
> 
> rdtsc is not a serializing instruction, and as such there's no
> guarantee it's not executed before or after any of it's preceding or
> following instructions. In order to make it serializing you need to
> add a lfence or mfence, or if you are not sure about the architecture
> you likely need to add both, see:
> 
> https://marc.info/?l=xen-devel&m=151983511212795&w=2
> 
> Also, as said in the previous review, the rdtsc helper should be
> placed in a x86 specific file because it's a x86 specific instruction.
> IMO it should be placed in arch/x86/include/arch/lib.h with the rest
> of the x86 specific instructions.
> 

Thanks for clarifying this point ! I will put the function in the 
correct place, and add an implementation of rdtsc_ordered with 
serialization 🙂

> Hope this clarifies the changes.

This definitely does, thanks !

-- 
Paul

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Roger Pau Monné]

On Tue, Apr 10, 2018 at 11:47:11AM +0200, Paul Semel wrote:
> On 04/10/2018 10:08 AM, 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>
> > > > > ---
> > > > 
> > > > This seems to be missing a list of changes between v2 and v3. Please
> > > > add such a list when posting new versions.
> > > > 
> > > > > +uint64_t since_boot_time(void)
> > > > > +{
> > > > > +    uint64_t tsc;
> > > > > +    uint32_t ver1, ver2;
> > > > > +    uint64_t system_time;
> > > > > +    uint64_t old_tsc;
> > > > > +
> > > > > +    do
> > > > > +    {
> > > > > +        do
> > > > > +        {
> > > > > +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > > > +            smp_rmb();
> > > > > +        } while ( (ver1 & 1) == 1 );
> > > > > +
> > > > > +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
> > > > > +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
> > > > > +        smp_rmb();
> > > > > +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > > > +        smp_rmb();
> > > > > +    } while ( ver1 != ver2 );
> > > > 
> > > > This is still overly complicated IMO, and you have not replied to my
> > > > question of whether doing the scale_delta below is OK.
> > > 
> > > About this scale_delta, we discussed with Andrew, and we are going to use
> > > another version of the function as far as I remember. That's why I am not
> > > taking care of it for the moment.
> > 
> > You should send that version then :).
> > 
> > > > 
> > > > AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> > > > checking for the version (in order to avoid reading inconsistent data
> > > > during an update), yet below you read tsc_to_system_mul and
> > > > tsc_shift.
> > > > 
> > > 
> > > I'm sorry, I am probably not getting your point here, because I am already
> > > checking for the version. I was actually checking for the wc_version too in
> > > the first version of those patches, but after chatting with Andrew, It
> > > appeared that it was not necessary..
> > 
> > AFAICT the following should work:
> > 
> > do
> > {
> >      ver1 = 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();
> >      /* NB: this barrier is probably not needed if rdtsc is serializing. */
> >      smp_rmb();
> > 
> >      ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > } while ( ver2 & 1 || ver1 != ver2 );
> > 
> 
> Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on every
> shared_info field accesses ?
> As far as I understand, we need to do this as most as we can to avoid having
> completely broken data (or security issues). Am I missing something ?

ACCESS_ONCE prevents the reordering of the reads, but here AFAICT we
don't really care about the order in which way they are performed as
long as they are all done before the read barrier (smp_rmb).

Note that I used ACCESS_ONCE for the last access to the version field.
I've done that to prevent the compiler from optimizing the code as:

} while ( shared_info.vcpu_info[0].time.version & 1 ||
          ver1 != shared_info.vcpu_info[0].time.version );

Which would be incorrect, since we want to use the same version data
for both checks in the while loop condition.

Roger.

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Paul Semel]

On 04/10/2018 12: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>
>>>>>> ---
>>>>>
>>>>> This seems to be missing a list of changes between v2 and v3. Please
>>>>> add such a list when posting new versions.
>>>>>
>>>>>> +uint64_t since_boot_time(void)
>>>>>> +{
>>>>>> +    uint64_t tsc;
>>>>>> +    uint32_t ver1, ver2;
>>>>>> +    uint64_t system_time;
>>>>>> +    uint64_t old_tsc;
>>>>>> +
>>>>>> +    do
>>>>>> +    {
>>>>>> +        do
>>>>>> +        {
>>>>>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>>>> +            smp_rmb();
>>>>>> +        } while ( (ver1 & 1) == 1 );
>>>>>> +
>>>>>> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>>>>>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>>>>>> +        smp_rmb();
>>>>>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>>>> +        smp_rmb();
>>>>>> +    } while ( ver1 != ver2 );
>>>>>
>>>>> This is still overly complicated IMO, and you have not replied to my
>>>>> question of whether doing the scale_delta below is OK.
>>>>
>>>> About this scale_delta, we discussed with Andrew, and we are going to use
>>>> another version of the function as far as I remember. That's why I am not
>>>> taking care of it for the moment.
>>>
>>> You should send that version then :).
>>>
>>>>>
>>>>> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
>>>>> checking for the version (in order to avoid reading inconsistent data
>>>>> during an update), yet below you read tsc_to_system_mul and
>>>>> tsc_shift.
>>>>>
>>>>
>>>> I'm sorry, I am probably not getting your point here, because I am already
>>>> checking for the version. I was actually checking for the wc_version too in
>>>> the first version of those patches, but after chatting with Andrew, It
>>>> appeared that it was not necessary..
>>>
>>> AFAICT the following should work:
>>>
>>> do
>>> {
>>>       ver1 = 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();
>>>       /* NB: this barrier is probably not needed if rdtsc is serializing. */
>>>       smp_rmb();
>>>
>>>       ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>> } while ( ver2 & 1 || ver1 != ver2 );
>>>
>>
>> Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on every
>> shared_info field accesses ?
>> As far as I understand, we need to do this as most as we can to avoid having
>> completely broken data (or security issues). Am I missing something ?
> 
> ACCESS_ONCE prevents the reordering of the reads, but here AFAICT we
> don't really care about the order in which way they are performed as
> long as they are all done before the read barrier (smp_rmb).
> 
> Note that I used ACCESS_ONCE for the last access to the version field.
> I've done that to prevent the compiler from optimizing the code as:
> 
> } while ( shared_info.vcpu_info[0].time.version & 1 ||
>            ver1 != shared_info.vcpu_info[0].time.version );
> 
> Which would be incorrect, since we want to use the same version data
> for both checks in the while loop condition.
> 

Okay, I really thought that it was also used to ensure that the accesses 
are not splitted into multiple instructions (for optimizations because 
of the loop), and thus put us in trouble if the shared memory was 
modified in between.

Thank you very much !

-- 
Paul

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Roger Pau Monné]

On Tue, Apr 10, 2018 at 12:32:23PM +0200, Paul Semel wrote:
> On 04/10/2018 12: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>
> > > > > > > ---
> > > > > > 
> > > > > > This seems to be missing a list of changes between v2 and v3. Please
> > > > > > add such a list when posting new versions.
> > > > > > 
> > > > > > > +uint64_t since_boot_time(void)
> > > > > > > +{
> > > > > > > +    uint64_t tsc;
> > > > > > > +    uint32_t ver1, ver2;
> > > > > > > +    uint64_t system_time;
> > > > > > > +    uint64_t old_tsc;
> > > > > > > +
> > > > > > > +    do
> > > > > > > +    {
> > > > > > > +        do
> > > > > > > +        {
> > > > > > > +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > > > > > +            smp_rmb();
> > > > > > > +        } while ( (ver1 & 1) == 1 );
> > > > > > > +
> > > > > > > +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
> > > > > > > +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
> > > > > > > +        smp_rmb();
> > > > > > > +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > > > > > +        smp_rmb();
> > > > > > > +    } while ( ver1 != ver2 );
> > > > > > 
> > > > > > This is still overly complicated IMO, and you have not replied to my
> > > > > > question of whether doing the scale_delta below is OK.
> > > > > 
> > > > > About this scale_delta, we discussed with Andrew, and we are going to use
> > > > > another version of the function as far as I remember. That's why I am not
> > > > > taking care of it for the moment.
> > > > 
> > > > You should send that version then :).
> > > > 
> > > > > > 
> > > > > > AFAICT uou _cannot_ access any of the vcpu_time_info fields without
> > > > > > checking for the version (in order to avoid reading inconsistent data
> > > > > > during an update), yet below you read tsc_to_system_mul and
> > > > > > tsc_shift.
> > > > > > 
> > > > > 
> > > > > I'm sorry, I am probably not getting your point here, because I am already
> > > > > checking for the version. I was actually checking for the wc_version too in
> > > > > the first version of those patches, but after chatting with Andrew, It
> > > > > appeared that it was not necessary..
> > > > 
> > > > AFAICT the following should work:
> > > > 
> > > > do
> > > > {
> > > >       ver1 = 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();
> > > >       /* NB: this barrier is probably not needed if rdtsc is serializing. */
> > > >       smp_rmb();
> > > > 
> > > >       ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
> > > > } while ( ver2 & 1 || ver1 != ver2 );
> > > > 
> > > 
> > > Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on every
> > > shared_info field accesses ?
> > > As far as I understand, we need to do this as most as we can to avoid having
> > > completely broken data (or security issues). Am I missing something ?
> > 
> > ACCESS_ONCE prevents the reordering of the reads, but here AFAICT we
> > don't really care about the order in which way they are performed as
> > long as they are all done before the read barrier (smp_rmb).
> > 
> > Note that I used ACCESS_ONCE for the last access to the version field.
> > I've done that to prevent the compiler from optimizing the code as:
> > 
> > } while ( shared_info.vcpu_info[0].time.version & 1 ||
> >            ver1 != shared_info.vcpu_info[0].time.version );
> > 
> > Which would be incorrect, since we want to use the same version data
> > for both checks in the while loop condition.
> > 
> 
> Okay, I really thought that it was also used to ensure that the accesses are
> not splitted into multiple instructions (for optimizations because of the
> loop), and thus put us in trouble if the shared memory was modified in
> between.

If the memory is modified in between either (ver2 & 1) == 1 or ver1 !=
ver2 because that's the protocol between the hypervisor and the guest
in order to update vpcu_time_info, so we will discard the read data
and start the loop again.

Roger.

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

Re: [PATCH v3 1/7] introduce time managment in xtf

[Paul Semel]

On 04/10/2018 12:36 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>
>>>>>>>> ---
>>>>>>>
>>>>>>> This seems to be missing a list of changes between v2 and v3. Please
>>>>>>> add such a list when posting new versions.
>>>>>>>
>>>>>>>> +uint64_t since_boot_time(void)
>>>>>>>> +{
>>>>>>>> +    uint64_t tsc;
>>>>>>>> +    uint32_t ver1, ver2;
>>>>>>>> +    uint64_t system_time;
>>>>>>>> +    uint64_t old_tsc;
>>>>>>>> +
>>>>>>>> +    do
>>>>>>>> +    {
>>>>>>>> +        do
>>>>>>>> +        {
>>>>>>>> +            ver1 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>>>>>> +            smp_rmb();
>>>>>>>> +        } while ( (ver1 & 1) == 1 );
>>>>>>>> +
>>>>>>>> +        system_time = ACCESS_ONCE(shared_info.vcpu_info[0].time.system_time);
>>>>>>>> +        old_tsc = ACCESS_ONCE(shared_info.vcpu_info[0].time.tsc_timestamp);
>>>>>>>> +        smp_rmb();
>>>>>>>> +        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>>>>>> +        smp_rmb();
>>>>>>>> +    } while ( ver1 != ver2 );
>>>>>>>
>>>>>>> This is still overly complicated IMO, and you have not replied to my
>>>>>>> question of whether doing the scale_delta below is OK.
>>>>>>
>>>>>> About this scale_delta, we discussed with Andrew, and we are going to use
>>>>>> another version of the function as far as I remember. That's why I am not
>>>>>> taking care of it for the moment.
>>>>>
>>>>> You should send that version then :).
>>>>>
>>>>>>>
>>>>>>> AFAICT uou _cannot_ access any of the vcpu_time_info fields without
>>>>>>> checking for the version (in order to avoid reading inconsistent data
>>>>>>> during an update), yet below you read tsc_to_system_mul and
>>>>>>> tsc_shift.
>>>>>>>
>>>>>>
>>>>>> I'm sorry, I am probably not getting your point here, because I am already
>>>>>> checking for the version. I was actually checking for the wc_version too in
>>>>>> the first version of those patches, but after chatting with Andrew, It
>>>>>> appeared that it was not necessary..
>>>>>
>>>>> AFAICT the following should work:
>>>>>
>>>>> do
>>>>> {
>>>>>        ver1 = 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();
>>>>>        /* NB: this barrier is probably not needed if rdtsc is serializing. */
>>>>>        smp_rmb();
>>>>>
>>>>>        ver2 = ACCESS_ONCE(shared_info.vcpu_info[0].time.version);
>>>>> } while ( ver2 & 1 || ver1 != ver2 );
>>>>>
>>>>
>>>> Just a (probably dumb) question. Why aren't you doing ACCESS_ONCE on every
>>>> shared_info field accesses ?
>>>> As far as I understand, we need to do this as most as we can to avoid having
>>>> completely broken data (or security issues). Am I missing something ?
>>>
>>> ACCESS_ONCE prevents the reordering of the reads, but here AFAICT we
>>> don't really care about the order in which way they are performed as
>>> long as they are all done before the read barrier (smp_rmb).
>>>
>>> Note that I used ACCESS_ONCE for the last access to the version field.
>>> I've done that to prevent the compiler from optimizing the code as:
>>>
>>> } while ( shared_info.vcpu_info[0].time.version & 1 ||
>>>             ver1 != shared_info.vcpu_info[0].time.version );
>>>
>>> Which would be incorrect, since we want to use the same version data
>>> for both checks in the while loop condition.
>>>
>>
>> Okay, I really thought that it was also used to ensure that the accesses are
>> not splitted into multiple instructions (for optimizations because of the
>> loop), and thus put us in trouble if the shared memory was modified in
>> between.
> 
> If the memory is modified in between either (ver2 & 1) == 1 or ver1 !=
> ver2 because that's the protocol between the hypervisor and the guest
> in order to update vpcu_time_info, so we will discard the read data
> and start the loop again.

Oh okay, I get it now, sorry for missing this !

Thanks,

-- 
Paul

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