[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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@xxxxxxxxx>
> ---
> 
> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.