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

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

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

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

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

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

[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: [PATCH v4 2/7] add current_time function to time manager

[Roger Pau Monné]

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

Re: [PATCH v4 3/7] add gettimeofday function to time managment

[Roger Pau Monné]

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

Re: [PATCH v4 3/7] add gettimeofday function to time managment

[Paul Semel]

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

Re: [PATCH v4 3/7] add gettimeofday function to time managment

[Roger Pau Monné]

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

Re: [PATCH v4 4/7] add nspin_sleep function to time manager

[Roger Pau Monné]

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

Re: [PATCH v4 5/7] add spin_sleep function to time manager

[Roger Pau Monné]

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

Re: [PATCH v4 6/7] add mspin_sleep function to time manager

[Roger Pau Monné]

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

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

[Roger Pau Monné]

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

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

[Paul Semel]

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

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

[Roger Pau Monné]

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