On Tue, Apr 10, 2018 at 09:16:56PM +0200, Paul Semel wrote:
> this function returns the "epoch" time
>
> Signed-off-by: Paul Semel <phen...@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>
Advertising
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
