Re: [Xen-devel] [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 04/08/14 14:12, Paul Durrant wrote:
> The following commits introduced the time reference counter MSR and
> TSC/APIC frequency MSRs into the viridian feature set respectively:
>
> e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b
> 84657efd9116f40924aa13c9d5a349e007da716f
>
> The time reference counter MSR feature was then reverted by commit
>
> 1cd4fab14ce25859efa4a2af13475e6650a5506c
>
> because a flaw in the implementation meant the counter was reset on
> migration.
>
> All of these changes were made without any addtional options being
> added to the VM configuration, or any compatibility checks being made
> in the domain save/restore code. Hence setting the single boolean
> 'viridian' option in the VM configuration yields a different set of
> features depending on which version of Xen the VM is started on, and the
> feature set can change across migration (so new MSRs can magically appear).
>
> This patch grandfathers in the current viridian features set and calls them
> the 'base' plus 'freq' feature set. HVM_PARAM_VIRIDIAN is re-purposed as
> a feature mask. It has only ever been legally set to 0 or 1, so the presence
> of the base set and freq set are indicated by setting bit 0. The 'freq' set
> can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4
> 'base' set. Newly implemented viridian features can be optionally enabled
> in future by setting further bits.
>
> The viridian option in xl.cfg(5) has also been changed to a string list so
> that the sets can be individually sepcified. For compatibility, if the
> option is specified as a boolean, then a true (1) value will be translated
> to a string list containing "base" and "freq".
>
> This patch also alters the allowed write accesses to HVM_PARAM_VIRIDIAN.
> Currently there is nothing to stop the guest writing this value (which,
> while harmless to anything else, should not happen) and nothing to
> stop a toolstack from setting the value back to zero whilst the guest is
> running, causing CPUID leaves to disappear and MSR accesses to start
> causing GPFs in the guest. Both of these possibilities are now disallowed:
> Once the parameter is set to a non-zero value it may not be cleared, and
> guests no longer have any write access.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Ian Campbell <ian.campbell@xxxxxxxxxx>
> Cc: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
> Cc: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> ---
>  docs/man/xl.cfg.pod.5           |   53 
> ++++++++++++++++++++++++++++++---------
>  tools/libxc/xc_domain_restore.c |    4 +--
>  tools/libxl/libxl.h             |    6 +++++
>  tools/libxl/libxl_create.c      |    1 -
>  tools/libxl/libxl_dom.c         |   46 ++++++++++++++++++++++++++-------
>  tools/libxl/libxl_types.idl     |    2 +-
>  tools/libxl/xl_cmdimpl.c        |   24 +++++++++++++++++-
>  tools/libxl/xl_sxp.c            |    8 ++++--
>  xen/arch/x86/hvm/hvm.c          |   18 +++++++++++--
>  xen/arch/x86/hvm/viridian.c     |   21 ++++++++++++++--
>  xen/include/asm-x86/hvm/hvm.h   |    7 ++++--
>  xen/include/public/hvm/params.h |   33 +++++++++++++++++++++++-
>  12 files changed, 188 insertions(+), 35 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index ff9ea77..a05e831 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1084,18 +1084,47 @@ Windows L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
>  Setting B<xen_platform_pci=0> with the default device_model "qemu-xen"
>  requires at least QEMU 1.6.
>  
> -=item B<viridian=BOOLEAN>
> -
> -Turns on or off the exposure of MicroSoft Hyper-V (AKA viridian)
> -compatible enlightenments to the guest.  These can improve performance
> -of Microsoft Windows guests from Windows Vista and Windows 2008
> -onwards and setting this option for such guests is strongly
> -recommended. This option should be harmless for other versions of
> -Windows (although it will not give any benefit) and the majority of
> -other non-Windows OSes. However it is known to be incompatible with
> -some other Operating Systems and in some circumstance can prevent
> -Xen's own paravirtualisation interfaces for HVM guests from being
> -used.
> +=item B<viridian=[ "SET", "SET", ...]>
> +
> +The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> +exposed to the guest. The following sets of enlightenments may be
> +specified:
> +
> +=over 4
> +
> +=item B<base>
> +
> +This set incorporates the Hypercall MSRs, Virtual processor index MSR,
> +and APIC access MSRs. This set is a pre-requisite for all other sets.
> +If it is not specified then all enlightenments will be disabled.
> +
> +These enlightenments can improve performance of Windows Vista and Windows
> +Server 2008 onwards and setting this option for such guests is strongly
> +recommended.
> +
> +=item B<freq>
> +
> +This set incorporates the TSC and APIC frequency MSRs.
> +
> +This enlightenment can improve performance of Windows 7 and Windows
> +Server 2008 R2 onwards.
> +
> +=back
> + 
> +See the latest version of Microsoft's Hypervisor Top-Level Functional
> +Specification for more details.
> +
> +The enlightenments should be harmless for other versions of Windows
> +(although they will not give any benefit) and the majority of other
> +non-Windows OSes.
> +However it is known that they are incompatible with some other Operating
> +Systems and in some circumstance can prevent Xen's own paravirtualisation
> +interfaces for HVM guests from being used.
> +
> +Specifying the viridian option as a boolean is deprecated. However, for
> +backwards compatibility, if it is specified as a boolean a value of
> +true (1) will cause both the 'base' and 'freq' sets to be exposed to
> +the guest, and a value of false (0) will disable all enlightenments.
>  
>  =back
>  
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index e73e0a2..5e7e62d 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -922,7 +922,7 @@ static int pagebuf_get_one(xc_interface *xch, struct 
> restore_ctx *ctx,
>          if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
>               RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
>          {
> -            PERROR("error read the viridian flag");
> +            PERROR("error read the viridian flags");
>              return -1;
>          }
>          return pagebuf_get_one(xch, ctx, buf, fd, dom);
> @@ -1733,7 +1733,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, 
> uint32_t dom,
>      }
>  
>      if (pagebuf.viridian != 0)
> -        xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> +        xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, pagebuf.viridian);
>  
>      /*
>       * If we are migrating in from a host that does not support
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5ae6532..b985f49 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -128,6 +128,12 @@
>  #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
>  
>  /*
> + * The libxl_domain_build_info u.hvm.viridian field is a string list
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_STRING_LIST 1
> +
> +
> +/*
>   * libxl ABI compatibility
>   *
>   * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 0686f96..ee19fc8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -273,7 +273,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
>          libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3,            true);
>          libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4,            true);
>          libxl_defbool_setdefault(&b_info->u.hvm.nx,                 true);
> -        libxl_defbool_setdefault(&b_info->u.hvm.viridian,           false);
>          libxl_defbool_setdefault(&b_info->u.hvm.hpet,               true);
>          libxl_defbool_setdefault(&b_info->u.hvm.vpt_align,          true);
>          libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm,         false);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 83eb29a..a15c185 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -209,21 +209,49 @@ static unsigned long timer_mode(const 
> libxl_domain_build_info *info)
>      return ((unsigned long)mode);
>  }
>  
> -static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
> +#if defined(__i386__) || defined(__x86_64__)
> +static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
> +                                      libxl_domain_build_info *const info)
> +{
> +    uint64_t feature_mask = HVMPV_no_freq;
> +
> +    char **p = info->u.hvm.viridian;
> +    while (*p) {       
> +        LOG(DETAIL, "+%s", *p);
> +        if (strcmp(*p, "base") == 0)
> +            feature_mask |= HVMPV_base_freq;
> +        else if (strcmp(*p, "freq") == 0)
> +            feature_mask &= ~HVMPV_no_freq;
> +        p++;
> +    }
> +
> +    if (xc_hvm_param_set(CTX->xch,
> +                         domid,
> +                         HVM_PARAM_VIRIDIAN,
> +                         feature_mask) != 0)
> +        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_WARNING,
> +                         "Couldn't set viridian feature mask (0x%lx)",

PRIx64, or this will break 32bit builds.

> +                         feature_mask);
> +}
> +#endif
> +
> +static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
>                                  libxl_domain_build_info *const info)

const libxl_domain_build_info *info ?

>  {
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
> +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED,
>                      libxl_defbool_val(info->u.hvm.pae));
>  #if defined(__i386__) || defined(__x86_64__)
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN,
> -                    libxl_defbool_val(info->u.hvm.viridian));
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
> +    if (libxl_string_list_length(&info->u.hvm.viridian) != 0)
> +        hvm_set_viridian_features(gc, domid, info);
> +
> +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_HPET_ENABLED,
>                      libxl_defbool_val(info->u.hvm.hpet));
>  #endif
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE, timer_mode(info));
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
> +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_TIMER_MODE,
> +                     timer_mode(info));
> +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_VPT_ALIGN,
>                      libxl_defbool_val(info->u.hvm.vpt_align));
> -    xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
> +    xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_NESTEDHVM,
>                      libxl_defbool_val(info->u.hvm.nested_hvm));
>  }
>  
> @@ -306,7 +334,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
>      state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, 
> state->console_domid);
>  
>      if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> -        hvm_set_conf_params(ctx->xch, domid, info);
> +        hvm_set_conf_params(gc, domid, info);
>  
>      rc = libxl__arch_domain_create(gc, d_config, domid);
>  
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a412f9c..6c5d7e0 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -363,7 +363,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
>                                         ("acpi_s3",          libxl_defbool),
>                                         ("acpi_s4",          libxl_defbool),
>                                         ("nx",               libxl_defbool),
> -                                       ("viridian",         libxl_defbool),
> +                                       ("viridian",         
> libxl_string_list),
>                                         ("timeoffset",       string),
>                                         ("hpet",             libxl_defbool),
>                                         ("vpt_align",        libxl_defbool),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 01bce2f..960f626 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -951,10 +951,32 @@ static void parse_config_data(const char *config_source,
>          xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
>          xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
>          xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
> -        xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 0);
>          xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
>          xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 
> 0);
>  
> +        switch (xlu_cfg_get_list_as_string_list(config, "viridian",
> +                                                &b_info->u.hvm.viridian, 1))
> +        {
> +        case 0: break; /* Success */
> +        case ESRCH: break; /* Option not present */
> +        case EINVAL: {
> +            libxl_defbool b;
> +
> +            xlu_cfg_get_defbool(config, "viridian", &b, 1);
> +            if (libxl_defbool_val(b)) {
> +                fprintf(stderr, "WARNING: Specifying \"viridian\""
> +                        " as a boolean is deprecated. "
> +                        "Please use a list of features.\n");
> +                split_string_into_string_list("base freq", " \n",
> +                                              &b_info->u.hvm.viridian);
> +            }
> +            break;
> +        }
> +        default:
> +            fprintf(stderr,"xl: Unable to parse bootloader_args.\n");
> +            exit(-ERROR_FAIL);
> +        }
> +
>          if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
>              const char *s = libxl_timer_mode_to_string(l);
>              fprintf(stderr, "WARNING: specifying \"timer_mode\" as an 
> integer is deprecated. "
> diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
> index 48eb608..347526f 100644
> --- a/tools/libxl/xl_sxp.c
> +++ b/tools/libxl/xl_sxp.c
> @@ -97,8 +97,12 @@ void printf_info_sexp(int domid, libxl_domain_config 
> *d_config)
>          printf("\t\t\t(acpi %s)\n",
>                 libxl_defbool_to_string(b_info->u.hvm.acpi));
>          printf("\t\t\t(nx %s)\n", libxl_defbool_to_string(b_info->u.hvm.nx));
> -        printf("\t\t\t(viridian %s)\n",
> -               libxl_defbool_to_string(b_info->u.hvm.viridian));
> +        if (b_info->u.hvm.viridian) {
> +            printf("\t\t\t(viridian");
> +            for (i=0; b_info->u.hvm.viridian[i]; i++)
> +                printf(" %s", b_info->u.hvm.viridian[i]);
> +            printf(")\n");
> +        }
>          printf("\t\t\t(hpet %s)\n",
>                 libxl_defbool_to_string(b_info->u.hvm.hpet));
>          printf("\t\t\t(vpt_align %s)\n",
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index fba13e0..04d43d0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, 
> XEN_GUEST_HANDLE_PARAM(void) arg)
>                      rc = -EINVAL;
>                  break;
>              case HVM_PARAM_VIRIDIAN:
> -                if ( a.value > 1 )
> -                    rc = -EINVAL;
> +                /* This should only ever be set once by the tools and read 
> by the guest. */
> +                rc = -EPERM;
> +                if ( curr_d == d )
> +                    break;
> +
> +                rc = -EPERM;
> +                if ( d->arch.hvm_domain.params[a.index] &&
> +                     a.value != d->arch.hvm_domain.params[a.index] )
> +                    break;

Setting it twice should be an error, even if it is set to the same value
again.  Also, EEXIST would be a better error.

> +                
> +                rc = -EINVAL;
> +                if ( a.value & ~HVMPV_feature_mask ||
> +                     !(a.value & HVMPV_base_freq) )
> +                    break;
> +
> +                rc = 0;

To save bloating the code here if/when new combinations are introduced,
would it be better to have a "rc = viridian_initialise(d, a.value);"
which validates the featureset and sets it if necessary.  The above
EEXISTs check could be included as well.

>                  break;
>              case HVM_PARAM_IDENT_PT:
>                  /* Not reflexive, as we must domain_pause(). */
> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> index 0fcbfd8..31c9656 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int 
> *eax,
>          /* Which hypervisor MSRs are available to the guest */
>          *eax = (CPUID3A_MSR_APIC_ACCESS |
>                  CPUID3A_MSR_HYPERCALL   |
> -                CPUID3A_MSR_VP_INDEX    |
> -                CPUID3A_MSR_FREQ);
> +                CPUID3A_MSR_VP_INDEX);
> +        if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> +            *eax |= CPUID3A_MSR_FREQ;
>          break;
>      case 4:
>          /* Recommended hypercall usage. */
> @@ -312,11 +313,17 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
>          break;
>  
>      case VIRIDIAN_MSR_TSC_FREQUENCY:
> +        if ( viridian_feature_mask(d) & HVMPV_no_freq )
> +            return 0;
> +
>          perfc_incr(mshv_rdmsr_tsc_frequency);
>          *val = (uint64_t)d->arch.tsc_khz * 1000ull;
>          break;
>  
>      case VIRIDIAN_MSR_APIC_FREQUENCY:
> +        if ( viridian_feature_mask(d) & HVMPV_no_freq )
> +            return 0;
> +
>          perfc_incr(mshv_rdmsr_apic_frequency);
>          *val = 1000000000ull / APIC_BUS_CYCLE_NS;
>          break;
> @@ -489,3 +496,13 @@ static int viridian_load_vcpu_ctxt(struct domain *d, 
> hvm_domain_context_t *h)
>  
>  HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
>                            viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 0ebd478..24e513b 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -344,8 +344,11 @@ static inline unsigned long 
> hvm_get_shadow_gs_base(struct vcpu *v)
>      return hvm_funcs.get_shadow_gs_base(v);
>  }
>  
> -#define is_viridian_domain(_d)                                             \
> - (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
> +#define viridian_feature_mask(d) \
> +    ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> +
> +#define is_viridian_domain(d) \
> +    (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq))
>  
>  void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
>                                 uint32_t *eax, uint32_t *ebx,
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 614ff5f..68d26fd 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -56,9 +56,40 @@
>  
>  #if defined(__i386__) || defined(__x86_64__)
>  
> -/* Expose Viridian interfaces to this HVM guest? */
> +/*
> + * Viridian enlightenments
> + *
> + * (See 
> http://download.microsoft.com/download/A/B/4/AB43A34E-BDD0-4FA6-BDEF-79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.docx)
> + *
> + * To expose viridian enlightenments to the guest set this parameter
> + * to the desired feature mask. The base feature set must be present
> + * in any valid feature mask.
> + */
>  #define HVM_PARAM_VIRIDIAN     9
>  
> +/* Base+Freq viridian feature sets:
> + *
> + * - Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and HV_X64_MSR_HYPERCALL)
> + * - APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR)
> + * - Virtual Processor index MSR (HV_X64_MSR_VP_INDEX)
> + * - Timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> + *   HV_X64_MSR_APIC_FREQUENCY)
> + */
> +#define _HVMPV_base_freq 0
> +#define HVMPV_base_freq  (1 << _HVMPV_base_freq)
> +
> +/* Feature set modifications */
> +
> +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> + * HV_X64_MSR_APIC_FREQUENCY).
> + * This modification restores the viridian feature set to the
> + * original 'base' set exposed in releases prior to Xen 4.4.
> + */
> +#define _HVMPV_no_freq 1
> +#define HVMPV_no_freq  (1 << _HVMPV_no_freq)
> +
> +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)

Please could we avoid introducing new negated-sense booleans, especially
when their predominant use is "if ( !$FOO_no_BAR )"

~Andrew

> +
>  #endif
>  
>  /*


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