[PATCH 0/2] wallclock time on arm

[PATCH 0/2] wallclock time on arm

[Stefano Stabellini]

Hi all,

this small series enables the wallclock time on arm and it consists
mostly in code movement from x86 to common.


Stefano Stabellini (2):
      xen: move wallclock functions from x86 to common
      arm: export platform_op XENPF_settime

 xen/arch/arm/Makefile             |    1 +
 xen/arch/arm/domain.c             |    3 ++
 xen/arch/arm/platform_hypercall.c |   62 ++++++++++++++++++++++++
 xen/arch/arm/time.c               |    5 --
 xen/arch/arm/traps.c              |    1 +
 xen/arch/x86/time.c               |   92 +-----------------------------------
 xen/common/time.c                 |   94 +++++++++++++++++++++++++++++++++++++
 xen/include/xsm/dummy.h           |   12 ++---
 xen/include/xsm/xsm.h             |   13 ++---
 9 files changed, 175 insertions(+), 108 deletions(-)
 create mode 100644 xen/arch/arm/platform_hypercall.c

Cheers,

Stefano

[PATCH 1/2] xen: move wallclock functions from x86 to common

[Stefano Stabellini]

Remove dummy arm implementation of wallclock_time.
Use shared_info() in common code rather than x86-ism to access it.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/time.c |    5 ---
 xen/arch/x86/time.c |   92 +------------------------------------------------
 xen/common/time.c   |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 95 insertions(+), 96 deletions(-)

diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
index 5ded30c..6207615 100644
--- a/xen/arch/arm/time.c
+++ b/xen/arch/arm/time.c
@@ -280,11 +280,6 @@ void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
     /* XXX update guest visible wallclock time */
 }
 
-struct tm wallclock_time(uint64_t *ns)
-{
-    return (struct tm) { 0 };
-}
-
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index bbb7e6c..764d7dc 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -47,9 +47,6 @@ string_param("clocksource", opt_clocksource);
 unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
 DEFINE_SPINLOCK(rtc_lock);
 unsigned long pit0_ticks;
-static unsigned long wc_sec; /* UTC time at last 'time update'. */
-static unsigned int wc_nsec;
-static DEFINE_SPINLOCK(wc_lock);
 
 struct cpu_time {
     u64 local_tsc_stamp;
@@ -900,37 +897,6 @@ void force_update_vcpu_system_time(struct vcpu *v)
     __update_vcpu_system_time(v, 1);
 }
 
-void update_domain_wallclock_time(struct domain *d)
-{
-    uint32_t *wc_version;
-    unsigned long sec;
-
-    spin_lock(&wc_lock);
-
-    wc_version = &shared_info(d, wc_version);
-    *wc_version = version_update_begin(*wc_version);
-    wmb();
-
-    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);
-
-    spin_unlock(&wc_lock);
-}
-
 static void update_domain_rtc(void)
 {
     struct domain *d;
@@ -988,27 +954,6 @@ int cpu_frequency_change(u64 freq)
     return 0;
 }
 
-/* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
-void do_settime(unsigned long secs, unsigned int nsecs, u64 system_time_base)
-{
-    u64 x;
-    u32 y;
-    struct domain *d;
-
-    x = SECONDS(secs) + nsecs - system_time_base;
-    y = do_div(x, 1000000000);
-
-    spin_lock(&wc_lock);
-    wc_sec  = x;
-    wc_nsec = y;
-    spin_unlock(&wc_lock);
-
-    rcu_read_lock(&domlist_read_lock);
-    for_each_domain ( d )
-        update_domain_wallclock_time(d);
-    rcu_read_unlock(&domlist_read_lock);
-}
-
 /* Per-CPU communication between rendezvous IRQ and softirq handler. */
 struct cpu_calibration {
     u64 local_tsc_stamp;
@@ -1608,25 +1553,6 @@ void send_timer_event(struct vcpu *v)
     send_guest_vcpu_virq(v, VIRQ_TIMER);
 }
 
-/* Return secs after 00:00:00 localtime, 1 January, 1970. */
-unsigned long get_localtime(struct domain *d)
-{
-    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL 
-        + d->time_offset_seconds;
-}
-
-/* Return microsecs after 00:00:00 localtime, 1 January, 1970. */
-uint64_t get_localtime_us(struct domain *d)
-{
-    return (SECONDS(wc_sec + d->time_offset_seconds) + wc_nsec + NOW())
-           / 1000UL;
-}
-
-unsigned long get_sec(void)
-{
-    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL;
-}
-
 /* "cmos_utc_offset" is the difference between UTC time and CMOS time. */
 static long cmos_utc_offset; /* in seconds */
 
@@ -1635,7 +1561,7 @@ int time_suspend(void)
     if ( smp_processor_id() == 0 )
     {
         cmos_utc_offset = -get_cmos_time();
-        cmos_utc_offset += (wc_sec + (wc_nsec + NOW()) / 1000000000ULL);
+        cmos_utc_offset += get_sec();
         kill_timer(&calibration_timer);
 
         /* Sync platform timer stamps. */
@@ -1715,22 +1641,6 @@ int hwdom_pit_access(struct ioreq *ioreq)
     return 0;
 }
 
-struct tm wallclock_time(uint64_t *ns)
-{
-    uint64_t seconds, nsec;
-
-    if ( !wc_sec )
-        return (struct tm) { 0 };
-
-    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
-    nsec = do_div(seconds, 1000000000);
-
-    if ( ns )
-        *ns = nsec;
-
-    return gmtime(seconds);
-}
-
 /*
  * PV SoftTSC Emulation.
  */
diff --git a/xen/common/time.c b/xen/common/time.c
index 29fdf52..306c5dc 100644
--- a/xen/common/time.c
+++ b/xen/common/time.c
@@ -16,7 +16,13 @@
  */
 
 #include <xen/config.h>
+#include <xen/sched.h>
+#include <xen/shared.h>
+#include <xen/spinlock.h>
 #include <xen/time.h>
+#include <asm/div64.h>
+#include <asm/domain.h>
+
 
 /* Nonzero if YEAR is a leap year (every 4 years,
    except every 100th isn't, and every 400th is).  */
@@ -34,6 +40,10 @@ const unsigned short int __mon_lengths[2][12] = {
 #define SECS_PER_HOUR (60 * 60)
 #define SECS_PER_DAY  (SECS_PER_HOUR * 24)
 
+static unsigned long wc_sec; /* UTC time at last 'time update'. */
+static unsigned int wc_nsec;
+static DEFINE_SPINLOCK(wc_lock);
+
 struct tm gmtime(unsigned long t)
 {
     struct tm tbuf;
@@ -85,3 +95,87 @@ struct tm gmtime(unsigned long t)
 
     return tbuf;
 }
+
+/* Explicitly OR with 1 just in case version number gets out of sync. */
+#define version_update_begin(v) (((v)+1)|1)
+#define version_update_end(v)   ((v)+1)
+
+void update_domain_wallclock_time(struct domain *d)
+{
+    uint32_t *wc_version;
+    unsigned long sec;
+
+    spin_lock(&wc_lock);
+
+    wc_version = &shared_info(d, wc_version);
+    *wc_version = version_update_begin(*wc_version);
+    wmb();
+
+    sec = wc_sec + d->time_offset_seconds;
+    shared_info(d, wc_sec)    = sec;
+    shared_info(d, wc_nsec)   = wc_nsec;
+    shared_info(d, wc_sec_hi) = sec >> 32;
+
+    wmb();
+    *wc_version = version_update_end(*wc_version);
+
+    spin_unlock(&wc_lock);
+}
+
+/* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
+void do_settime(unsigned long secs, unsigned int nsecs, u64 system_time_base)
+{
+    u64 x;
+    u32 y;
+    struct domain *d;
+
+    x = SECONDS(secs) + nsecs - system_time_base;
+    y = do_div(x, 1000000000);
+
+    spin_lock(&wc_lock);
+    wc_sec  = x;
+    wc_nsec = y;
+    spin_unlock(&wc_lock);
+
+    rcu_read_lock(&domlist_read_lock);
+    for_each_domain ( d )
+        update_domain_wallclock_time(d);
+    rcu_read_unlock(&domlist_read_lock);
+}
+
+/* Return secs after 00:00:00 localtime, 1 January, 1970. */
+unsigned long get_localtime(struct domain *d)
+{
+    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL 
+        + d->time_offset_seconds;
+}
+
+/* Return microsecs after 00:00:00 localtime, 1 January, 1970. */
+uint64_t get_localtime_us(struct domain *d)
+{
+    return (SECONDS(wc_sec + d->time_offset_seconds) + wc_nsec + NOW())
+           / 1000UL;
+}
+
+unsigned long get_sec(void)
+{
+    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL;
+}
+
+struct tm wallclock_time(uint64_t *ns)
+{
+    uint64_t seconds, nsec;
+
+    if ( !wc_sec )
+        return (struct tm) { 0 };
+
+    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
+    nsec = do_div(seconds, 1000000000);
+
+    if ( ns )
+        *ns = nsec;
+
+    return gmtime(seconds);
+}
+
+
-- 
1.7.10.4

Re: [PATCH 1/2] xen: move wallclock functions from x86 to common

[Julien Grall]

Hi,

You forgot to CC the x86 maintainers.

Regards,

On 05/11/15 16:57, Stefano Stabellini wrote:
> Remove dummy arm implementation of wallclock_time.
> Use shared_info() in common code rather than x86-ism to access it.
> 
> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/time.c |    5 ---
>  xen/arch/x86/time.c |   92 +------------------------------------------------
>  xen/common/time.c   |   94 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 95 insertions(+), 96 deletions(-)
> 
> diff --git a/xen/arch/arm/time.c b/xen/arch/arm/time.c
> index 5ded30c..6207615 100644
> --- a/xen/arch/arm/time.c
> +++ b/xen/arch/arm/time.c
> @@ -280,11 +280,6 @@ void domain_set_time_offset(struct domain *d, int64_t time_offset_seconds)
>      /* XXX update guest visible wallclock time */
>  }
>  
> -struct tm wallclock_time(uint64_t *ns)
> -{
> -    return (struct tm) { 0 };
> -}
> -
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> index bbb7e6c..764d7dc 100644
> --- a/xen/arch/x86/time.c
> +++ b/xen/arch/x86/time.c
> @@ -47,9 +47,6 @@ string_param("clocksource", opt_clocksource);
>  unsigned long __read_mostly cpu_khz;  /* CPU clock frequency in kHz. */
>  DEFINE_SPINLOCK(rtc_lock);
>  unsigned long pit0_ticks;
> -static unsigned long wc_sec; /* UTC time at last 'time update'. */
> -static unsigned int wc_nsec;
> -static DEFINE_SPINLOCK(wc_lock);
>  
>  struct cpu_time {
>      u64 local_tsc_stamp;
> @@ -900,37 +897,6 @@ void force_update_vcpu_system_time(struct vcpu *v)
>      __update_vcpu_system_time(v, 1);
>  }
>  
> -void update_domain_wallclock_time(struct domain *d)
> -{
> -    uint32_t *wc_version;
> -    unsigned long sec;
> -
> -    spin_lock(&wc_lock);
> -
> -    wc_version = &shared_info(d, wc_version);
> -    *wc_version = version_update_begin(*wc_version);
> -    wmb();
> -
> -    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);
> -
> -    spin_unlock(&wc_lock);
> -}
> -
>  static void update_domain_rtc(void)
>  {
>      struct domain *d;
> @@ -988,27 +954,6 @@ int cpu_frequency_change(u64 freq)
>      return 0;
>  }
>  
> -/* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
> -void do_settime(unsigned long secs, unsigned int nsecs, u64 system_time_base)
> -{
> -    u64 x;
> -    u32 y;
> -    struct domain *d;
> -
> -    x = SECONDS(secs) + nsecs - system_time_base;
> -    y = do_div(x, 1000000000);
> -
> -    spin_lock(&wc_lock);
> -    wc_sec  = x;
> -    wc_nsec = y;
> -    spin_unlock(&wc_lock);
> -
> -    rcu_read_lock(&domlist_read_lock);
> -    for_each_domain ( d )
> -        update_domain_wallclock_time(d);
> -    rcu_read_unlock(&domlist_read_lock);
> -}
> -
>  /* Per-CPU communication between rendezvous IRQ and softirq handler. */
>  struct cpu_calibration {
>      u64 local_tsc_stamp;
> @@ -1608,25 +1553,6 @@ void send_timer_event(struct vcpu *v)
>      send_guest_vcpu_virq(v, VIRQ_TIMER);
>  }
>  
> -/* Return secs after 00:00:00 localtime, 1 January, 1970. */
> -unsigned long get_localtime(struct domain *d)
> -{
> -    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL 
> -        + d->time_offset_seconds;
> -}
> -
> -/* Return microsecs after 00:00:00 localtime, 1 January, 1970. */
> -uint64_t get_localtime_us(struct domain *d)
> -{
> -    return (SECONDS(wc_sec + d->time_offset_seconds) + wc_nsec + NOW())
> -           / 1000UL;
> -}
> -
> -unsigned long get_sec(void)
> -{
> -    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL;
> -}
> -
>  /* "cmos_utc_offset" is the difference between UTC time and CMOS time. */
>  static long cmos_utc_offset; /* in seconds */
>  
> @@ -1635,7 +1561,7 @@ int time_suspend(void)
>      if ( smp_processor_id() == 0 )
>      {
>          cmos_utc_offset = -get_cmos_time();
> -        cmos_utc_offset += (wc_sec + (wc_nsec + NOW()) / 1000000000ULL);
> +        cmos_utc_offset += get_sec();
>          kill_timer(&calibration_timer);
>  
>          /* Sync platform timer stamps. */
> @@ -1715,22 +1641,6 @@ int hwdom_pit_access(struct ioreq *ioreq)
>      return 0;
>  }
>  
> -struct tm wallclock_time(uint64_t *ns)
> -{
> -    uint64_t seconds, nsec;
> -
> -    if ( !wc_sec )
> -        return (struct tm) { 0 };
> -
> -    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
> -    nsec = do_div(seconds, 1000000000);
> -
> -    if ( ns )
> -        *ns = nsec;
> -
> -    return gmtime(seconds);
> -}
> -
>  /*
>   * PV SoftTSC Emulation.
>   */
> diff --git a/xen/common/time.c b/xen/common/time.c
> index 29fdf52..306c5dc 100644
> --- a/xen/common/time.c
> +++ b/xen/common/time.c
> @@ -16,7 +16,13 @@
>   */
>  
>  #include <xen/config.h>
> +#include <xen/sched.h>
> +#include <xen/shared.h>
> +#include <xen/spinlock.h>
>  #include <xen/time.h>
> +#include <asm/div64.h>
> +#include <asm/domain.h>
> +
>  
>  /* Nonzero if YEAR is a leap year (every 4 years,
>     except every 100th isn't, and every 400th is).  */
> @@ -34,6 +40,10 @@ const unsigned short int __mon_lengths[2][12] = {
>  #define SECS_PER_HOUR (60 * 60)
>  #define SECS_PER_DAY  (SECS_PER_HOUR * 24)
>  
> +static unsigned long wc_sec; /* UTC time at last 'time update'. */
> +static unsigned int wc_nsec;
> +static DEFINE_SPINLOCK(wc_lock);
> +
>  struct tm gmtime(unsigned long t)
>  {
>      struct tm tbuf;
> @@ -85,3 +95,87 @@ struct tm gmtime(unsigned long t)
>  
>      return tbuf;
>  }
> +
> +/* Explicitly OR with 1 just in case version number gets out of sync. */
> +#define version_update_begin(v) (((v)+1)|1)
> +#define version_update_end(v)   ((v)+1)
> +
> +void update_domain_wallclock_time(struct domain *d)
> +{
> +    uint32_t *wc_version;
> +    unsigned long sec;
> +
> +    spin_lock(&wc_lock);
> +
> +    wc_version = &shared_info(d, wc_version);
> +    *wc_version = version_update_begin(*wc_version);
> +    wmb();
> +
> +    sec = wc_sec + d->time_offset_seconds;
> +    shared_info(d, wc_sec)    = sec;
> +    shared_info(d, wc_nsec)   = wc_nsec;
> +    shared_info(d, wc_sec_hi) = sec >> 32;
> +
> +    wmb();
> +    *wc_version = version_update_end(*wc_version);
> +
> +    spin_unlock(&wc_lock);
> +}
> +
> +/* Set clock to <secs,usecs> after 00:00:00 UTC, 1 January, 1970. */
> +void do_settime(unsigned long secs, unsigned int nsecs, u64 system_time_base)
> +{
> +    u64 x;
> +    u32 y;
> +    struct domain *d;
> +
> +    x = SECONDS(secs) + nsecs - system_time_base;
> +    y = do_div(x, 1000000000);
> +
> +    spin_lock(&wc_lock);
> +    wc_sec  = x;
> +    wc_nsec = y;
> +    spin_unlock(&wc_lock);
> +
> +    rcu_read_lock(&domlist_read_lock);
> +    for_each_domain ( d )
> +        update_domain_wallclock_time(d);
> +    rcu_read_unlock(&domlist_read_lock);
> +}
> +
> +/* Return secs after 00:00:00 localtime, 1 January, 1970. */
> +unsigned long get_localtime(struct domain *d)
> +{
> +    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL 
> +        + d->time_offset_seconds;
> +}
> +
> +/* Return microsecs after 00:00:00 localtime, 1 January, 1970. */
> +uint64_t get_localtime_us(struct domain *d)
> +{
> +    return (SECONDS(wc_sec + d->time_offset_seconds) + wc_nsec + NOW())
> +           / 1000UL;
> +}
> +
> +unsigned long get_sec(void)
> +{
> +    return wc_sec + (wc_nsec + NOW()) / 1000000000ULL;
> +}
> +
> +struct tm wallclock_time(uint64_t *ns)
> +{
> +    uint64_t seconds, nsec;
> +
> +    if ( !wc_sec )
> +        return (struct tm) { 0 };
> +
> +    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
> +    nsec = do_div(seconds, 1000000000);
> +
> +    if ( ns )
> +        *ns = nsec;
> +
> +    return gmtime(seconds);
> +}
> +
> +
> 


-- 
Julien Grall

Re: [PATCH 1/2] xen: move wallclock functions from x86 to common

[Jan Beulich]

>>> On 05.11.15 at 17:57, <stefano.stabellini@eu.citrix.com> wrote:
> --- a/xen/common/time.c
> +++ b/xen/common/time.c
> @@ -16,7 +16,13 @@
>   */
>  
>  #include <xen/config.h>
> +#include <xen/sched.h>
> +#include <xen/shared.h>
> +#include <xen/spinlock.h>
>  #include <xen/time.h>
> +#include <asm/div64.h>
> +#include <asm/domain.h>
> +
>  
>  /* Nonzero if YEAR is a leap year (every 4 years,

Stray blank line being added.

Also please take the opportunity to remove xen/config.h here.

> @@ -85,3 +95,87 @@ struct tm gmtime(unsigned long t)
>  
>      return tbuf;
>  }
> +
> +/* Explicitly OR with 1 just in case version number gets out of sync. */
> +#define version_update_begin(v) (((v)+1)|1)
> +#define version_update_end(v)   ((v)+1)

This should be moved to a header instead of getting defined a second
time here. Also please add spaces to match our coding style.

> +struct tm wallclock_time(uint64_t *ns)
> +{
> +    uint64_t seconds, nsec;
> +
> +    if ( !wc_sec )
> +        return (struct tm) { 0 };
> +
> +    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
> +    nsec = do_div(seconds, 1000000000);
> +
> +    if ( ns )
> +        *ns = nsec;
> +
> +    return gmtime(seconds);
> +}
> +
> +

Stray blank lines again.

Jan

Re: [PATCH 1/2] xen: move wallclock functions from x86 to common

[Stefano Stabellini]

On Thu, 5 Nov 2015, Jan Beulich wrote:
> >>> On 05.11.15 at 17:57, <stefano.stabellini@eu.citrix.com> wrote:
> > --- a/xen/common/time.c
> > +++ b/xen/common/time.c
> > @@ -16,7 +16,13 @@
> >   */
> >  
> >  #include <xen/config.h>
> > +#include <xen/sched.h>
> > +#include <xen/shared.h>
> > +#include <xen/spinlock.h>
> >  #include <xen/time.h>
> > +#include <asm/div64.h>
> > +#include <asm/domain.h>
> > +
> >  
> >  /* Nonzero if YEAR is a leap year (every 4 years,
> 
> Stray blank line being added.
> 
> Also please take the opportunity to remove xen/config.h here.

OK


> > @@ -85,3 +95,87 @@ struct tm gmtime(unsigned long t)
> >  
> >      return tbuf;
> >  }
> > +
> > +/* Explicitly OR with 1 just in case version number gets out of sync. */
> > +#define version_update_begin(v) (((v)+1)|1)
> > +#define version_update_end(v)   ((v)+1)
> 
> This should be moved to a header instead of getting defined a second
> time here. Also please add spaces to match our coding style.

OK


> > +struct tm wallclock_time(uint64_t *ns)
> > +{
> > +    uint64_t seconds, nsec;
> > +
> > +    if ( !wc_sec )
> > +        return (struct tm) { 0 };
> > +
> > +    seconds = NOW() + SECONDS(wc_sec) + wc_nsec;
> > +    nsec = do_div(seconds, 1000000000);
> > +
> > +    if ( ns )
> > +        *ns = nsec;
> > +
> > +    return gmtime(seconds);
> > +}
> > +
> > +
> 
> Stray blank lines again.

OK

[PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini]

Call update_domain_wallclock_time at domain initialization.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
---
 xen/arch/arm/Makefile             |    1 +
 xen/arch/arm/domain.c             |    3 ++
 xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
 xen/arch/arm/traps.c              |    1 +
 xen/include/xsm/dummy.h           |   12 +++----
 xen/include/xsm/xsm.h             |   13 ++++----
 6 files changed, 80 insertions(+), 12 deletions(-)
 create mode 100644 xen/arch/arm/platform_hypercall.c

diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
index 1ef39f7..240aa29 100644
--- a/xen/arch/arm/Makefile
+++ b/xen/arch/arm/Makefile
@@ -23,6 +23,7 @@ obj-y += percpu.o
 obj-y += guestcopy.o
 obj-y += physdev.o
 obj-y += platform.o
+obj-y += platform_hypercall.o
 obj-y += setup.o
 obj-y += bootfdt.o
 obj-y += time.o
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index b2bfc7d..ac9b1b3 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -742,6 +742,9 @@ int arch_set_info_guest(
     v->arch.ttbr1 = ctxt->ttbr1;
     v->arch.ttbcr = ctxt->ttbcr;
 
+    if ( v->vcpu_id == 0 )
+        update_domain_wallclock_time(v->domain);
+
     v->is_initialised = 1;
 
     if ( ctxt->flags & VGCF_online )
diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
new file mode 100644
index 0000000..f60d7b3
--- /dev/null
+++ b/xen/arch/arm/platform_hypercall.c
@@ -0,0 +1,62 @@
+/******************************************************************************
+ * platform_hypercall.c
+ * 
+ * Hardware platform operations. Intended for use by domain-0 kernel.
+ * 
+ * Copyright (c) 2015, Citrix
+ */
+
+#include <xen/config.h>
+#include <xen/types.h>
+#include <xen/sched.h>
+#include <xen/guest_access.h>
+#include <xen/spinlock.h>
+#include <public/platform.h>
+#include <xsm/xsm.h>
+#include <asm/current.h>
+#include <asm/event.h>
+
+DEFINE_SPINLOCK(xenpf_lock);
+
+long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
+{
+    long ret;
+    struct xen_platform_op curop, *op = &curop;
+
+    if ( copy_from_guest(op, u_xenpf_op, 1) )
+        return -EFAULT;
+
+    if ( op->interface_version != XENPF_INTERFACE_VERSION )
+        return -EACCES;
+
+    ret = xsm_platform_op(XSM_PRIV, op->cmd);
+    if ( ret )
+        return ret;
+
+    spin_lock(&xenpf_lock);
+
+    switch ( op->cmd )
+    {
+    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;
+
+    default:
+        ret = -ENOSYS;
+        break;
+    }
+
+    spin_unlock(&xenpf_lock);
+    return ret;
+}
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index 9d2bd6a..c49bd3f 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1233,6 +1233,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
     HYPERCALL(multicall, 2),
+    HYPERCALL(platform_op, 1),
     HYPERCALL_ARM(vcpu_op, 3),
 };
 
diff --git a/xen/include/xsm/dummy.h b/xen/include/xsm/dummy.h
index 9fe372c..aec5a9b 100644
--- a/xen/include/xsm/dummy.h
+++ b/xen/include/xsm/dummy.h
@@ -583,6 +583,12 @@ static XSM_INLINE int xsm_mem_sharing(XSM_DEFAULT_ARG struct domain *d)
     return xsm_default_action(action, current->domain, d);
 }
 #endif
+ 
+static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
+{
+    XSM_ASSERT_ACTION(XSM_PRIV);
+    return xsm_default_action(action, current->domain, NULL);
+}
 
 #ifdef CONFIG_X86
 static XSM_INLINE int xsm_do_mca(XSM_DEFAULT_VOID)
@@ -639,12 +645,6 @@ static XSM_INLINE int xsm_apic(XSM_DEFAULT_ARG struct domain *d, int cmd)
     return xsm_default_action(action, d, NULL);
 }
 
-static XSM_INLINE int xsm_platform_op(XSM_DEFAULT_ARG uint32_t op)
-{
-    XSM_ASSERT_ACTION(XSM_PRIV);
-    return xsm_default_action(action, current->domain, NULL);
-}
-
 static XSM_INLINE int xsm_machine_memory_map(XSM_DEFAULT_VOID)
 {
     XSM_ASSERT_ACTION(XSM_PRIV);
diff --git a/xen/include/xsm/xsm.h b/xen/include/xsm/xsm.h
index ba3caed..f48cf60 100644
--- a/xen/include/xsm/xsm.h
+++ b/xen/include/xsm/xsm.h
@@ -164,6 +164,8 @@ struct xsm_operations {
     int (*mem_sharing) (struct domain *d);
 #endif
 
+    int (*platform_op) (uint32_t cmd);
+
 #ifdef CONFIG_X86
     int (*do_mca) (void);
     int (*shadow_control) (struct domain *d, uint32_t op);
@@ -175,7 +177,6 @@ struct xsm_operations {
     int (*mem_sharing_op) (struct domain *d, struct domain *cd, int op);
     int (*apic) (struct domain *d, int cmd);
     int (*memtype) (uint32_t access);
-    int (*platform_op) (uint32_t cmd);
     int (*machine_memory_map) (void);
     int (*domain_memory_map) (struct domain *d);
 #define XSM_MMU_UPDATE_READ      1
@@ -624,6 +625,11 @@ static inline int xsm_mem_sharing (xsm_default_t def, struct domain *d)
 }
 #endif
 
+static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
+{
+    return xsm_ops->platform_op(op);
+}
+
 #ifdef CONFIG_X86
 static inline int xsm_do_mca(xsm_default_t def)
 {
@@ -675,11 +681,6 @@ static inline int xsm_memtype (xsm_default_t def, uint32_t access)
     return xsm_ops->memtype(access);
 }
 
-static inline int xsm_platform_op (xsm_default_t def, uint32_t op)
-{
-    return xsm_ops->platform_op(op);
-}
-
 static inline int xsm_machine_memory_map(xsm_default_t def)
 {
     return xsm_ops->machine_memory_map();
-- 
1.7.10.4

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[David Vrabel]

On 05/11/15 16:57, Stefano Stabellini wrote:
> +    case XENPF_settime32:
> +        do_settime(op->u.settime32.secs,
> +                   op->u.settime32.nsecs,
> +                   op->u.settime32.system_time);
> +        break;

I don't think you want to provide this hypercall -- only provide the
XENPF_settime64 one.

David

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini]

On Thu, 5 Nov 2015, David Vrabel wrote:
> On 05/11/15 16:57, Stefano Stabellini wrote:
> > +    case XENPF_settime32:
> > +        do_settime(op->u.settime32.secs,
> > +                   op->u.settime32.nsecs,
> > +                   op->u.settime32.system_time);
> > +        break;
> 
> I don't think you want to provide this hypercall -- only provide the
> XENPF_settime64 one.

OK, make sense

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Julien Grall]

Hi Stefano,

You forgot to CC Daniel for the XSM part. Please use
scripts/get_maintainers.pl to get the relevant maintainers.

On 05/11/15 16:57, Stefano Stabellini wrote:
> Call update_domain_wallclock_time at domain initialization.

It's not really what you are doing in the code. You are calling
update_domain_wallclock_time when the first vCPU is initialized.

Also some rationale to explain why this call should be done here would
be good.

Finally, I'm a bit surprised that you only need to call
update_domain_wallclock_time when the domain is created. x86 needs to
call in various places.

For instance we may want to call update_domain_wallclock_time in
construct_dom0 before clearing the pause flags. This is because the
wallclock may be out of sync as construction DOM0 takes some time.

> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> ---
>  xen/arch/arm/Makefile             |    1 +
>  xen/arch/arm/domain.c             |    3 ++
>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/traps.c              |    1 +
>  xen/include/xsm/dummy.h           |   12 +++----
>  xen/include/xsm/xsm.h             |   13 ++++----

You also have to fix xsm/flask/hooks.c.

>  6 files changed, 80 insertions(+), 12 deletions(-)
>  create mode 100644 xen/arch/arm/platform_hypercall.c

[..]

> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index b2bfc7d..ac9b1b3 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -742,6 +742,9 @@ int arch_set_info_guest(
>      v->arch.ttbr1 = ctxt->ttbr1;
>      v->arch.ttbcr = ctxt->ttbcr;
>  
> +    if ( v->vcpu_id == 0 )
> +        update_domain_wallclock_time(v->domain);
> +
>      v->is_initialised = 1;
>  
>      if ( ctxt->flags & VGCF_online )
> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
> new file mode 100644
> index 0000000..f60d7b3
> --- /dev/null
> +++ b/xen/arch/arm/platform_hypercall.c
> @@ -0,0 +1,62 @@
> +/******************************************************************************
> + * platform_hypercall.c
> + * 
> + * Hardware platform operations. Intended for use by domain-0 kernel.
> + * 
> + * Copyright (c) 2015, Citrix
> + */
> +
> +#include <xen/config.h>
> +#include <xen/types.h>
> +#include <xen/sched.h>
> +#include <xen/guest_access.h>
> +#include <xen/spinlock.h>
> +#include <public/platform.h>
> +#include <xsm/xsm.h>
> +#include <asm/current.h>
> +#include <asm/event.h>
> +
> +DEFINE_SPINLOCK(xenpf_lock);
> +
> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> +{

Would it make sense to introduce a common platform code which take care
of common hypercall? See for instance do_domctl and arch_do_domctl.

> +    long ret;
> +    struct xen_platform_op curop, *op = &curop;
> +
> +    if ( copy_from_guest(op, u_xenpf_op, 1) )
> +        return -EFAULT;
> +
> +    if ( op->interface_version != XENPF_INTERFACE_VERSION )
> +        return -EACCES;
> +
> +    ret = xsm_platform_op(XSM_PRIV, op->cmd);
> +    if ( ret )
> +        return ret;
> +
> +    spin_lock(&xenpf_lock);
> +
> +    switch ( op->cmd )
> +    {
> +    case XENPF_settime32:
> +        do_settime(op->u.settime32.secs,
> +                   op->u.settime32.nsecs,
> +                   op->u.settime32.system_time);
> +        break;

Do we really want to support settime32 on ARM?

> +
> +    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;
> +
> +    default:
> +        ret = -ENOSYS;
> +        break;
> +    }
> +
> +    spin_unlock(&xenpf_lock);
> +    return ret;
> +}

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Andrew Cooper]

On 05/11/15 17:34, Julien Grall wrote:
> Hi Stefano,
>
> You forgot to CC Daniel for the XSM part. Please use
> scripts/get_maintainers.pl to get the relevant maintainers.
>
> On 05/11/15 16:57, Stefano Stabellini wrote:
>> Call update_domain_wallclock_time at domain initialization.
> It's not really what you are doing in the code. You are calling
> update_domain_wallclock_time when the first vCPU is initialized.
>
> Also some rationale to explain why this call should be done here would
> be good.
>
> Finally, I'm a bit surprised that you only need to call
> update_domain_wallclock_time when the domain is created. x86 needs to
> call in various places.
>
> For instance we may want to call update_domain_wallclock_time in
> construct_dom0 before clearing the pause flags. This is because the
> wallclock may be out of sync as construction DOM0 takes some time.

Just as a note for the x86 side of things, I am planning to see whether
it is possible to remove all (real) RTC knowledge from Xen.

Nothing in Xen actually needs to know wallclock time (other than just
propagating what dom0 says to other domains).  It can safely be ignored
until dom0 has made a settime hypercall.

Currently the RTC and CMOS ram is shared between Xen and dom0, in a way
which is basically impossible to actually lock down.  Removing the RTC
as a source of wallclock time allows Xen to give the RTC wholesale to
dom0, which removes quite a lot of complexity.

Furthermore, it removes all use of gettime EFI call, which is known
buggy in just about all currently-available firmware, and will
definitely be an improvement.

~Andrew

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini]

On Thu, 5 Nov 2015, Julien Grall wrote:
> Hi Stefano,
> 
> You forgot to CC Daniel for the XSM part. Please use
> scripts/get_maintainers.pl to get the relevant maintainers.
> 
> On 05/11/15 16:57, Stefano Stabellini wrote:
> > Call update_domain_wallclock_time at domain initialization.
> 
> It's not really what you are doing in the code. You are calling
> update_domain_wallclock_time when the first vCPU is initialized.
> 
> Also some rationale to explain why this call should be done here would
> be good.
> 
> Finally, I'm a bit surprised that you only need to call
> update_domain_wallclock_time when the domain is created. x86 needs to
> call in various places.

It is also called automatically from do_settime. I don't think we need
any other arch-specific call sites.


> For instance we may want to call update_domain_wallclock_time in
> construct_dom0 before clearing the pause flags. This is because the
> wallclock may be out of sync as construction DOM0 takes some time.

That's not necessary: the wallclock in Xen is the number
of seconds since 1970 at the time the physical machine booted, plus the
domain specific offset, so it is not subject to quick incremental
changes, like a monotonic clock.


> > Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > ---
> >  xen/arch/arm/Makefile             |    1 +
> >  xen/arch/arm/domain.c             |    3 ++
> >  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
> >  xen/arch/arm/traps.c              |    1 +
> >  xen/include/xsm/dummy.h           |   12 +++----
> >  xen/include/xsm/xsm.h             |   13 ++++----
> 
> You also have to fix xsm/flask/hooks.c.

Uhm.. OK


> >  6 files changed, 80 insertions(+), 12 deletions(-)
> >  create mode 100644 xen/arch/arm/platform_hypercall.c
> 
> [..]
> 
> > diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> > index b2bfc7d..ac9b1b3 100644
> > --- a/xen/arch/arm/domain.c
> > +++ b/xen/arch/arm/domain.c
> > @@ -742,6 +742,9 @@ int arch_set_info_guest(
> >      v->arch.ttbr1 = ctxt->ttbr1;
> >      v->arch.ttbcr = ctxt->ttbcr;
> >  
> > +    if ( v->vcpu_id == 0 )
> > +        update_domain_wallclock_time(v->domain);
> > +
> >      v->is_initialised = 1;
> >  
> >      if ( ctxt->flags & VGCF_online )
> > diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
> > new file mode 100644
> > index 0000000..f60d7b3
> > --- /dev/null
> > +++ b/xen/arch/arm/platform_hypercall.c
> > @@ -0,0 +1,62 @@
> > +/******************************************************************************
> > + * platform_hypercall.c
> > + * 
> > + * Hardware platform operations. Intended for use by domain-0 kernel.
> > + * 
> > + * Copyright (c) 2015, Citrix
> > + */
> > +
> > +#include <xen/config.h>
> > +#include <xen/types.h>
> > +#include <xen/sched.h>
> > +#include <xen/guest_access.h>
> > +#include <xen/spinlock.h>
> > +#include <public/platform.h>
> > +#include <xsm/xsm.h>
> > +#include <asm/current.h>
> > +#include <asm/event.h>
> > +
> > +DEFINE_SPINLOCK(xenpf_lock);
> > +
> > +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> > +{
> 
> Would it make sense to introduce a common platform code which take care
> of common hypercall? See for instance do_domctl and arch_do_domctl.

In this case I don't think so. I don't see the other existing
platform_ops being used on arm.

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Julien Grall]

Hi Stefano,

On 09/11/15 17:09, Stefano Stabellini wrote:
> On Thu, 5 Nov 2015, Julien Grall wrote:
>> For instance we may want to call update_domain_wallclock_time in
>> construct_dom0 before clearing the pause flags. This is because the
>> wallclock may be out of sync as construction DOM0 takes some time.
> 
> That's not necessary: the wallclock in Xen is the number
> of seconds since 1970 at the time the physical machine booted, plus the
> domain specific offset, so it is not subject to quick incremental
> changes, like a monotonic clock.

Well, building dom0 takes more than one sec, even on big platform.

And if it's not subject to quick incremental, what's the point to call
update_domain_wallclock_time in an odd way in arch_set_info_guest rather
than in arch_domain_create?

>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>> ---
>>>  xen/arch/arm/Makefile             |    1 +
>>>  xen/arch/arm/domain.c             |    3 ++
>>>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
>>>  xen/arch/arm/traps.c              |    1 +
>>>  xen/include/xsm/dummy.h           |   12 +++----
>>>  xen/include/xsm/xsm.h             |   13 ++++----
>>
>> You also have to fix xsm/flask/hooks.c.
> 
> Uhm.. OK
> 
> 
>>>  6 files changed, 80 insertions(+), 12 deletions(-)
>>>  create mode 100644 xen/arch/arm/platform_hypercall.c
>>
>> [..]
>>
>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>> index b2bfc7d..ac9b1b3 100644
>>> --- a/xen/arch/arm/domain.c
>>> +++ b/xen/arch/arm/domain.c
>>> @@ -742,6 +742,9 @@ int arch_set_info_guest(
>>>      v->arch.ttbr1 = ctxt->ttbr1;
>>>      v->arch.ttbcr = ctxt->ttbcr;
>>>  
>>> +    if ( v->vcpu_id == 0 )
>>> +        update_domain_wallclock_time(v->domain);
>>> +
>>>      v->is_initialised = 1;
>>>  
>>>      if ( ctxt->flags & VGCF_online )
>>> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
>>> new file mode 100644
>>> index 0000000..f60d7b3
>>> --- /dev/null
>>> +++ b/xen/arch/arm/platform_hypercall.c
>>> @@ -0,0 +1,62 @@
>>> +/******************************************************************************
>>> + * platform_hypercall.c
>>> + * 
>>> + * Hardware platform operations. Intended for use by domain-0 kernel.
>>> + * 
>>> + * Copyright (c) 2015, Citrix
>>> + */
>>> +
>>> +#include <xen/config.h>
>>> +#include <xen/types.h>
>>> +#include <xen/sched.h>
>>> +#include <xen/guest_access.h>
>>> +#include <xen/spinlock.h>
>>> +#include <public/platform.h>
>>> +#include <xsm/xsm.h>
>>> +#include <asm/current.h>
>>> +#include <asm/event.h>
>>> +
>>> +DEFINE_SPINLOCK(xenpf_lock);
>>> +
>>> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>>> +{
>>
>> Would it make sense to introduce a common platform code which take care
>> of common hypercall? See for instance do_domctl and arch_do_domctl.
> 
> In this case I don't think so. I don't see the other existing
> platform_ops being used on arm.

Are you sure? I can see some of the sub-hypercall implemented for ARM
too such as XENPF_efi_runtime_call, XENPF_change_freq,
XENPF_getidletime, XENPF_cpu_{online,offline}...

I'm not asking for implementing all of them now, but just preparing an
infrastructure for later similar to the domctl version.

Regards,

-- 
Julien Grall

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini]

On Tue, 10 Nov 2015, Julien Grall wrote:
> Hi Stefano,
> 
> On 09/11/15 17:09, Stefano Stabellini wrote:
> > On Thu, 5 Nov 2015, Julien Grall wrote:
> >> For instance we may want to call update_domain_wallclock_time in
> >> construct_dom0 before clearing the pause flags. This is because the
> >> wallclock may be out of sync as construction DOM0 takes some time.
> > 
> > That's not necessary: the wallclock in Xen is the number
> > of seconds since 1970 at the time the physical machine booted, plus the
> > domain specific offset, so it is not subject to quick incremental
> > changes, like a monotonic clock.
> 
> Well, building dom0 takes more than one sec, even on big platform.
> 
> And if it's not subject to quick incremental, what's the point to call
> update_domain_wallclock_time in an odd way in arch_set_info_guest rather
> than in arch_domain_create?

Only to make sure that is called after domain_vtimer_init.
In fact I could move it to arch_domain_create right after it. That might
be better?


> >>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>> ---
> >>>  xen/arch/arm/Makefile             |    1 +
> >>>  xen/arch/arm/domain.c             |    3 ++
> >>>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
> >>>  xen/arch/arm/traps.c              |    1 +
> >>>  xen/include/xsm/dummy.h           |   12 +++----
> >>>  xen/include/xsm/xsm.h             |   13 ++++----
> >>
> >> You also have to fix xsm/flask/hooks.c.
> > 
> > Uhm.. OK
> > 
> > 
> >>>  6 files changed, 80 insertions(+), 12 deletions(-)
> >>>  create mode 100644 xen/arch/arm/platform_hypercall.c
> >>
> >> [..]
> >>
> >>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >>> index b2bfc7d..ac9b1b3 100644
> >>> --- a/xen/arch/arm/domain.c
> >>> +++ b/xen/arch/arm/domain.c
> >>> @@ -742,6 +742,9 @@ int arch_set_info_guest(
> >>>      v->arch.ttbr1 = ctxt->ttbr1;
> >>>      v->arch.ttbcr = ctxt->ttbcr;
> >>>  
> >>> +    if ( v->vcpu_id == 0 )
> >>> +        update_domain_wallclock_time(v->domain);
> >>> +
> >>>      v->is_initialised = 1;
> >>>  
> >>>      if ( ctxt->flags & VGCF_online )
> >>> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
> >>> new file mode 100644
> >>> index 0000000..f60d7b3
> >>> --- /dev/null
> >>> +++ b/xen/arch/arm/platform_hypercall.c
> >>> @@ -0,0 +1,62 @@
> >>> +/******************************************************************************
> >>> + * platform_hypercall.c
> >>> + * 
> >>> + * Hardware platform operations. Intended for use by domain-0 kernel.
> >>> + * 
> >>> + * Copyright (c) 2015, Citrix
> >>> + */
> >>> +
> >>> +#include <xen/config.h>
> >>> +#include <xen/types.h>
> >>> +#include <xen/sched.h>
> >>> +#include <xen/guest_access.h>
> >>> +#include <xen/spinlock.h>
> >>> +#include <public/platform.h>
> >>> +#include <xsm/xsm.h>
> >>> +#include <asm/current.h>
> >>> +#include <asm/event.h>
> >>> +
> >>> +DEFINE_SPINLOCK(xenpf_lock);
> >>> +
> >>> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> >>> +{
> >>
> >> Would it make sense to introduce a common platform code which take care
> >> of common hypercall? See for instance do_domctl and arch_do_domctl.
> > 
> > In this case I don't think so. I don't see the other existing
> > platform_ops being used on arm.
> 
> Are you sure? I can see some of the sub-hypercall implemented for ARM
> too such as XENPF_efi_runtime_call, XENPF_change_freq,
> XENPF_getidletime, XENPF_cpu_{online,offline}...
> 
> I'm not asking for implementing all of them now, but just preparing an
> infrastructure for later similar to the domctl version.

The spin_trylock call is not useful on ARM and many of the other ops are
not either. In addition the implementation of XENPF_settime64 is
slightly different too.

I don't think there is any value in trying to share the do_platform_op
function now. But maybe in the future.

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Julien Grall]

On 10/11/15 14:27, Stefano Stabellini wrote:
> On Tue, 10 Nov 2015, Julien Grall wrote:
>> Hi Stefano,
>>
>> On 09/11/15 17:09, Stefano Stabellini wrote:
>>> On Thu, 5 Nov 2015, Julien Grall wrote:
>>>> For instance we may want to call update_domain_wallclock_time in
>>>> construct_dom0 before clearing the pause flags. This is because the
>>>> wallclock may be out of sync as construction DOM0 takes some time.
>>>
>>> That's not necessary: the wallclock in Xen is the number
>>> of seconds since 1970 at the time the physical machine booted, plus the
>>> domain specific offset, so it is not subject to quick incremental
>>> changes, like a monotonic clock.
>>
>> Well, building dom0 takes more than one sec, even on big platform.
>>
>> And if it's not subject to quick incremental, what's the point to call
>> update_domain_wallclock_time in an odd way in arch_set_info_guest rather
>> than in arch_domain_create?
> 
> Only to make sure that is called after domain_vtimer_init.
> In fact I could move it to arch_domain_create right after it. That might
> be better?

Yes, it would make more sense.

I'm also wondering if we can directly do the call in domain_vtimer_init?


>>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
>>>>> ---
>>>>>  xen/arch/arm/Makefile             |    1 +
>>>>>  xen/arch/arm/domain.c             |    3 ++
>>>>>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
>>>>>  xen/arch/arm/traps.c              |    1 +
>>>>>  xen/include/xsm/dummy.h           |   12 +++----
>>>>>  xen/include/xsm/xsm.h             |   13 ++++----
>>>>
>>>> You also have to fix xsm/flask/hooks.c.
>>>
>>> Uhm.. OK
>>>
>>>
>>>>>  6 files changed, 80 insertions(+), 12 deletions(-)
>>>>>  create mode 100644 xen/arch/arm/platform_hypercall.c
>>>>
>>>> [..]
>>>>
>>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
>>>>> index b2bfc7d..ac9b1b3 100644
>>>>> --- a/xen/arch/arm/domain.c
>>>>> +++ b/xen/arch/arm/domain.c
>>>>> @@ -742,6 +742,9 @@ int arch_set_info_guest(
>>>>>      v->arch.ttbr1 = ctxt->ttbr1;
>>>>>      v->arch.ttbcr = ctxt->ttbcr;
>>>>>  
>>>>> +    if ( v->vcpu_id == 0 )
>>>>> +        update_domain_wallclock_time(v->domain);
>>>>> +
>>>>>      v->is_initialised = 1;
>>>>>  
>>>>>      if ( ctxt->flags & VGCF_online )
>>>>> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
>>>>> new file mode 100644
>>>>> index 0000000..f60d7b3
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/arm/platform_hypercall.c
>>>>> @@ -0,0 +1,62 @@
>>>>> +/******************************************************************************
>>>>> + * platform_hypercall.c
>>>>> + * 
>>>>> + * Hardware platform operations. Intended for use by domain-0 kernel.
>>>>> + * 
>>>>> + * Copyright (c) 2015, Citrix
>>>>> + */
>>>>> +
>>>>> +#include <xen/config.h>
>>>>> +#include <xen/types.h>
>>>>> +#include <xen/sched.h>
>>>>> +#include <xen/guest_access.h>
>>>>> +#include <xen/spinlock.h>
>>>>> +#include <public/platform.h>
>>>>> +#include <xsm/xsm.h>
>>>>> +#include <asm/current.h>
>>>>> +#include <asm/event.h>
>>>>> +
>>>>> +DEFINE_SPINLOCK(xenpf_lock);
>>>>> +
>>>>> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
>>>>> +{
>>>>
>>>> Would it make sense to introduce a common platform code which take care
>>>> of common hypercall? See for instance do_domctl and arch_do_domctl.
>>>
>>> In this case I don't think so. I don't see the other existing
>>> platform_ops being used on arm.
>>
>> Are you sure? I can see some of the sub-hypercall implemented for ARM
>> too such as XENPF_efi_runtime_call, XENPF_change_freq,
>> XENPF_getidletime, XENPF_cpu_{online,offline}...
>>
>> I'm not asking for implementing all of them now, but just preparing an
>> infrastructure for later similar to the domctl version.
> 
> The spin_trylock call is not useful on ARM and many of the other ops are
> not either. In addition the implementation of XENPF_settime64 is
> slightly different too.

That's a call to forget changing the locking when it will be required
and may lead to security issue.

It really doesn't hurt us to do the spin_trylock solution today.

> 
> I don't think there is any value in trying to share the do_platform_op
> function now. But maybe in the future.

Ok.

-- 
Julien Grall

Re: [PATCH 2/2] arm: export platform_op XENPF_settime

[Stefano Stabellini]

On Tue, 10 Nov 2015, Julien Grall wrote:
> On 10/11/15 14:27, Stefano Stabellini wrote:
> > On Tue, 10 Nov 2015, Julien Grall wrote:
> >> Hi Stefano,
> >>
> >> On 09/11/15 17:09, Stefano Stabellini wrote:
> >>> On Thu, 5 Nov 2015, Julien Grall wrote:
> >>>> For instance we may want to call update_domain_wallclock_time in
> >>>> construct_dom0 before clearing the pause flags. This is because the
> >>>> wallclock may be out of sync as construction DOM0 takes some time.
> >>>
> >>> That's not necessary: the wallclock in Xen is the number
> >>> of seconds since 1970 at the time the physical machine booted, plus the
> >>> domain specific offset, so it is not subject to quick incremental
> >>> changes, like a monotonic clock.
> >>
> >> Well, building dom0 takes more than one sec, even on big platform.
> >>
> >> And if it's not subject to quick incremental, what's the point to call
> >> update_domain_wallclock_time in an odd way in arch_set_info_guest rather
> >> than in arch_domain_create?
> > 
> > Only to make sure that is called after domain_vtimer_init.
> > In fact I could move it to arch_domain_create right after it. That might
> > be better?
> 
> Yes, it would make more sense.
> 
> I'm also wondering if we can directly do the call in domain_vtimer_init?

Maybe that's not a good idea given that wallclock and monotonic clocks
are usually separate, but we could certainly move setting
time_offset_seconds there.

 
> >>>>> Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>>>> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> >>>>> ---
> >>>>>  xen/arch/arm/Makefile             |    1 +
> >>>>>  xen/arch/arm/domain.c             |    3 ++
> >>>>>  xen/arch/arm/platform_hypercall.c |   62 +++++++++++++++++++++++++++++++++++++
> >>>>>  xen/arch/arm/traps.c              |    1 +
> >>>>>  xen/include/xsm/dummy.h           |   12 +++----
> >>>>>  xen/include/xsm/xsm.h             |   13 ++++----
> >>>>
> >>>> You also have to fix xsm/flask/hooks.c.
> >>>
> >>> Uhm.. OK
> >>>
> >>>
> >>>>>  6 files changed, 80 insertions(+), 12 deletions(-)
> >>>>>  create mode 100644 xen/arch/arm/platform_hypercall.c
> >>>>
> >>>> [..]
> >>>>
> >>>>> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> >>>>> index b2bfc7d..ac9b1b3 100644
> >>>>> --- a/xen/arch/arm/domain.c
> >>>>> +++ b/xen/arch/arm/domain.c
> >>>>> @@ -742,6 +742,9 @@ int arch_set_info_guest(
> >>>>>      v->arch.ttbr1 = ctxt->ttbr1;
> >>>>>      v->arch.ttbcr = ctxt->ttbcr;
> >>>>>  
> >>>>> +    if ( v->vcpu_id == 0 )
> >>>>> +        update_domain_wallclock_time(v->domain);
> >>>>> +
> >>>>>      v->is_initialised = 1;
> >>>>>  
> >>>>>      if ( ctxt->flags & VGCF_online )
> >>>>> diff --git a/xen/arch/arm/platform_hypercall.c b/xen/arch/arm/platform_hypercall.c
> >>>>> new file mode 100644
> >>>>> index 0000000..f60d7b3
> >>>>> --- /dev/null
> >>>>> +++ b/xen/arch/arm/platform_hypercall.c
> >>>>> @@ -0,0 +1,62 @@
> >>>>> +/******************************************************************************
> >>>>> + * platform_hypercall.c
> >>>>> + * 
> >>>>> + * Hardware platform operations. Intended for use by domain-0 kernel.
> >>>>> + * 
> >>>>> + * Copyright (c) 2015, Citrix
> >>>>> + */
> >>>>> +
> >>>>> +#include <xen/config.h>
> >>>>> +#include <xen/types.h>
> >>>>> +#include <xen/sched.h>
> >>>>> +#include <xen/guest_access.h>
> >>>>> +#include <xen/spinlock.h>
> >>>>> +#include <public/platform.h>
> >>>>> +#include <xsm/xsm.h>
> >>>>> +#include <asm/current.h>
> >>>>> +#include <asm/event.h>
> >>>>> +
> >>>>> +DEFINE_SPINLOCK(xenpf_lock);
> >>>>> +
> >>>>> +long do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
> >>>>> +{
> >>>>
> >>>> Would it make sense to introduce a common platform code which take care
> >>>> of common hypercall? See for instance do_domctl and arch_do_domctl.
> >>>
> >>> In this case I don't think so. I don't see the other existing
> >>> platform_ops being used on arm.
> >>
> >> Are you sure? I can see some of the sub-hypercall implemented for ARM
> >> too such as XENPF_efi_runtime_call, XENPF_change_freq,
> >> XENPF_getidletime, XENPF_cpu_{online,offline}...
> >>
> >> I'm not asking for implementing all of them now, but just preparing an
> >> infrastructure for later similar to the domctl version.
> > 
> > The spin_trylock call is not useful on ARM and many of the other ops are
> > not either. In addition the implementation of XENPF_settime64 is
> > slightly different too.
> 
> That's a call to forget changing the locking when it will be required
> and may lead to security issue.
> 
> It really doesn't hurt us to do the spin_trylock solution today.

All right


> > I don't think there is any value in trying to share the do_platform_op
> > function now. But maybe in the future.
> 
> Ok.
> 
> -- 
> Julien Grall
>