Re: [PATCH] time: widen wallclock seconds to 64 bits

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 29/01/15 15:39, Jan Beulich wrote:
> Linux is in the process of converting their seconds representation to
> 64 bits, so in order to support it consistently we should follow suit
> (which at some point in quite a few years we'd have to do anyway). To
> represent this in struct shared_info we leverage a 32-bit hole in
> x86-64's and arm's variant of the structure; for x86-32 guests the only
> (reasonable) choice we have is to put the extension in struct
> arch_shared_info.
>
> A note on the conditional suppressing the xen_wc_sec_hi helper macro
> definition in the ix86 case for hypervisor and tools: Neither of the
> two actually need this, and its presence causes the tools to fail to
> build (due to the inclusion of both the x86-64 and x86-32 variants of
> the header).
>
> As a secondary change, x86's do_platform_op() gets a pointless
> initializer as well as a pointless assignment of that same variable
> dropped.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

>
> --- a/tools/include/xen-foreign/Makefile
> +++ b/tools/include/xen-foreign/Makefile
> @@ -17,7 +17,7 @@ clean:
>  distclean: clean
>  
>  checker: checker.c $(headers)
> -	$(HOSTCC) $(HOSTCFLAGS) -o $@ $<
> +	$(HOSTCC) $(HOSTCFLAGS) -D__XEN_TOOLS__ -o $@ $<
>  
>  check-headers: checker
>  	./checker > tmp.size
> --- a/tools/include/xen-foreign/reference.size
> +++ b/tools/include/xen-foreign/reference.size
> @@ -9,6 +9,6 @@ vcpu_guest_context        |     344     
>  arch_vcpu_info            |       0       0      24      16
>  vcpu_time_info            |      32      32      32      32
>  vcpu_info                 |      48      48      64      64
> -arch_shared_info          |       0       0      24      48
> -shared_info               |    1088    1088    2340    3136
> +arch_shared_info          |       0       0      28      48
> +shared_info               |    1088    1088    2344    3136
>  
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -239,7 +239,7 @@ void update_vcpu_system_time(struct vcpu
>      /* XXX update shared_info->wc_* */
>  }
>  
> -void domain_set_time_offset(struct domain *d, int32_t time_offset_seconds)
> +void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
>  {
>      d->time_offset_seconds = time_offset_seconds;
>      /* XXX update guest visible wallclock time */
> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -187,7 +187,7 @@ static void resource_access(void *info)
>  
>  ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>  {
> -    ret_t ret = 0;
> +    ret_t ret;
>      struct xen_platform_op curop, *op = &curop;
>  
>      if ( copy_from_guest(op, u_xenpf_op, 1) )
> @@ -212,14 +212,20 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PA
>  
>      switch ( op->cmd )
>      {
> -    case XENPF_settime:
> -    {
> -        do_settime(op->u.settime.secs, 
> -                   op->u.settime.nsecs, 
> -                   op->u.settime.system_time);
> -        ret = 0;
> -    }
> -    break;
> +    case XENPF_settime32:
> +        do_settime(op->u.settime32.secs,
> +                   op->u.settime32.nsecs,
> +                   op->u.settime32.system_time);
> +        break;
> +
> +    case XENPF_settime64:
> +        if ( likely(!op->u.settime64.mbz) )
> +            do_settime(op->u.settime64.secs,
> +                       op->u.settime64.nsecs,
> +                       op->u.settime64.system_time);
> +        else
> +            ret = -EINVAL;
> +        break;
>  
>      case XENPF_add_memtype:
>      {
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -47,7 +47,8 @@ string_param("clocksource", opt_clocksou
>  unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
>  DEFINE_SPINLOCK(rtc_lock);
>  unsigned long pit0_ticks;
> -static u32 wc_sec, wc_nsec; /* UTC time at last 'time update'. */
> +static unsigned long wc_sec; /* UTC time at last 'time update'. */
> +static unsigned int wc_nsec;
>  static DEFINE_SPINLOCK(wc_lock);
>  
>  struct cpu_time {
> @@ -902,6 +903,7 @@ void force_update_vcpu_system_time(struc
>  void update_domain_wallclock_time(struct domain *d)
>  {
>      uint32_t *wc_version;
> +    unsigned long sec;
>  
>      spin_lock(&wc_lock);
>  
> @@ -909,8 +911,19 @@ void update_domain_wallclock_time(struct
>      *wc_version = version_update_begin(*wc_version);
>      wmb();
>  
> -    shared_info(d, wc_sec)  = wc_sec + d->time_offset_seconds;
> -    shared_info(d, wc_nsec) = wc_nsec;
> +    sec = wc_sec + d->time_offset_seconds;
> +    if ( likely(!has_32bit_shinfo(d)) )
> +    {
> +        d->shared_info->native.wc_sec    = sec;
> +        d->shared_info->native.wc_nsec   = wc_nsec;
> +        d->shared_info->native.wc_sec_hi = sec >> 32;
> +    }
> +    else
> +    {
> +        d->shared_info->compat.wc_sec         = sec;
> +        d->shared_info->compat.wc_nsec        = wc_nsec;
> +        d->shared_info->compat.arch.wc_sec_hi = sec >> 32;
> +    }
>  
>      wmb();
>      *wc_version = version_update_end(*wc_version);
> @@ -931,7 +944,7 @@ static void update_domain_rtc(void)
>      rcu_read_unlock(&domlist_read_lock);
>  }
>  
> -void domain_set_time_offset(struct domain *d, int32_t time_offset_seconds)
> +void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
>  {
>      d->time_offset_seconds = time_offset_seconds;
>      if ( is_hvm_domain(d) )
> @@ -976,13 +989,13 @@ int cpu_frequency_change(u64 freq)
>  }
>  
>  /* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
> -void do_settime(unsigned long secs, unsigned long nsecs, u64 system_time_base)
> +void do_settime(unsigned long secs, unsigned int nsecs, u64 system_time_base)
>  {
>      u64 x;
>      u32 y;
>      struct domain *d;
>  
> -    x = SECONDS(secs) + (u64)nsecs - system_time_base;
> +    x = SECONDS(secs) + nsecs - system_time_base;
>      y = do_div(x, 1000000000);
>  
>      spin_lock(&wc_lock);
> --- a/xen/include/public/arch-arm.h
> +++ b/xen/include/public/arch-arm.h
> @@ -165,6 +165,7 @@
>  
>  #define XEN_HYPERCALL_TAG   0XEA1
>  
> +#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
>  #define uint64_aligned_t uint64_t __attribute__((aligned(8)))
>  
>  #ifndef __ASSEMBLY__
> --- a/xen/include/public/arch-x86/xen.h
> +++ b/xen/include/public/arch-x86/xen.h
> @@ -255,6 +255,10 @@ struct arch_shared_info {
>      unsigned long p2m_cr3;         /* cr3 value of the p2m address space */
>      unsigned long p2m_vaddr;       /* virtual address of the p2m list */
>      unsigned long p2m_generation;  /* generation count of p2m mapping */
> +#ifdef __i386__
> +    /* There's no room for this field in the generic structure. */
> +    uint32_t wc_sec_hi;
> +#endif
>  };
>  typedef struct arch_shared_info arch_shared_info_t;
>  
> --- a/xen/include/public/arch-x86/xen-x86_32.h
> +++ b/xen/include/public/arch-x86/xen-x86_32.h
> @@ -104,6 +104,7 @@
>      do { if ( sizeof(hnd) == 8 ) *(uint64_t *)&(hnd) = 0;   \
>           (hnd).p = val;                                     \
>      } while ( 0 )
> +#define  int64_aligned_t  int64_t __attribute__((aligned(8)))
>  #define uint64_aligned_t uint64_t __attribute__((aligned(8)))
>  #define __XEN_GUEST_HANDLE_64(name) __guest_handle_64_ ## name
>  #define XEN_GUEST_HANDLE_64(name) __XEN_GUEST_HANDLE_64(name)
> --- a/xen/include/public/domctl.h
> +++ b/xen/include/public/domctl.h
> @@ -37,7 +37,7 @@
>  #include "hvm/save.h"
>  #include "memory.h"
>  
> -#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000a
> +#define XEN_DOMCTL_INTERFACE_VERSION 0x0000000b
>  
>  /*
>   * NB. xen_domctl.domain is an IN/OUT parameter for this operation.
> @@ -449,7 +449,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_domctl_hyper
>  
>  /* XEN_DOMCTL_settimeoffset */
>  struct xen_domctl_settimeoffset {
> -    int32_t  time_offset_seconds; /* applied to domain wallclock time */
> +    int64_aligned_t time_offset_seconds; /* applied to domain wallclock time */
>  };
>  typedef struct xen_domctl_settimeoffset xen_domctl_settimeoffset_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_domctl_settimeoffset_t);
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -35,13 +35,28 @@
>   * Set clock such that it would read <secs,nsecs> after 00:00:00 UTC,
>   * 1 January, 1970 if the current system time was <system_time>.
>   */
> -#define XENPF_settime             17
> -struct xenpf_settime {
> +#define XENPF_settime32           17
> +struct xenpf_settime32 {
>      /* IN variables. */
>      uint32_t secs;
>      uint32_t nsecs;
>      uint64_t system_time;
>  };
> +#define XENPF_settime64           62
> +struct xenpf_settime64 {
> +    /* IN variables. */
> +    uint64_t secs;
> +    uint32_t nsecs;
> +    uint32_t mbz;
> +    uint64_t system_time;
> +};
> +#if __XEN_INTERFACE_VERSION__ < 0x00040600
> +#define XENPF_settime XENPF_settime32
> +#define xenpf_settime xenpf_settime32
> +#else
> +#define XENPF_settime XENPF_settime64
> +#define xenpf_settime xenpf_settime64
> +#endif
>  typedef struct xenpf_settime xenpf_settime_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_settime_t);
>  
> @@ -579,6 +594,8 @@ struct xen_platform_op {
>      uint32_t interface_version; /* XENPF_INTERFACE_VERSION */
>      union {
>          struct xenpf_settime           settime;
> +        struct xenpf_settime32         settime32;
> +        struct xenpf_settime64         settime64;
>          struct xenpf_add_memtype       add_memtype;
>          struct xenpf_del_memtype       del_memtype;
>          struct xenpf_read_memtype      read_memtype;
> --- a/xen/include/public/xen.h
> +++ b/xen/include/public/xen.h
> @@ -682,6 +682,12 @@ struct shared_info {
>      uint32_t wc_version;      /* Version counter: see vcpu_time_info_t. */
>      uint32_t wc_sec;          /* Secs  00:00:00 UTC, Jan 1, 1970.  */
>      uint32_t wc_nsec;         /* Nsecs 00:00:00 UTC, Jan 1, 1970.  */
> +#if !defined(__i386__)
> +    uint32_t wc_sec_hi;
> +# define xen_wc_sec_hi wc_sec_hi
> +#elif !defined(__XEN__) && !defined(__XEN_TOOLS__)
> +# define xen_wc_sec_hi arch.wc_sec_hi
> +#endif
>  
>      struct arch_shared_info arch;
>  
> @@ -870,6 +876,9 @@ __DEFINE_XEN_GUEST_HANDLE(uint64, uint64
>  /* Default definitions for macros used by domctl/sysctl. */
>  #if defined(__XEN__) || defined(__XEN_TOOLS__)
>  
> +#ifndef int64_aligned_t
> +#define int64_aligned_t int64_t
> +#endif
>  #ifndef uint64_aligned_t
>  #define uint64_aligned_t uint64_t
>  #endif
> --- a/xen/include/xen/sched.h
> +++ b/xen/include/xen/sched.h
> @@ -359,7 +359,7 @@ struct domain
>      /* Domain is paused by controller software? */
>      int              controller_pause_count;
>  
> -    int32_t          time_offset_seconds;
> +    int64_t          time_offset_seconds;
>  
>  #ifdef HAS_PASSTHROUGH
>      /* Does this guest need iommu mappings (-1 meaning "being set up")? */
> --- a/xen/include/xen/time.h
> +++ b/xen/include/xen/time.h
> @@ -65,11 +65,11 @@ extern void update_vcpu_system_time(stru
>  extern void update_domain_wallclock_time(struct domain *d);
>  
>  extern void do_settime(
> -    unsigned long secs, unsigned long nsecs, u64 system_time_base);
> +    unsigned long secs, unsigned int nsecs, u64 system_time_base);
>  
>  extern void send_timer_event(struct vcpu *v);
>  
> -void domain_set_time_offset(struct domain *d, int32_t time_offset_seconds);
> +void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds);
>  
>  #include <asm/time.h>
>  
> --- a/xen/xsm/flask/hooks.c
> +++ b/xen/xsm/flask/hooks.c
> @@ -1365,7 +1365,8 @@ static int flask_platform_op(uint32_t op
>          return 0;
>  #endif
>  
> -    case XENPF_settime:
> +    case XENPF_settime32:
> +    case XENPF_settime64:
>          return domain_has_xen(current->domain, XEN__SETTIME);
>  
>      case XENPF_add_memtype:
> --- a/xen/xsm/flask/policy/access_vectors
> +++ b/xen/xsm/flask/policy/access_vectors
> @@ -8,7 +8,8 @@
>  # executing the hypercall, and the target is the xen initial sid (type xen_t).
>  class xen
>  {
> -# XENPF_settime
> +# XENPF_settime32
> +# XENPF_settime64
>      settime
>  # XEN_SYSCTL_tbuf_op
>      tbufcontrol
>
>