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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Wed, Apr 04, 2018 at 03:50:48PM +0000, Pawel Wieczorkiewicz wrote:
> From: Paul Semel <phentex@xxxxxxxxx>
> 
> 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@xxxxxxxxx>
> 
> cr https://code.amazon.com/reviews/CR-786223

This seems like some Amazon internal tracking, which shouldn't be sent
upstream (the webpage is not even accessible from the outside).

> ---
>  build/files.mk     |  1 +
>  common/time.c      | 87 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  include/xtf/time.h | 35 ++++++++++++++++++++++
>  3 files changed, 123 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..11ac168
> --- /dev/null
> +++ b/common/time.c
> @@ -0,0 +1,87 @@
> +#include <xtf/types.h>
> +#include <xtf/traps.h>
> +#include <xtf/time.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;
> +}
> +
> +
> +#if defined(__i386__)
> +uint32_t since_boot_time(void)
> +#else
> +uint64_t since_boot_time(void)
> +#endif

Why is the return type different for 32 vs 64bits?

I see no reason to not return a 64bit value in both cases IMO, and
that would remove the need for all the ifdefery below.

> +{
> +    uint64_t tsc;
> +    uint32_t version, wc_version;
> +#if defined(__i386__)
> +    uint32_t system_time;
> +#else
> +    uint64_t system_time;
> +#endif
> +    uint64_t old_tsc;
> +
> +    do
> +    {
> +        do
> +        {
> +            wc_version = shared_info.wc_version ;
> +            version = shared_info.vcpu_info[0].time.version;
> +        } while ( (version & 1) == 1 || (wc_version & 1) == 1);
> +
> +        system_time = shared_info.vcpu_info[0].time.system_time;
> +        old_tsc = shared_info.vcpu_info[0].time.tsc_timestamp;
> +    } while (
> +            version != shared_info.vcpu_info[0].time.version ||
> +            wc_version != shared_info.wc_version
> +            );
> +
> +    rdtscp(tsc);
> +
> +    system_time += scale_delta(tsc - old_tsc,
> +                               
> shared_info.vcpu_info[0].time.tsc_to_system_mul,
> +                               shared_info.vcpu_info[0].time.tsc_shift);

This seems way more complicated that what's actually needed. First of
all you don't need to check wc_version at all if you are only fetching
the vcpu_time_info fields (wc_version is for the wallclock).

Then AFAICT you are also missing the barriers (or using something like
Linux's ACCESS_ONCE):

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

> +
> +    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..15cbd48
> --- /dev/null
> +++ b/include/xtf/time.h
> @@ -0,0 +1,35 @@
> +/**
> + * @file include/xtf/time.h
> + *
> + * Time management
> + */
> +#ifndef XTF_TIME_H
> +# define XTF_TIME_H
> +
> +#include <xtf/types.h>
> +
> +#define rdtscp(tsc) {\

Why is this called rdtscp when you are actually using rdtsc? (and not
rdtscp?)

> +    uint32_t lo, hi;\
> +    __asm__ volatile("rdtsc": "=a"(lo), "=d"(hi));\
> +    tsc = ((uint64_t)hi << 32) | lo;\

rdtsc is not a serializing instruction, see:

https://marc.info/?l=xen-devel&m=151939254729277

Also this should likely belong in arch/x86/include/arch/lib.h.

Thanks, Roger.

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