[PATCH v11 0/2] x86/viridian improvements

[PATCH v11 0/2] x86/viridian improvements

[Paul Durrant]

This patch series incorporates several improvements to the code
supporting viridian (i.e. hyper-v compatible) enlightenments for
Windows guests:

Patch #1 series lays the foundations for adding new viridian
enlightenments such that they can be optionally enabled, and not
immediately exposed to a guest across a save/restore boundary.

Patch #2 adds support for the 'Partition Time Reference Counter'
enlightenment.

v2:
- Addressed comments from Jan Beulich
- Added patch #2

v3:
- Addressed comments from Andrew Cooper and Jan Beulich
- Re-worked patch #2
- Simplified patch #3 to use guest TSC

v4:
- Added missing domain info to printks in patch #2

v5:
- Clarified comment of patch #1 as suggested by David Vrabel
- More logging tweaks in patch #2 as suggested by Andrew Cooper

v6:
- Dropped previous patch to reduce logging verbosity as it has been
  applied
- Toolstack changes only:
  - New libxl_integer_list and libxl_viridian_enlightenment types to
    avoid passing of strings between xl and libxl, as requested by
    Ian Jackson.
  - Retained and deprecated viridian defbool rather than replacing it
    to avoid API breakage pointed out by Ian Campbell. Enlightenment list
    is now in a new hvm-only viridian_enlightenments field of the build
    info.

v7:
- Changes to patch #2 only:
  - Reference time calculation now makes use of struct time_scale and
    set_time_scale and scale_delta functions to avoid overflow issues
    pointed out by Christoph Egger.

v8:
- Changes to patch #2 only:
  - Fixed 32- to 64-bit type promotion issue as pointed out by
    Jan Beulich

v9:
- Changes to ocaml stubs in patch #1 only
  - There was a missing calloc in the libxl_integer_list ocaml stubs

v10:
- Toolstack changes only:
  - Re-worked toolstack code after discussion with Ian C and Ian J.
    See http://lists.xen.org/archives/html/xen-devel/2014-09/msg00416.html

v11:
- Toolstack changes only:
  - Re-worked toolstack code after further discussion with Ian J.

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

[Paul Durrant]

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' and 'freq' feature sets. HVM_PARAM_VIRIDIAN is re-purposed as
a feature mask. The hypervisor has only ever allowed it ot be set to 0
or 1, so the presence of the base and freq sets 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 list so
that the sets can be individually enabled or disabled. For compatibility,
if the option is specified as a boolean, then a true (1) value will enable
the base and freq sets and a false (0) value will not enable any
enlightenments.

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 modified (only
re-written with the same value), and guests no longer have any write
access.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Acked-by: 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>
Cc: David Scott <dave.scott@eu.citrix.com>
---
 docs/man/xl.cfg.pod.5           |   68 ++++++++++++++++++++++++++++------
 tools/libxc/xc_domain_restore.c |    4 +-
 tools/libxl/gentypes.py         |    6 +++
 tools/libxl/idl.py              |    6 +++
 tools/libxl/libxl.h             |    5 +++
 tools/libxl/libxl_dom.c         |   78 +++++++++++++++++++++++++++++++++++++--
 tools/libxl/libxl_types.idl     |    8 ++++
 tools/libxl/xl_cmdimpl.c        |   55 +++++++++++++++++++++++++--
 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, 283 insertions(+), 26 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 1e04eed..274de36 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1106,18 +1106,62 @@ 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=[ "GROUP", "GROUP", ...]>
+
+The groups of Microsoft Hyper-V (AKA viridian) compatible enlightenments
+exposed to the guest. The following groups of enlightenments may be
+specified:
+
+=over 4
+
+=item B<base>
+
+This group incorporates the Hypercall MSRs, Virtual processor index MSR,
+and APIC access MSRs. These enlightenments can improve performance of
+Windows Vista and Windows Server 2008 onwards and setting this option
+for such guests is strongly recommended.
+This group is also a pre-requisite for all others. If it is disabled
+then it is an error to attempt to enable any other group.
+
+=item B<freq>
+
+This group incorporates the TSC and APIC frequency MSRs. These
+enlightenments can improve performance of Windows 7 and Windows
+Server 2008 R2 onwards.
+
+=item B<defaults>
+
+This is a special value that enables the default set of groups, which
+is currently the B<base> and B<freq> groups.
+
+=item B<all>
+
+This is a special value that enables all available groups.
+
+=back
+
+Groups can be disabled by prefixing the name with '!'. So, for example,
+to enable all groups except B<freq>, specify:
+
+=over 4
+
+B<viridian=[ "all", "!freq" ]>
+
+=back
+
+For details of the enlightenments see the latest version of Microsoft's
+Hypervisor Top-Level Functional Specification.
+
+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.
+
+The viridian option can be specified as a boolean. A value of true (1)
+is equivalent to the list [ "defaults" ], and a value of false (0) is
+equivalent to an empty list.
 
 =back
 
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index b9a56d5..d362e3a 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 reading the viridian enlightenments");
             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/gentypes.py b/tools/libxl/gentypes.py
index 3e73821..e101de4 100644
--- a/tools/libxl/gentypes.py
+++ b/tools/libxl/gentypes.py
@@ -564,6 +564,8 @@ if __name__ == '__main__':
             f.write("%sconst char *%s_to_string(%s);\n" % (ty.hidden(), ty.typename, ty.make_arg("p")))
             f.write("%sint %s_from_string(const char *s, %s);\n" % (ty.hidden(), ty.typename, ty.make_arg("e", passby=idl.PASS_BY_REFERENCE)))
             f.write("%sextern libxl_enum_string_table %s_string_table[];\n" % (ty.hidden(), ty.typename))
+            f.write("%sextern %s %s_min_val;\n" % (ty.hidden(), ty.typename, ty.typename))
+            f.write("%sextern %s %s_max_val;\n" % (ty.hidden(), ty.typename, ty.typename))
         f.write("\n")
 
     f.write("""#endif /* %s */\n""" % (header_define))
@@ -678,6 +680,10 @@ if __name__ == '__main__':
         f.write("}\n")
         f.write("\n")
 
+        f.write("%s %s_min_val = %d;\n" % (ty.typename, ty.typename, ty.min))
+        f.write("%s %s_max_val = %d;\n" % (ty.typename, ty.typename, ty.max))
+        f.write("\n")
+
     for ty in [t for t in types if t.json_gen_fn is not None]:
         f.write("yajl_gen_status %s_gen_json(yajl_gen hand, %s)\n" % (ty.typename, ty.make_arg("p", passby=idl.PASS_BY_REFERENCE)))
         f.write("{\n")
diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
index 437049e..983253d 100644
--- a/tools/libxl/idl.py
+++ b/tools/libxl/idl.py
@@ -174,9 +174,15 @@ class Enumeration(Type):
             self.namespace)
 
         self.values = []
+        self.min = sys.maxint
+        self.max = -sys.maxint - 1
         for v in values:
             # (value, name)
             (num,name) = v
+            if num < self.min:
+                self.min = num
+            if num > self.max:
+                self.max = num
             self.values.append(EnumerationValue(self, num, name,
                                                 typename=self.rawname))
     def lookup(self, name):
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index bfeb3bc..6f628cf 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -128,6 +128,11 @@
 #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
 
 /*
+ * libxl_domain_build_info has u.hvm.viridian_enable and _disable fields.
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_ENABLE_DISABLE 1
+
+/*
  * libxl ABI compatibility
  *
  * The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c944804..eb87e24 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -209,14 +209,80 @@ static unsigned long timer_mode(const libxl_domain_build_info *info)
     return ((unsigned long)mode);
 }
 
+#if defined(__i386__) || defined(__x86_64__)
+static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
+                                     libxl_domain_build_info *const info)
+{
+    libxl_bitmap enlightenments;
+    libxl_viridian_enlightenment v;
+    uint64_t mask = 0;
+
+    libxl_bitmap_init(&enlightenments);
+    libxl_bitmap_alloc(CTX, &enlightenments, libxl_viridian_enlightenment_max_val + 1);
+
+    if (libxl_defbool_val(info->u.hvm.viridian)) {
+        /* Enable defaults */
+        libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
+        libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
+    }
+
+    libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
+        if (libxl_bitmap_test(&info->u.hvm.viridian_disable, v)) {
+            LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "%s group both enabled and disabled",
+                       libxl_viridian_enlightenment_to_string(v));
+            goto err;
+        }
+        if (v <= libxl_viridian_enlightenment_max_val)
+            libxl_bitmap_set(&enlightenments, v);
+    }
+
+    libxl_for_each_set_bit(v, info->u.hvm.viridian_disable)
+        if (v <= libxl_viridian_enlightenment_max_val)
+            libxl_bitmap_reset(&enlightenments, v);
+
+    /* The base set is a pre-requisite for all others */
+    if (!libxl_bitmap_is_empty(&enlightenments) &&
+        !libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
+        LIBXL__LOG(CTX, LIBXL__LOG_ERROR, "base group not enabled");
+        goto err;
+    }
+
+    libxl_for_each_set_bit(v, enlightenments)
+        LOG(DETAIL, "%s group enabled", libxl_viridian_enlightenment_to_string(v));
+
+    if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE)) {
+        mask |= HVMPV_base_freq;
+
+        if (!libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ))
+            mask |= HVMPV_no_freq;
+    }
+
+    if (mask != 0 &&
+        xc_hvm_param_set(CTX->xch,
+                         domid,
+                         HVM_PARAM_VIRIDIAN,
+                         mask) != 0) {
+        LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_ERROR,
+                         "Couldn't set viridian feature mask (0x%"PRIx64")",
+                         mask);
+        goto err;
+    }
+
+    libxl_bitmap_dispose(&enlightenments);
+    return 0;
+
+err:
+    libxl_bitmap_dispose(&enlightenments);
+    return ERROR_FAIL;
+}
+#endif
+
 static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
                                 libxl_domain_build_info *const info)
 {
     xc_hvm_param_set(handle, 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,
                     libxl_defbool_val(info->u.hvm.hpet));
 #endif
@@ -350,8 +416,14 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
     state->store_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->store_domid);
     state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
 
-    if (info->type == LIBXL_DOMAIN_TYPE_HVM)
+    if (info->type == LIBXL_DOMAIN_TYPE_HVM) {
         hvm_set_conf_params(ctx->xch, domid, info);
+#if defined(__i386__) || defined(__x86_64__)
+        rc = hvm_set_viridian_features(gc, domid, info);
+        if (rc)
+            return rc;
+#endif
+    }
 
     rc = libxl__arch_domain_create(gc, d_config, domid);
 
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0b3496f..f383ce0 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -175,6 +175,12 @@ libxl_vendor_device = Enumeration("vendor_device", [
     (0, "NONE"),
     (1, "XENSERVER"),
     ])
+
+libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
+    (0, "base"),
+    (1, "freq"),
+    ])
+
 #
 # Complex libxl types
 #
@@ -365,6 +371,8 @@ libxl_domain_build_info = Struct("domain_build_info",[
                                        ("acpi_s4",          libxl_defbool),
                                        ("nx",               libxl_defbool),
                                        ("viridian",         libxl_defbool),
+                                       ("viridian_enable",  libxl_bitmap),
+                                       ("viridian_disable", libxl_bitmap),
                                        ("timeoffset",       string),
                                        ("hpet",             libxl_defbool),
                                        ("vpt_align",        libxl_defbool),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index f1c136a..58d4a1d 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -775,8 +775,8 @@ static void parse_config_data(const char *config_source,
     long l;
     XLU_Config *config;
     XLU_ConfigList *cpus, *vbds, *nics, *pcis, *cvfbs, *cpuids, *vtpms;
-    XLU_ConfigList *ioports, *irqs, *iomem;
-    int num_ioports, num_irqs, num_iomem, num_cpus;
+    XLU_ConfigList *ioports, *irqs, *iomem, *viridian;
+    int num_ioports, num_irqs, num_iomem, num_cpus, num_viridian;
     int pci_power_mgmt = 0;
     int pci_msitranslate = 0;
     int pci_permissive = 0;
@@ -993,10 +993,59 @@ 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(config, "viridian",
+                                 &viridian, &num_viridian, 1))
+        {
+        case 0: /* Success */
+            if (num_viridian) {
+                libxl_bitmap_alloc(ctx, &b_info->u.hvm.viridian_enable,
+                                   libxl_viridian_enlightenment_max_val + 1);
+                libxl_bitmap_alloc(ctx, &b_info->u.hvm.viridian_disable,
+                                   libxl_viridian_enlightenment_max_val + 1);
+            }
+            for (i = 0; i < num_viridian; i++) {
+                libxl_viridian_enlightenment v;
+
+                buf = xlu_cfg_get_listitem(viridian, i);
+                if (strcmp(buf, "all") == 0)
+                    libxl_bitmap_set_any(&b_info->u.hvm.viridian_enable);
+                else if (strcmp(buf, "defaults") == 0)
+                    libxl_defbool_set(&b_info->u.hvm.viridian, true);
+                else {
+                    libxl_bitmap *s = &b_info->u.hvm.viridian_enable;
+                    libxl_bitmap *r = &b_info->u.hvm.viridian_disable;
+
+                    if (*buf == '!') {
+                        s = &b_info->u.hvm.viridian_disable;
+                        r = &b_info->u.hvm.viridian_enable;
+                        buf++;
+                    }
+
+                    e = libxl_viridian_enlightenment_from_string(buf, &v);
+                    if (e) {
+                        fprintf(stderr,
+                                "xl: unknown viridian enlightenment '%s'\n",
+                                buf);
+                        exit(-ERROR_FAIL);
+                    }
+
+                    libxl_bitmap_set(s, v);
+                    libxl_bitmap_reset(r, v);
+                }
+            }
+            break;
+        case ESRCH: break; /* Option not present */
+        case EINVAL:
+            xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 1);
+            break;
+        default:
+            fprintf(stderr,"xl: Unable to parse viridian enlightenments.\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/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 94b18ba..8aeb8a6 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5554,8 +5554,24 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
                     rc = -EINVAL;
                 break;
             case HVM_PARAM_VIRIDIAN:
-                if ( a.value > 1 )
+                /* This should only ever be set once by the tools and read by the guest. */
+                rc = -EPERM;
+                if ( curr_d == d )
+                    break;
+
+                if ( a.value != d->arch.hvm_domain.params[a.index] )
+                {
+                    rc = -EEXIST;
+                    if ( d->arch.hvm_domain.params[a.index] != 0 )
+                        break;
+
                     rc = -EINVAL;
+                    if ( (a.value & ~HVMPV_feature_mask) ||
+                         !(a.value & HVMPV_base_freq) )
+                        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 b646a8a..28c4479 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)
+
 #endif
 
 /*
-- 
1.7.10.4

[PATCH v11 2/2] x86/viridian: Add partition time reference counter MSR support

[Paul Durrant]

This patch optionally re-instates support for the partition time reference
counter that was previously introduced by commit
e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b and reverted by commit
1cd4fab14ce25859efa4a2af13475e6650a5506c. The previous implementation was
non-optional and flawed.

This implementation uses the tsc of vcpu0, which is preserved across
save/restore as part of the architectural state, and then converts that
to a 100ns tick using the domain's tsc_khz.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Acked-by: 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>
Cc: Christoph Egger <chegger@amazon.de>
---
 docs/man/xl.cfg.pod.5            |    8 +++++++-
 tools/libxl/libxl_dom.c          |    4 ++++
 tools/libxl/libxl_types.idl      |    1 +
 xen/arch/x86/hvm/viridian.c      |   29 ++++++++++++++++++++++++-----
 xen/arch/x86/time.c              |    4 ++--
 xen/include/asm-x86/perfc_defn.h |    1 +
 xen/include/asm-x86/time.h       |    4 ++++
 xen/include/public/hvm/params.h  |    9 ++++++++-
 8 files changed, 51 insertions(+), 9 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 274de36..2ceb393 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1129,10 +1129,16 @@ This group incorporates the TSC and APIC frequency MSRs. These
 enlightenments can improve performance of Windows 7 and Windows
 Server 2008 R2 onwards.
 
+=item B<time_ref_count>
+
+This group incorporates Partition Time Reference Counter MSR. This
+enlightenment can improve performance of Windows 8 and Windows
+Server 2012 onwards.
+
 =item B<defaults>
 
 This is a special value that enables the default set of groups, which
-is currently the B<base> and B<freq> groups.
+is currently the B<base>, B<freq> and B<time_ref_count> groups.
 
 =item B<all>
 
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index eb87e24..cdcfe00 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -224,6 +224,7 @@ static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
         /* Enable defaults */
         libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
         libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
+        libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT);
     }
 
     libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
@@ -257,6 +258,9 @@ static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
             mask |= HVMPV_no_freq;
     }
 
+    if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT))
+        mask |= HVMPV_time_ref_count;
+
     if (mask != 0 &&
         xc_hvm_param_set(CTX->xch,
                          domid,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index f383ce0..0298a4c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -179,6 +179,7 @@ libxl_vendor_device = Enumeration("vendor_device", [
 libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
     (0, "base"),
     (1, "freq"),
+    (2, "time_ref_count"),
     ])
 
 #
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 28c4479..53138fd 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -36,11 +36,11 @@
 #define HvNotifyLongSpinWait    8
 
 /* Viridian CPUID 4000003, Viridian MSR availability. */
-#define CPUID3A_MSR_REF_COUNT   (1 << 1)
-#define CPUID3A_MSR_APIC_ACCESS (1 << 4)
-#define CPUID3A_MSR_HYPERCALL   (1 << 5)
-#define CPUID3A_MSR_VP_INDEX    (1 << 6)
-#define CPUID3A_MSR_FREQ        (1 << 11)
+#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1)
+#define CPUID3A_MSR_APIC_ACCESS    (1 << 4)
+#define CPUID3A_MSR_HYPERCALL      (1 << 5)
+#define CPUID3A_MSR_VP_INDEX       (1 << 6)
+#define CPUID3A_MSR_FREQ           (1 << 11)
 
 /* Viridian CPUID 4000004, Implementation Recommendations. */
 #define CPUID4A_MSR_BASED_APIC  (1 << 3)
@@ -93,6 +93,8 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
                 CPUID3A_MSR_VP_INDEX);
         if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
             *eax |= CPUID3A_MSR_FREQ;
+        if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
+            *eax |= CPUID3A_MSR_TIME_REF_COUNT;
         break;
     case 4:
         /* Recommended hypercall usage. */
@@ -344,6 +346,23 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
         *val = v->arch.hvm_vcpu.viridian.apic_assist.raw;
         break;
 
+    case VIRIDIAN_MSR_TIME_REF_COUNT:
+    {
+        uint64_t tsc;
+        struct time_scale tsc_to_ns;
+
+        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
+            return 0;
+
+        perfc_incr(mshv_rdmsr_time_ref_count);
+        tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
+
+        /* convert tsc to count of 100ns periods */
+        set_time_scale(&tsc_to_ns, (uint64_t)d->arch.tsc_khz * 1000ul);
+        *val = scale_delta(tsc, &tsc_to_ns) / 100ul;
+        break;
+    }
+
     default:
         return 0;
     }
diff --git a/xen/arch/x86/time.c b/xen/arch/x86/time.c
index bd89219..74c01e3 100644
--- a/xen/arch/x86/time.c
+++ b/xen/arch/x86/time.c
@@ -121,7 +121,7 @@ static inline u32 mul_frac(u32 multiplicand, u32 multiplier)
  * Scale a 64-bit delta by scaling and multiplying by a 32-bit fraction,
  * yielding a 64-bit result.
  */
-static inline u64 scale_delta(u64 delta, struct time_scale *scale)
+u64 scale_delta(u64 delta, struct time_scale *scale)
 {
     u64 product;
 
@@ -272,7 +272,7 @@ static u64 init_pit_and_calibrate_tsc(void)
     return ((end - start) * (u64)CALIBRATE_FRAC);
 }
 
-static void set_time_scale(struct time_scale *ts, u64 ticks_per_sec)
+void set_time_scale(struct time_scale *ts, u64 ticks_per_sec)
 {
     u64 tps64 = ticks_per_sec;
     u32 tps32;
diff --git a/xen/include/asm-x86/perfc_defn.h b/xen/include/asm-x86/perfc_defn.h
index 7d802cc..170da00 100644
--- a/xen/include/asm-x86/perfc_defn.h
+++ b/xen/include/asm-x86/perfc_defn.h
@@ -120,6 +120,7 @@ PERFCOUNTER(mshv_rdmsr_hc_page,         "MS Hv rdmsr hypercall page")
 PERFCOUNTER(mshv_rdmsr_vp_index,        "MS Hv rdmsr vp index")
 PERFCOUNTER(mshv_rdmsr_tsc_frequency,   "MS Hv rdmsr TSC frequency")
 PERFCOUNTER(mshv_rdmsr_apic_frequency,  "MS Hv rdmsr APIC frequency")
+PERFCOUNTER(mshv_rdmsr_time_ref_count,  "MS Hv rdmsr time ref count")
 PERFCOUNTER(mshv_rdmsr_icr,             "MS Hv rdmsr icr")
 PERFCOUNTER(mshv_rdmsr_tpr,             "MS Hv rdmsr tpr")
 PERFCOUNTER(mshv_rdmsr_apic_assist,     "MS Hv rdmsr APIC assist")
diff --git a/xen/include/asm-x86/time.h b/xen/include/asm-x86/time.h
index 420620e..c4d82f6 100644
--- a/xen/include/asm-x86/time.h
+++ b/xen/include/asm-x86/time.h
@@ -76,4 +76,8 @@ void cpuid_time_leaf(uint32_t sub_idx, uint32_t *eax, uint32_t *ebx,
 
 u64 stime2tsc(s_time_t stime);
 
+struct time_scale;
+void set_time_scale(struct time_scale *ts, u64 ticks_per_sec);
+u64 scale_delta(u64 delta, struct time_scale *scale);
+
 #endif /* __X86_TIME_H__ */
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 68d26fd..3c51072 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -88,7 +88,14 @@
 #define _HVMPV_no_freq 1
 #define HVMPV_no_freq  (1 << _HVMPV_no_freq)
 
-#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
+/* Enable Partition Time Reference Counter (HV_X64_MSR_TIME_REF_COUNT) */
+#define _HVMPV_time_ref_count 2
+#define HVMPV_time_ref_count  (1 << _HVMPV_time_ref_count)
+
+#define HVMPV_feature_mask \
+	(HVMPV_base_freq | \
+	 HVMPV_no_freq | \
+	 HVMPV_time_ref_count)
 
 #endif
 
-- 
1.7.10.4

Re: [PATCH v11 2/2] x86/viridian: Add partition time reference counter MSR support

[Jan Beulich]

>>> On 16.09.14 at 19:18, <paul.durrant@citrix.com> wrote:
> +    case VIRIDIAN_MSR_TIME_REF_COUNT:
> +    {
> +        uint64_t tsc;
> +        struct time_scale tsc_to_ns;
> +
> +        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> +            return 0;
> +
> +        perfc_incr(mshv_rdmsr_time_ref_count);
> +        tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> +
> +        /* convert tsc to count of 100ns periods */
> +        set_time_scale(&tsc_to_ns, (uint64_t)d->arch.tsc_khz * 1000ul);

As I keep pushing a private note for when this can eventually be
applied from one revision to the next - should you have to do
another round, could I ask you to drop the pointless cast here?

Jan

Re: [PATCH v11 2/2] x86/viridian: Add partition time reference counter MSR support

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 17 September 2014 11:45
> To: Paul Durrant
> Cc: Christoph Egger; Ian Campbell; Ian Jackson; Stefano Stabellini; xen-
> devel@lists.xen.org; Keir (Xen.org)
> Subject: Re: [Xen-devel] [PATCH v11 2/2] x86/viridian: Add partition time
> reference counter MSR support
> 
> >>> On 16.09.14 at 19:18, <paul.durrant@citrix.com> wrote:
> > +    case VIRIDIAN_MSR_TIME_REF_COUNT:
> > +    {
> > +        uint64_t tsc;
> > +        struct time_scale tsc_to_ns;
> > +
> > +        if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> > +            return 0;
> > +
> > +        perfc_incr(mshv_rdmsr_time_ref_count);
> > +        tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d));
> > +
> > +        /* convert tsc to count of 100ns periods */
> > +        set_time_scale(&tsc_to_ns, (uint64_t)d->arch.tsc_khz * 1000ul);
> 
> As I keep pushing a private note for when this can eventually be
> applied from one revision to the next - should you have to do
> another round, could I ask you to drop the pointless cast here?
> 

Sure. Hopefully I won't (now that I reached agreement with Ian J concerning the toolstack bits) but I'll stick an extra commit in my branch and fold it into this patch if I do.

  Paul

Re: [PATCH v11 0/2] x86/viridian improvements

[Paul Durrant]

Ping?

> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Paul Durrant
> Sent: 16 September 2014 18:19
> To: xen-devel@lists.xen.org
> Subject: [Xen-devel] [PATCH v11 0/2] x86/viridian improvements
> 
> This patch series incorporates several improvements to the code
> supporting viridian (i.e. hyper-v compatible) enlightenments for
> Windows guests:
> 
> Patch #1 series lays the foundations for adding new viridian
> enlightenments such that they can be optionally enabled, and not
> immediately exposed to a guest across a save/restore boundary.
> 
> Patch #2 adds support for the 'Partition Time Reference Counter'
> enlightenment.
> 
> v2:
> - Addressed comments from Jan Beulich
> - Added patch #2
> 
> v3:
> - Addressed comments from Andrew Cooper and Jan Beulich
> - Re-worked patch #2
> - Simplified patch #3 to use guest TSC
> 
> v4:
> - Added missing domain info to printks in patch #2
> 
> v5:
> - Clarified comment of patch #1 as suggested by David Vrabel
> - More logging tweaks in patch #2 as suggested by Andrew Cooper
> 
> v6:
> - Dropped previous patch to reduce logging verbosity as it has been
>   applied
> - Toolstack changes only:
>   - New libxl_integer_list and libxl_viridian_enlightenment types to
>     avoid passing of strings between xl and libxl, as requested by
>     Ian Jackson.
>   - Retained and deprecated viridian defbool rather than replacing it
>     to avoid API breakage pointed out by Ian Campbell. Enlightenment list
>     is now in a new hvm-only viridian_enlightenments field of the build
>     info.
> 
> v7:
> - Changes to patch #2 only:
>   - Reference time calculation now makes use of struct time_scale and
>     set_time_scale and scale_delta functions to avoid overflow issues
>     pointed out by Christoph Egger.
> 
> v8:
> - Changes to patch #2 only:
>   - Fixed 32- to 64-bit type promotion issue as pointed out by
>     Jan Beulich
> 
> v9:
> - Changes to ocaml stubs in patch #1 only
>   - There was a missing calloc in the libxl_integer_list ocaml stubs
> 
> v10:
> - Toolstack changes only:
>   - Re-worked toolstack code after discussion with Ian C and Ian J.
>     See http://lists.xen.org/archives/html/xen-devel/2014-09/msg00416.html
> 
> v11:
> - Toolstack changes only:
>   - Re-worked toolstack code after further discussion with Ian J.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v11 0/2] x86/viridian improvements

[Ian Campbell]

Is this waiting for tools or hypervisor input?

On Mon, 2014-09-22 at 13:36 +0000, Paul Durrant wrote:
> Ping?
> 
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Paul Durrant
> > Sent: 16 September 2014 18:19
> > To: xen-devel@lists.xen.org
> > Subject: [Xen-devel] [PATCH v11 0/2] x86/viridian improvements
> > 
> > This patch series incorporates several improvements to the code
> > supporting viridian (i.e. hyper-v compatible) enlightenments for
> > Windows guests:
> > 
> > Patch #1 series lays the foundations for adding new viridian
> > enlightenments such that they can be optionally enabled, and not
> > immediately exposed to a guest across a save/restore boundary.
> > 
> > Patch #2 adds support for the 'Partition Time Reference Counter'
> > enlightenment.
> > 
> > v2:
> > - Addressed comments from Jan Beulich
> > - Added patch #2
> > 
> > v3:
> > - Addressed comments from Andrew Cooper and Jan Beulich
> > - Re-worked patch #2
> > - Simplified patch #3 to use guest TSC
> > 
> > v4:
> > - Added missing domain info to printks in patch #2
> > 
> > v5:
> > - Clarified comment of patch #1 as suggested by David Vrabel
> > - More logging tweaks in patch #2 as suggested by Andrew Cooper
> > 
> > v6:
> > - Dropped previous patch to reduce logging verbosity as it has been
> >   applied
> > - Toolstack changes only:
> >   - New libxl_integer_list and libxl_viridian_enlightenment types to
> >     avoid passing of strings between xl and libxl, as requested by
> >     Ian Jackson.
> >   - Retained and deprecated viridian defbool rather than replacing it
> >     to avoid API breakage pointed out by Ian Campbell. Enlightenment list
> >     is now in a new hvm-only viridian_enlightenments field of the build
> >     info.
> > 
> > v7:
> > - Changes to patch #2 only:
> >   - Reference time calculation now makes use of struct time_scale and
> >     set_time_scale and scale_delta functions to avoid overflow issues
> >     pointed out by Christoph Egger.
> > 
> > v8:
> > - Changes to patch #2 only:
> >   - Fixed 32- to 64-bit type promotion issue as pointed out by
> >     Jan Beulich
> > 
> > v9:
> > - Changes to ocaml stubs in patch #1 only
> >   - There was a missing calloc in the libxl_integer_list ocaml stubs
> > 
> > v10:
> > - Toolstack changes only:
> >   - Re-worked toolstack code after discussion with Ian C and Ian J.
> >     See http://lists.xen.org/archives/html/xen-devel/2014-09/msg00416.html
> > 
> > v11:
> > - Toolstack changes only:
> >   - Re-worked toolstack code after further discussion with Ian J.
> > 
> > 
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel

Re: [PATCH v11 0/2] x86/viridian improvements

[Jan Beulich]

>>> On 22.09.14 at 16:04, <Ian.Campbell@citrix.com> wrote:
> Is this waiting for tools or hypervisor input?

The hypervisor side has been fine for a couple of revs I think.

Jan

> On Mon, 2014-09-22 at 13:36 +0000, Paul Durrant wrote:
>> Ping?
>> 
>> > -----Original Message-----
>> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
>> > bounces@lists.xen.org] On Behalf Of Paul Durrant
>> > Sent: 16 September 2014 18:19
>> > To: xen-devel@lists.xen.org 
>> > Subject: [Xen-devel] [PATCH v11 0/2] x86/viridian improvements
>> > 
>> > This patch series incorporates several improvements to the code
>> > supporting viridian (i.e. hyper-v compatible) enlightenments for
>> > Windows guests:
>> > 
>> > Patch #1 series lays the foundations for adding new viridian
>> > enlightenments such that they can be optionally enabled, and not
>> > immediately exposed to a guest across a save/restore boundary.
>> > 
>> > Patch #2 adds support for the 'Partition Time Reference Counter'
>> > enlightenment.
>> > 
>> > v2:
>> > - Addressed comments from Jan Beulich
>> > - Added patch #2
>> > 
>> > v3:
>> > - Addressed comments from Andrew Cooper and Jan Beulich
>> > - Re-worked patch #2
>> > - Simplified patch #3 to use guest TSC
>> > 
>> > v4:
>> > - Added missing domain info to printks in patch #2
>> > 
>> > v5:
>> > - Clarified comment of patch #1 as suggested by David Vrabel
>> > - More logging tweaks in patch #2 as suggested by Andrew Cooper
>> > 
>> > v6:
>> > - Dropped previous patch to reduce logging verbosity as it has been
>> >   applied
>> > - Toolstack changes only:
>> >   - New libxl_integer_list and libxl_viridian_enlightenment types to
>> >     avoid passing of strings between xl and libxl, as requested by
>> >     Ian Jackson.
>> >   - Retained and deprecated viridian defbool rather than replacing it
>> >     to avoid API breakage pointed out by Ian Campbell. Enlightenment list
>> >     is now in a new hvm-only viridian_enlightenments field of the build
>> >     info.
>> > 
>> > v7:
>> > - Changes to patch #2 only:
>> >   - Reference time calculation now makes use of struct time_scale and
>> >     set_time_scale and scale_delta functions to avoid overflow issues
>> >     pointed out by Christoph Egger.
>> > 
>> > v8:
>> > - Changes to patch #2 only:
>> >   - Fixed 32- to 64-bit type promotion issue as pointed out by
>> >     Jan Beulich
>> > 
>> > v9:
>> > - Changes to ocaml stubs in patch #1 only
>> >   - There was a missing calloc in the libxl_integer_list ocaml stubs
>> > 
>> > v10:
>> > - Toolstack changes only:
>> >   - Re-worked toolstack code after discussion with Ian C and Ian J.
>> >     See http://lists.xen.org/archives/html/xen-devel/2014-09/msg00416.html 
>> > 
>> > v11:
>> > - Toolstack changes only:
>> >   - Re-worked toolstack code after further discussion with Ian J.
>> > 
>> > 
>> > _______________________________________________
>> > Xen-devel mailing list
>> > Xen-devel@lists.xen.org 
>> > http://lists.xen.org/xen-devel 
>> 
>> _______________________________________________
>> Xen-devel mailing list
>> Xen-devel@lists.xen.org 
>> http://lists.xen.org/xen-devel 
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org 
> http://lists.xen.org/xen-devel 

Re: [PATCH v11 0/2] x86/viridian improvements

[Ian Campbell]

On Mon, 2014-09-22 at 15:59 +0100, Jan Beulich wrote:
> >>> On 22.09.14 at 16:04, <Ian.Campbell@citrix.com> wrote:
> > Is this waiting for tools or hypervisor input?
> 
> The hypervisor side has been fine for a couple of revs I think.

Thanks, it's on my list, but I've also CCd Ian J, who IIRC had more
opinions than me on the previous revision...

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

[Ian Jackson]

Paul Durrant writes ("[PATCH v11 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask"):
> The following commits introduced the time reference counter MSR and
> TSC/APIC frequency MSRs into the viridian feature set respectively:

The libxl and libxc parts of this, and the corresponding docs, look
good to me, apart from one part that I would like Ian C to comment on
- see below.

If you had separated that out into a patch of its own, I would have
given a tools ack to this one :-).

Thanks,
Ian.

>  tools/libxl/gentypes.py         |    6 +++
...
> diff --git a/tools/libxl/gentypes.py b/tools/libxl/gentypes.py
> index 3e73821..e101de4 100644
> --- a/tools/libxl/gentypes.py
> +++ b/tools/libxl/gentypes.py
> @@ -564,6 +564,8 @@ if __name__ == '__main__':
...
> +        f.write("%s %s_min_val = %d;\n" % (ty.typename, ty.typename, ty.min))
> +        f.write("%s %s_max_val = %d;\n" % (ty.typename, ty.typename, ty.max))
> +        f.write("\n")
> +
...
> diff --git a/tools/libxl/idl.py b/tools/libxl/idl.py
> index 437049e..983253d 100644
> --- a/tools/libxl/idl.py
> +++ b/tools/libxl/idl.py
> @@ -174,9 +174,15 @@ class Enumeration(Type):
>              self.namespace)
>  
>          self.values = []
> +        self.min = sys.maxint
> +        self.max = -sys.maxint - 1
>          for v in values:
>              # (value, name)
>              (num,name) = v
> +            if num < self.min:
> +                self.min = num
> +            if num > self.max:
> +                self.max = num

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

[Ian Campbell]

On Mon, 2014-09-22 at 17:48 +0100, Ian Jackson wrote:
> Paul Durrant writes ("[PATCH v11 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask"):
> > The following commits introduced the time reference counter MSR and
> > TSC/APIC frequency MSRs into the viridian feature set respectively:
> 
> The libxl and libxc parts of this, and the corresponding docs, look
> good to me, apart from one part that I would like Ian C to comment on
> - see below.
> 
> If you had separated that out into a patch of its own, I would have
> given a tools ack to this one :-).

AFAICT only the max_val is actually used, min_val I suppose is just for
completeness.

Can we not just either include an explicit MAX case in the enum itself
or provide a libxl function to allocate a suitably sized bitmap like we
do with libxl_cpu_bitmap_alloc (which lets us keep all this internal so
we don't need to think about the API implications too hard).

> +libxl_viridian_enlightenment = Enumeration("viridian_enlightenment",
> [
> +    (0, "base"),
> +    (1, "freq"),
> +    ])

Does this need to be kept in sync with the hvm.h values?.

> +/* 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)

Given that these have no backwards compatiblity reqs can't we avoid the
negative feature at the hypercall and DTRT in libxl?

Ian.

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

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 23 September 2014 09:35
> To: Ian Jackson
> Cc: Paul Durrant; xen-devel@lists.xen.org; Keir (Xen.org); Stefano Stabellini;
> Dave Scott
> Subject: Re: [PATCH v11 1/2] x86/viridian: Re-purpose the HVM parameter to
> be a feature mask
> 
> On Mon, 2014-09-22 at 17:48 +0100, Ian Jackson wrote:
> > Paul Durrant writes ("[PATCH v11 1/2] x86/viridian: Re-purpose the HVM
> parameter to be a feature mask"):
> > > The following commits introduced the time reference counter MSR and
> > > TSC/APIC frequency MSRs into the viridian feature set respectively:
> >
> > The libxl and libxc parts of this, and the corresponding docs, look
> > good to me, apart from one part that I would like Ian C to comment on
> > - see below.
> >
> > If you had separated that out into a patch of its own, I would have
> > given a tools ack to this one :-).
> 
> AFAICT only the max_val is actually used, min_val I suppose is just for
> completeness.
> 

Yes.

> Can we not just either include an explicit MAX case in the enum itself
> or provide a libxl function to allocate a suitably sized bitmap like we
> do with libxl_cpu_bitmap_alloc (which lets us keep all this internal so
> we don't need to think about the API implications too hard).
> 

Is there a definite problem with what I've done? Man and max values for enumerations seem like useful things to have. I could put an upper limit on the enlightenments but using this mechanism seems neater.

> > +libxl_viridian_enlightenment = Enumeration("viridian_enlightenment",
> > [
> > +    (0, "base"),
> > +    (1, "freq"),
> > +    ])
> 
> Does this need to be kept in sync with the hvm.h values?.
> 

It doesn't need to be in sync. Obviously, if another enlightenment is added then the chances are that something would have to be added here, but there's no necessarily a 1:1 correspondence.

> > +/* 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)
> 
> Given that these have no backwards compatiblity reqs can't we avoid the
> negative feature at the hypercall and DTRT in libxl?
> 

Not really. There are implications on migrate. A value of 1 in the HVM param needs to represent the set of enlightenments in Xen 4.4. This has already been discussed.

  Paul

> Ian.

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

[Ian Campbell]

On Tue, 2014-09-23 at 09:45 +0100, Paul Durrant wrote:
> > Can we not just either include an explicit MAX case in the enum itself
> > or provide a libxl function to allocate a suitably sized bitmap like we
> > do with libxl_cpu_bitmap_alloc (which lets us keep all this internal so
> > we don't need to think about the API implications too hard).
> > 
> 
> Is there a definite problem with what I've done?

It's adding a new public API which would need more careful
consideration/review.

>  Man and max values for enumerations seem like useful things to have.

This case is a bit different because they happen to represent bitmaps.
but in general we want users to be using the enum names/values and the
to/from string conversion routines, exposing the incidental min/max may
encourage them to try and do strange things.

>  I could put an upper limit on the enlightenments but using this mechanism seems neater.
> 
> > > +libxl_viridian_enlightenment = Enumeration("viridian_enlightenment",
> > > [
> > > +    (0, "base"),
> > > +    (1, "freq"),
> > > +    ])
> > 
> > Does this need to be kept in sync with the hvm.h values?.
> > 
> 
> It doesn't need to be in sync. Obviously, if another enlightenment is
> added then the chances are that something would have to be added here,
> but there's no necessarily a 1:1 correspondence.

1:1 correspondence was what I was trying to ask about (like the
SHUTDOWN_* values, which must match).

> 
> > > +/* 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)
> > 
> > Given that these have no backwards compatiblity reqs can't we avoid the
> > negative feature at the hypercall and DTRT in libxl?
> > 
> 
> Not really. There are implications on migrate. A value of 1 in the HVM
> param needs to represent the set of enlightenments in Xen 4.4. This
> has already been discussed.

So the answer is that this is transparently migrated via the hvmparam
and not taken care of by the toolstack, ok then.

I'd have though declaring 0x1 to be the "legacy set" and starting the
new mechanism at 0x2 onwards would have been neater in the long run, but
if the h/visor chaps are happy then fine.

Ian.

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

[Paul Durrant]

> -----Original Message-----
> From: Ian Campbell
> Sent: 23 September 2014 10:01
> To: Paul Durrant
> Cc: Ian Jackson; xen-devel@lists.xen.org; Keir (Xen.org); Stefano Stabellini;
> Dave Scott
> Subject: Re: [PATCH v11 1/2] x86/viridian: Re-purpose the HVM parameter to
> be a feature mask
> 
> On Tue, 2014-09-23 at 09:45 +0100, Paul Durrant wrote:
> > > Can we not just either include an explicit MAX case in the enum itself
> > > or provide a libxl function to allocate a suitably sized bitmap like we
> > > do with libxl_cpu_bitmap_alloc (which lets us keep all this internal so
> > > we don't need to think about the API implications too hard).
> > >
> >
> > Is there a definite problem with what I've done?
> 
> It's adding a new public API which would need more careful
> consideration/review.
> 
> >  Man and max values for enumerations seem like useful things to have.
> 
> This case is a bit different because they happen to represent bitmaps.
> but in general we want users to be using the enum names/values and the
> to/from string conversion routines, exposing the incidental min/max may
> encourage them to try and do strange things.

Ok, I guess I'll hardcode then. Where though, directly in libxl.h? Or would you prefer an enumeration member in the idl that's guaranteed to be the highest value member?

> 
> >  I could put an upper limit on the enlightenments but using this mechanism
> seems neater.
> >
> > > > +libxl_viridian_enlightenment =
> Enumeration("viridian_enlightenment",
> > > > [
> > > > +    (0, "base"),
> > > > +    (1, "freq"),
> > > > +    ])
> > >
> > > Does this need to be kept in sync with the hvm.h values?.
> > >
> >
> > It doesn't need to be in sync. Obviously, if another enlightenment is
> > added then the chances are that something would have to be added here,
> > but there's no necessarily a 1:1 correspondence.
> 
> 1:1 correspondence was what I was trying to ask about (like the
> SHUTDOWN_* values, which must match).
> 
> >
> > > > +/* 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)
> > >
> > > Given that these have no backwards compatiblity reqs can't we avoid the
> > > negative feature at the hypercall and DTRT in libxl?
> > >
> >
> > Not really. There are implications on migrate. A value of 1 in the HVM
> > param needs to represent the set of enlightenments in Xen 4.4. This
> > has already been discussed.
> 
> So the answer is that this is transparently migrated via the hvmparam
> and not taken care of by the toolstack, ok then.
> 

Yes, that's right.

> I'd have though declaring 0x1 to be the "legacy set" and starting the
> new mechanism at 0x2 onwards would have been neater in the long run, but
> if the h/visor chaps are happy then fine.
> 

But that's basically what I've done. The problem is that an addition was made to the legacy set in Xen 4.4 and the only way to optionally turn this off again is to add a negated feature. It's ugly, but it's the only way I can see to add control and stop MSRs from disappearing on migrate.

  Paul

> Ian.