Re: [Xen-devel] [PATCH 2/2] x86/hvm: honor guest's option when updating secondary system time for guest

["Wu, Feng" <feng.wu@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@xxxxxxxxxx]
> Sent: Tuesday, July 08, 2014 6:08 PM
> To: Wu, Feng; xen-devel@xxxxxxxxxxxxx
> Cc: tim@xxxxxxx; linux@xxxxxxxxxxxxxx; jbeulich@xxxxxxxx; keir@xxxxxxx
> Subject: Re: [Xen-devel] [PATCH 2/2] x86/hvm: honor guest's option when
> updating secondary system time for guest
> 
> On 08/07/14 00:18, Feng Wu wrote:
> > In this patch, we add a new hypervisor which allows guests to enable
> > SMAP check when Xen is updating the secondary system time for guest.
> > The SMAP check for this update is disabled by default.
> >
> > Reported-by: Sander Eikelenboom <linux@xxxxxxxxxxxxxx>
> > Signed-off-by: Feng Wu <feng.wu@xxxxxxxxx>
> 
> This patch uses the same v->arch.smap_check_policy variable as the
> previous patch, meaning its value is unconditionally
> SMAP_CHECK_HONOR_CPL_AC other than the short window while the
> runstate
> area is being updated.

Guests can enable it by hypercall ' 
VCPUOP_enable_smap_check_vcpu_time_memory_area ',
If the guest calls this hypercall, v->arch.enable_smap_check will be set to 1, 
so when updating
the secondary system time for guest, v->arch.smap_check_policy will be set to 
SMAP_CHECK_ENABLED.

Thanks,
Feng

> 
> As a result, there appears to be no way for this to cause working
> updates to a system time living in guest userspace.
> 
> ~Andrew
> 
> > ---
> >  xen/arch/x86/domain.c        |  6 ++++++
> >  xen/arch/x86/hvm/hvm.c       |  2 ++
> >  xen/arch/x86/time.c          | 12 +++++++++++-
> >  xen/include/asm-x86/domain.h |  9 ++++++++-
> >  xen/include/public/vcpu.h    | 10 ++++++++++
> >  5 files changed, 37 insertions(+), 2 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index b0c8810..a787c46 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -1053,6 +1053,12 @@ arch_do_vcpu_op(
> >          break;
> >      }
> >
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> > +    {
> > +        v->arch.enable_smap_check = 1;
> > +        break;
> > +    }
> > +
> >      case VCPUOP_get_physid:
> >      {
> >          struct vcpu_get_physid cpu_id;
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index ef2411c..a922711 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4629,6 +4629,7 @@ static long hvm_vcpu_op(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = do_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > @@ -4688,6 +4689,7 @@ static long hvm_vcpu_op_compat32(
> >      case VCPUOP_stop_singleshot_timer:
> >      case VCPUOP_register_vcpu_info:
> >      case VCPUOP_register_vcpu_time_memory_area:
> > +    case VCPUOP_enable_smap_check_vcpu_time_memory_area:
> >          rc = compat_vcpu_op(cmd, vcpuid, arg);
> >          break;
> >      default:
> > diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
> > index a4e1656..338381e 100644
> > --- a/xen/arch/x86/time.c
> > +++ b/xen/arch/x86/time.c
> > @@ -821,7 +821,7 @@ static void __update_vcpu_system_time(struct vcpu
> *v, int force)
> >          v->arch.pv_vcpu.pending_system_time = _u;
> >  }
> >
> > -bool_t update_secondary_system_time(const struct vcpu *v,
> > +bool_t update_secondary_system_time(struct vcpu *v,
> >                                      struct vcpu_time_info *u)
> >  {
> >      XEN_GUEST_HANDLE(vcpu_time_info_t) user_u =
> v->arch.time_info_guest;
> > @@ -829,9 +829,16 @@ bool_t update_secondary_system_time(const
> struct vcpu *v,
> >      if ( guest_handle_is_null(user_u) )
> >          return 1;
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_ENABLED;
> > +
> >      /* 1. Update userspace version. */
> >      if ( __copy_field_to_guest(user_u, u, version) == sizeof(u->version) )
> > +    {
> > +        if ( v->arch.enable_smap_check )
> > +            v->arch.smap_check_policy =
> SMAP_CHECK_HONOR_CPL_AC;
> >          return 0;
> > +    }
> >      wmb();
> >      /* 2. Update all other userspace fields. */
> >      __copy_to_guest(user_u, u, 1);
> > @@ -840,6 +847,9 @@ bool_t update_secondary_system_time(const struct
> vcpu *v,
> >      u->version = version_update_end(u->version);
> >      __copy_field_to_guest(user_u, u, version);
> >
> > +    if ( v->arch.enable_smap_check )
> > +        v->arch.smap_check_policy = SMAP_CHECK_HONOR_CPL_AC;
> > +
> >      return 1;
> >  }
> >
> > diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
> > index d7cac4f..051fed0 100644
> > --- a/xen/include/asm-x86/domain.h
> > +++ b/xen/include/asm-x86/domain.h
> > @@ -455,6 +455,13 @@ struct arch_vcpu
> >       *     SMAP_CHECK_DISABLED     - disable the check
> >       */
> >      uint8_t smap_check_policy;
> > +
> > +    /*
> > +     * This is a flag to indicate that whether the guest wants to enable
> > +     * SMAP check when Xen is updating the secondary system time from
> it.
> > +     */
> > +    bool_t enable_smap_check;
> > +    uint16_t pad;
> >  } __cacheline_aligned;
> >
> >  #define SMAP_CHECK_HONOR_CPL_AC  0
> > @@ -466,7 +473,7 @@ struct arch_vcpu
> >  #define hvm_svm         hvm_vcpu.u.svm
> >
> >  bool_t update_runstate_area(struct vcpu *);
> > -bool_t update_secondary_system_time(const struct vcpu *,
> > +bool_t update_secondary_system_time(struct vcpu *,
> >                                      struct vcpu_time_info *);
> >
> >  void vcpu_show_execution_state(struct vcpu *);
> > diff --git a/xen/include/public/vcpu.h b/xen/include/public/vcpu.h
> > index e888daf..d348395 100644
> > --- a/xen/include/public/vcpu.h
> > +++ b/xen/include/public/vcpu.h
> > @@ -227,6 +227,16 @@ struct vcpu_register_time_memory_area {
> >  typedef struct vcpu_register_time_memory_area
> vcpu_register_time_memory_area_t;
> >  DEFINE_XEN_GUEST_HANDLE(vcpu_register_time_memory_area_t);
> >
> > +/*
> > + * Flags to tell Xen whether we need to do the SMAP check when updating
> > + * the secondary copy of the vcpu time when SMAP is enabled. Since the
> > + * memory location for the secondary copy of the vcpu time may be mapped
> > + * into userspace by guests intendedly, we let the guest to determine
> > + * whether the check is needed. The default behavior of hypevisor is
> > + * not doing the check.
> > + */
> > +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> > +
> >  #endif /* __XEN_PUBLIC_VCPU_H__ */
> >
> >  /*


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel