[PATCH 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask

* [PATCH 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask
@ 2014-08-01 15:21 Paul Durrant
  2014-08-01 15:21 ` [PATCH 2/2] x86/viridian: Add partition time reference counter MSR support Paul Durrant
  2014-08-01 15:41 ` [PATCH 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask Jan Beulich
  0 siblings, 2 replies; 6+ messages in thread
From: Paul Durrant @ 2014-08-01 15:21 UTC (permalink / raw)
  To: xen-devel
  Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, Ian Jackson,
	Paul Durrant, Jan Beulich

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@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 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          |   16 ++++++++++--
 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, 186 insertions(+), 35 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index ff9ea77..d770114 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.
+
+=item B<freq>
+
+This set incorporates the TSC and APIC frequency MSRs.
+
+=back
+ 
+See the latest version of Microsoft's Hypervisor Top-Level Functional
+Specification for more details.
+
+This base set of enlightenments can improve performance of Windows Vista
+and Windows Server 2008 onwards and setting this option for such guests is
+strongly recommended.
+
+The TSC and APIC frequency MSRs can improve performance of Microsoft
+Windows 7 and Server 2008 R2 onwards.
+
+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)",
+                         feature_mask);
+}
+#endif
+
+static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
                                 libxl_domain_build_info *const 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..59ef08f 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5533,8 +5533,20 @@ 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] )
+                    break;
+                
+                rc = -EINVAL;
+                if ( a.value & ~HVMPV_feature_mask )
+                    break;
+
+                rc = 0;
                 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..891b21f 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..e0fec6d 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)
+
 #endif
 
 /*
-- 
1.7.10.4

^ permalink raw reply related	[flat|nested] 6+ messages in thread