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

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Tue, Jul 08, 2014 at 07:18:18AM +0800, 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@eikelenboom.it>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>  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

What is 'intendedly'?

> + * whether the check is needed. The default behavior of hypevisor is
> + * not doing the check.

Under what conditions do we want the guest to do the check? What is
the impact (performance) if we do the check?

Why don't we want by default do the check?

> + */
> +#define VCPUOP_enable_smap_check_vcpu_time_memory_area   14
> +
>  #endif /* __XEN_PUBLIC_VCPU_H__ */
>  
>  /*
> -- 
> 1.8.3.1
>