* [PATCH v9 0/2] x86/viridian improvements
@ 2014-08-28 10:38 Paul Durrant
2014-08-28 10:38 ` [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
2014-08-28 10:38 ` [PATCH v9 2/2] x86/viridian: Add partition time reference counter MSR support Paul Durrant
0 siblings, 2 replies; 4+ messages in thread
From: Paul Durrant @ 2014-08-28 10:38 UTC (permalink / raw)
To: xen-devel
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-28 10:38 [PATCH v9 0/2] x86/viridian improvements Paul Durrant
@ 2014-08-28 10:38 ` Paul Durrant
2014-09-03 16:26 ` Ian Jackson
2014-08-28 10:38 ` [PATCH v9 2/2] x86/viridian: Add partition time reference counter MSR support Paul Durrant
1 sibling, 1 reply; 4+ messages in thread
From: Paul Durrant @ 2014-08-28 10:38 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, David Scott, Stefano Stabellini, Ian Jackson,
Paul Durrant, Ian Campbell
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 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>
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 | 53 +++++++++++++++++++------
tools/libxc/xc_domain_restore.c | 4 +-
tools/libxl/gentest.py | 16 +++++++-
tools/libxl/libxl.c | 59 ++++++++++++++++++++++++++++
tools/libxl/libxl.h | 22 +++++++++++
tools/libxl/libxl_dom.c | 71 +++++++++++++++++++++++++++++-----
tools/libxl/libxl_internal.h | 7 ++++
tools/libxl/libxl_json.c | 50 ++++++++++++++++++++++++
tools/libxl/libxl_json.h | 1 +
tools/libxl/libxl_types.idl | 13 +++++++
tools/libxl/xl_cmdimpl.c | 51 ++++++++++++++++++++++--
tools/ocaml/libs/xl/genwrap.py | 1 +
tools/ocaml/libs/xl/xenlight_stubs.c | 34 ++++++++++++++++
tools/python/xen/lowlevel/xl/xl.c | 6 +++
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 +++++++++++++++-
18 files changed, 434 insertions(+), 33 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index 1e04eed..d91f101 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1106,18 +1106,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 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/gentest.py b/tools/libxl/gentest.py
index 95323d1..2562ddf 100644
--- a/tools/libxl/gentest.py
+++ b/tools/libxl/gentest.py
@@ -21,7 +21,8 @@ def randomize_enum(e):
return random.choice([v.name for v in e.values])
handcoded = ["libxl_bitmap", "libxl_key_value_list",
- "libxl_cpuid_policy_list", "libxl_string_list"]
+ "libxl_cpuid_policy_list", "libxl_string_list",
+ "libxl_integer_list"]
def gen_rand_init(ty, v, indent = " ", parent = None):
s = ""
@@ -192,6 +193,19 @@ static void libxl_cpuid_policy_list_rand_init(libxl_cpuid_policy_list *pp)
*pp = p;
}
+static void libxl_integer_list_rand_init(libxl_integer_list *pil)
+{
+ int i, nr = rand() % 16;
+ libxl_integer_list il;
+
+ il.count = nr;
+ il.array = calloc(nr, sizeof(int));
+ for (i = 0; i < nr; i++)
+ il.array[i] = rand();
+
+ *pil = il;
+}
+
static void libxl_string_list_rand_init(libxl_string_list *p)
{
int i, nr = rand() % 16;
diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c
index 3526539..20fbbbd 100644
--- a/tools/libxl/libxl.c
+++ b/tools/libxl/libxl.c
@@ -192,6 +192,65 @@ int libxl_ctx_free(libxl_ctx *ctx)
return 0;
}
+void libxl_integer_list_init(libxl_integer_list *pil)
+{
+ pil->count = 0;
+}
+
+void libxl_integer_list_dispose(libxl_integer_list *pil)
+{
+ if (pil->count != 0)
+ free(pil->array);
+}
+
+void libxl_integer_list_append(libxl_ctx *ctx,
+ libxl_integer_list *pil,
+ int v)
+{
+ GC_INIT(ctx);
+ int i;
+
+ i = pil->count++;
+ pil->array = libxl__realloc(NOGC,
+ pil->array,
+ pil->count * sizeof(int));
+ pil->array[i] = v;
+
+ GC_FREE;
+}
+
+int libxl_integer_list_item(libxl_integer_list *pil, unsigned int i)
+{
+ assert(i < pil->count);
+
+ return pil->array[i];
+}
+
+void libxl_integer_list_copy(libxl_ctx *ctx,
+ libxl_integer_list *dst,
+ libxl_integer_list *src)
+{
+ GC_INIT(ctx);
+ int i;
+
+ dst->count = src->count;
+ if (dst->count == 0)
+ goto out;
+
+ dst->array = libxl__calloc(NOGC, dst->count, sizeof(int));
+
+ for (i = 0; i < dst->count; i++)
+ dst->array[i] = src->array[i];
+
+out:
+ GC_FREE;
+}
+
+int libxl_integer_list_length(const libxl_integer_list *pil)
+{
+ return pil->count;
+}
+
void libxl_string_list_dispose(libxl_string_list *psl)
{
int i;
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index bfeb3bc..3bebb5e 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -128,6 +128,15 @@
#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
/*
+ * LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_ENLIGHTENMENTS indicates that
+ * the viridian_enlightenments field is present in the hvm sections of
+ * libxl_domain_build_info. This field tells libxl which specific
+ * viridian (Hyper-V) enlightenments to enable, replacing the single
+ * viridian defbool which is only retained for API compatibility.
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_ENLIGHTENMENTS 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
@@ -584,6 +593,19 @@ typedef uint8_t libxl_mac[6];
#define LIBXL_MAC_BYTES(mac) mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]
void libxl_mac_copy(libxl_ctx *ctx, libxl_mac *dst, libxl_mac *src);
+typedef struct {
+ unsigned int count;
+ int *array;
+} libxl_integer_list;
+void libxl_integer_list_init(libxl_integer_list *il);
+void libxl_integer_list_append(libxl_ctx *ctx, libxl_integer_list *il,
+ int v);
+int libxl_integer_list_item(libxl_integer_list *il, unsigned int i);
+void libxl_integer_list_dispose(libxl_integer_list *il);
+int libxl_integer_list_length(const libxl_integer_list *il);
+void libxl_integer_list_copy(libxl_ctx *ctx, libxl_integer_list *dst,
+ libxl_integer_list *src);
+
typedef char **libxl_string_list;
void libxl_string_list_dispose(libxl_string_list *sl);
int libxl_string_list_length(const libxl_string_list *sl);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index c944804..e854984 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -209,21 +209,74 @@ 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 = 0;
+ unsigned int i, n;
+
+ if (libxl_defbool_val(info->u.hvm.viridian)) {
+ LOG(DETAIL, "base+freq");
+ feature_mask = HVMPV_base_freq;
+ goto done;
+ }
+
+ n = libxl_integer_list_length(&info->u.hvm.viridian_enlightenments);
+
+ if (n != 0) {
+ feature_mask = HVMPV_no_freq;
+
+ for (i = 0; i < n; i++) {
+ libxl_viridian_enlightenment v;
+
+ v = libxl_integer_list_item(&info->u.hvm.viridian_enlightenments, i);
+
+ LOG(DETAIL, "+%s", libxl_viridian_enlightenment_to_string(v));
+
+ switch (v) {
+ case LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE:
+ feature_mask |= HVMPV_base_freq;
+ break;
+ case LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ:
+ feature_mask &= ~HVMPV_no_freq;
+ break;
+ default:
+ LIBXL__LOG(CTX, LIBXL__LOG_WARNING,
+ "Unknown viridian enlightenment %d\n",
+ v);
+ break;
+ }
+ }
+ }
+
+done:
+ if (feature_mask != 0 &&
+ 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%"PRIx64")",
+ 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,
+ 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));
}
@@ -351,7 +404,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_internal.h b/tools/libxl/libxl_internal.h
index beb052e..62188f2 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -3168,6 +3168,8 @@ int libxl__cpuid_policy_list_parse_json(libxl__gc *gc,
libxl_cpuid_policy_list *p);
int libxl__string_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
libxl_string_list *p);
+int libxl__integer_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
+ libxl_integer_list *p);
int libxl__key_value_list_parse_json(libxl__gc *gc,
const libxl__json_object *o,
libxl_key_value_list *p);
@@ -3207,6 +3209,11 @@ static inline int libxl__hwcap_is_default(libxl_hwcap *hwcap)
return 0;
}
+static inline int libxl__integer_list_is_empty(libxl_integer_list *pil)
+{
+ return !libxl_integer_list_length(pil);
+}
+
static inline int libxl__string_list_is_empty(libxl_string_list *psl)
{
return !libxl_string_list_length(psl);
diff --git a/tools/libxl/libxl_json.c b/tools/libxl/libxl_json.c
index ceb014a..ffda84d 100644
--- a/tools/libxl/libxl_json.c
+++ b/tools/libxl/libxl_json.c
@@ -269,6 +269,56 @@ int libxl__key_value_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
return 0;
}
+yajl_gen_status libxl_integer_list_gen_json(yajl_gen hand, libxl_integer_list *p)
+{
+ yajl_gen_status s;
+ int i;
+
+ s = yajl_gen_array_open(hand);
+ if (s != yajl_gen_status_ok) goto out;
+
+ if (p->count == 0) goto empty;
+
+ for (i = 0; i < p->count; i++) {
+ s = yajl_gen_integer(hand, p->array[i]);
+ if (s != yajl_gen_status_ok) goto out;
+ }
+empty:
+ s = yajl_gen_array_close(hand);
+out:
+ return s;
+}
+
+int libxl__integer_list_parse_json(libxl__gc *gc, const libxl__json_object *o,
+ libxl_integer_list *p)
+{
+ const libxl__json_object *t;
+ flexarray_t *array = NULL;
+ int i;
+
+ if (!libxl__json_object_is_array(o))
+ return ERROR_FAIL;
+
+ array = libxl__json_object_get_array(o);
+
+ if (array->count == 0) {
+ p->count = 0;
+ return 0;
+ }
+
+ p->count = array->count;
+ p->array = libxl__calloc(NOGC, p->count, sizeof(int));
+
+ for (i = 0; (t = libxl__json_array_get(o, i)); i++) {
+ if (!libxl__json_object_is_integer(t))
+ return ERROR_FAIL;
+
+ p->array[i] = libxl__json_object_get_integer(t);
+ }
+
+ return 0;
+}
+
yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *pl)
{
libxl_string_list l = *pl;
diff --git a/tools/libxl/libxl_json.h b/tools/libxl/libxl_json.h
index af26e78..63ef5a1 100644
--- a/tools/libxl/libxl_json.h
+++ b/tools/libxl/libxl_json.h
@@ -30,6 +30,7 @@ yajl_gen_status libxl_bitmap_gen_json(yajl_gen hand, libxl_bitmap *p);
yajl_gen_status libxl_cpuid_policy_list_gen_json(yajl_gen hand,
libxl_cpuid_policy_list *p);
yajl_gen_status libxl_string_list_gen_json(yajl_gen hand, libxl_string_list *p);
+yajl_gen_status libxl_integer_list_gen_json(yajl_gen hand, libxl_integer_list *p);
yajl_gen_status libxl_key_value_list_gen_json(yajl_gen hand,
libxl_key_value_list *p);
yajl_gen_status libxl_hwcap_gen_json(yajl_gen hand, libxl_hwcap *p);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 0b3496f..3b0059c 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -22,6 +22,9 @@ libxl_cpuid_policy_list = Builtin("cpuid_policy_list", dispose_fn="libxl_cpuid_d
json_parse_type="JSON_ARRAY", check_default_fn="libxl__cpuid_policy_is_empty",
copy_fn="libxl_cpuid_policy_list_copy")
+libxl_integer_list = Builtin("integer_list", dispose_fn="libxl_integer_list_dispose", passby=PASS_BY_REFERENCE,
+ json_parse_type="JSON_ARRAY", check_default_fn="libxl__integer_list_is_empty",
+ copy_fn="libxl_integer_list_copy")
libxl_string_list = Builtin("string_list", dispose_fn="libxl_string_list_dispose", passby=PASS_BY_REFERENCE,
json_parse_type="JSON_ARRAY", check_default_fn="libxl__string_list_is_empty",
copy_fn="libxl_string_list_copy")
@@ -175,6 +178,13 @@ libxl_vendor_device = Enumeration("vendor_device", [
(0, "NONE"),
(1, "XENSERVER"),
])
+
+libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
+ (0, "unknown"),
+ (1, "base"),
+ (2, "freq"),
+ ])
+
#
# Complex libxl types
#
@@ -364,7 +374,10 @@ libxl_domain_build_info = Struct("domain_build_info",[
("acpi_s3", libxl_defbool),
("acpi_s4", libxl_defbool),
("nx", libxl_defbool),
+ # deprecated
("viridian", libxl_defbool),
+ # see libxl_viridian_enlightenment for valid values
+ ("viridian_enlightenments", libxl_integer_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 f1c136a..5b41658 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,55 @@ 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);
+ libxl_integer_list_init(&b_info->u.hvm.viridian_enlightenments);
+
+ switch (xlu_cfg_get_list(config, "viridian",
+ &viridian, &num_viridian, 1))
+ {
+ case 0: /* Success */
+ for (i = 0; i < num_viridian; i++) {
+ libxl_viridian_enlightenment v;
+
+ buf = xlu_cfg_get_listitem(viridian, i);
+ e = libxl_viridian_enlightenment_from_string(buf, &v);
+ if (e) {
+ fprintf(stderr,
+ "xl: unknown viridian enlightenment '%s'\n",
+ buf);
+ exit(-ERROR_FAIL);
+ }
+
+ libxl_integer_list_append(ctx,
+ &b_info->u.hvm.viridian_enlightenments,
+ v);
+ }
+ break;
+ 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");
+ libxl_integer_list_append(ctx,
+ &b_info->u.hvm.viridian_enlightenments,
+ LIBXL_VIRIDIAN_ENLIGHTENMENT_BASE);
+ libxl_integer_list_append(ctx,
+ &b_info->u.hvm.viridian_enlightenments,
+ LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
+ }
+ 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/tools/ocaml/libs/xl/genwrap.py b/tools/ocaml/libs/xl/genwrap.py
index 402e489..5ef9010 100644
--- a/tools/ocaml/libs/xl/genwrap.py
+++ b/tools/ocaml/libs/xl/genwrap.py
@@ -15,6 +15,7 @@ builtins = {
"libxl_uuid": ("int array", "Uuid_val(&%(c)s, %(o)s)", "Val_uuid(&%(c)s)"),
"libxl_bitmap": ("bool array", "Bitmap_val(ctx, &%(c)s, %(o)s)", "Val_bitmap(&%(c)s)"),
"libxl_key_value_list": ("(string * string) list", "libxl_key_value_list_val(&%(c)s, %(o)s)", "Val_key_value_list(&%(c)s)"),
+ "libxl_integer_list": ("int list", "libxl_integer_list_val(&%(c)s, %(o)s)", "Val_integer_list(&%(c)s)"),
"libxl_string_list": ("string list", "libxl_string_list_val(&%(c)s, %(o)s)", "Val_string_list(&%(c)s)"),
"libxl_mac": ("int array", "Mac_val(&%(c)s, %(o)s)", "Val_mac(&%(c)s)"),
"libxl_hwcap": ("int32 array", None, "Val_hwcap(&%(c)s)"),
diff --git a/tools/ocaml/libs/xl/xenlight_stubs.c b/tools/ocaml/libs/xl/xenlight_stubs.c
index 4133527..8f06023 100644
--- a/tools/ocaml/libs/xl/xenlight_stubs.c
+++ b/tools/ocaml/libs/xl/xenlight_stubs.c
@@ -189,6 +189,40 @@ static value Val_key_value_list(libxl_key_value_list *c_val)
CAMLreturn(list);
}
+static int libxl_integer_list_val(libxl_integer_list *c_val, value v)
+{
+ CAMLparam1(v);
+ int i;
+ libxl_integer_list l;
+
+ l.count = list_len(v);
+ l.array = calloc(l.count, sizeof(int));
+
+ for (i = 0; v != Val_emptylist; i++, v = Field(v, 1) )
+ l.array[i] = Int_val(Field(v, 0));
+
+ *c_val = l;
+ CAMLreturn(0);
+}
+
+static value Val_integer_list(libxl_integer_list *c_val)
+{
+ CAMLparam0();
+ CAMLlocal3(list, cons, integer);
+ int i;
+
+ list = Val_emptylist;
+ for (i = c_val->count; i >= 0; i--) {
+ integer = caml_copy_nativeint(c_val->array[i]);
+ cons = caml_alloc(2, 0);
+ Store_field(cons, 0, integer); // head
+ Store_field(cons, 1, list); // tail
+ list = cons;
+ }
+
+ CAMLreturn(list);
+}
+
static int libxl_string_list_val(libxl_string_list *c_val, value v)
{
CAMLparam1(v);
diff --git a/tools/python/xen/lowlevel/xl/xl.c b/tools/python/xen/lowlevel/xl/xl.c
index 32f982a..9ee5451 100644
--- a/tools/python/xen/lowlevel/xl/xl.c
+++ b/tools/python/xen/lowlevel/xl/xl.c
@@ -265,6 +265,12 @@ int attrib__libxl_mac_set(PyObject *v, libxl_mac *pptr)
return fixed_bytearray_set(v, *pptr, 6);
}
+int attrib__libxl_integer_list_set(PyObject *v, libxl_integer_list *pptr)
+{
+ PyErr_SetString(PyExc_NotImplementedError, "Setting integer_list");
+ return -1;
+}
+
int attrib__libxl_string_list_set(PyObject *v, libxl_string_list *pptr)
{
PyErr_SetString(PyExc_NotImplementedError, "Setting string_list");
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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH v9 2/2] x86/viridian: Add partition time reference counter MSR support
2014-08-28 10:38 [PATCH v9 0/2] x86/viridian improvements Paul Durrant
2014-08-28 10:38 ` [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
@ 2014-08-28 10:38 ` Paul Durrant
1 sibling, 0 replies; 4+ messages in thread
From: Paul Durrant @ 2014-08-28 10:38 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, Christoph Egger,
Ian Jackson, 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 | 7 +++++++
tools/libxl/libxl_dom.c | 3 +++
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, 50 insertions(+), 8 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index d91f101..ac3798a 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1131,6 +1131,13 @@ This set incorporates the TSC and APIC frequency MSRs.
This enlightenment can improve performance of Windows 7 and Windows
Server 2008 R2 onwards.
+=item B<time_ref_count>
+
+This set incorporates Partition Time Reference Counter MSR.
+
+This enlightenment can improve performance of Windows 8 and Windows
+Server 2012 onwards.
+
=back
See the latest version of Microsoft's Hypervisor Top-Level Functional
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index e854984..579188a 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -241,6 +241,9 @@ static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
case LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ:
feature_mask &= ~HVMPV_no_freq;
break;
+ case LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT:
+ feature_mask |= HVMPV_time_ref_count;
+ break;
default:
LIBXL__LOG(CTX, LIBXL__LOG_WARNING,
"Unknown viridian enlightenment %d\n",
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index 3b0059c..a2a523e 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -183,6 +183,7 @@ libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
(0, "unknown"),
(1, "base"),
(2, "freq"),
+ (3, "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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-28 10:38 ` [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
@ 2014-09-03 16:26 ` Ian Jackson
0 siblings, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2014-09-03 16:26 UTC (permalink / raw)
To: Paul Durrant
Cc: David Scott, Stefano Stabellini, Keir Fraser, Ian Campbell,
xen-devel
Paul Durrant writes ("[PATCH v9 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 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".
We had a conversation on IRC about the API. See below.
For libxl I think this means:
- Keep the existing flag
- Introduce two new bitmaps called something like
viridian_features_{enable,disable}
- Features actually enabled are
!defbool_value(viridian) ? 0 :
((viridian_features_enable | default) & ~viridian_features_disable)
The feature bits would be provided via a libxl enum either specified
with values 1,2,4 etc. or (better) with a new `shifted' property.
For xl I would suggest:
Keep the existing config name and do not introduce any new variables.
Existing values 0 and 1 (or false and true) mean `enable, defaults'.
Alternatively user can specify comma-separated list of {feature
name, or `all'} each optionally preceded by `!'. That means
`enable, adjust feature set accordingly'.
Ian.
16:25 <Diziet> ijc: Looking at this viridian stuff I'm not particularly
enamoured by this integer list API for something that could be a
bitmask. Also it lacks sensible defaulting.
16:25 <Diziet> What do you think ?
16:26 <@ijc> Diziet: IIRC my opinion was there should be a defbool for each
individual feature
16:27 <@ijc> a bitmask or a list lacks the necessary defaulting behaviour I
think.
16:27 <@ijc> a bitmask is certainly better than a list though, but not my
prefeence
16:27 <xadimgnik> isn't an empty list a reasonable default?
16:28 <@ijc> that only allows a global default, not a per feature default, I
think
16:28 <Diziet> If we want to add a feature in the future that's on by default,
we can't do so, with the current API.
16:28 <Diziet> If we are sure never to want to do that then I guess we can live
with it.
16:29 <xadimgnik> why not? you'd simply default it to on in libxl unless an
option turning it off were passed in the list
16:29 <Diziet> For defaulting behaviour with bitmasks we would need to have
separate "enable" and "disable" bitmasks.
16:30 <Diziet> xadimgnik: If we do that we can't turn it off by default in the
future.
16:30 <Diziet> Also I wonder whether much of the time people would prefer to
specify "all available"
16:30 <Diziet> Which the current API doesn't provide. A bitmask would if you
were permitted to pass ~
16:30 <Diziet> 0
16:32 <xadimgnik> hmm, 'all available' may be reasonable... I'd kind of prefer
it if folks actually had to specify what they wanted though
16:32 <Diziet> Why would you prefer that ?
16:33 <xadimgnik> using all available would mean new enlightenments get turned
on by default when VMs are moved to newer versions of Xen
16:34 <xadimgnik> and that may cause some surprise in some cases I guess
16:35 <Diziet> `Moved' = migrated ? Or just booted on the new setup ?
16:36 <xadimgnik> well the enlightenment would remain off on migrate (because
the save record has the actual viridian HVM param in it), but
would become enabled on reboot when the 'all available' in
the config file takes effect
16:37 <Diziet> That doesn't sound very dangerous. Perhaps it's not desirable
in all configurations but forbidding the user from specifying it
seems rather strong.
16:37 <@ijc> I still think an explicit named defbool per enlightenment is
better than a bitmap
16:38 <xadimgnik> Not dangerous, but it's the sort of thing that leads to
support tickets being raised against XenServer
16:38 <Diziet> ijc: How would the user specify "all available" ?
16:38 <Diziet> xadimgnik: So you can just not do that for your users.
16:43 <xadimgnik> yes, that's true; if you want an 'all available' then maybe a
bitmask is the way to go
16:44 <@ijc> Diziet: all available wold be leave them all as default,
wouldn'tit?
16:45 <@ijc> I'd imageind that the existing field (which must remain) would
serve as a global on/off switch
16:46 <xadimgnik> perhaps leave the boolean option in xl.cfg and then have a
list which, if empty, means 'all available'?
16:48 <Diziet> Let's think firstly about API. The existing API must remain but
I thought it was going to be deprecated.
16:49 <Diziet> If we don't deprecate it then we can use it to gate the whole
thing.
16:49 <Diziet> In which case perhaps we don't need per-feature defaults. I'm
not sure.
16:49 <Diziet> Personally I think simpler would be to have two bitmaps, enable
and disable, and keep the old defbool for compatibility only.
16:50 <Diziet> With a rule that passing ~0 for either enable or disable is
permitted. !!(enable & disable) would be an error.
16:51 <xadimgnik> I'm ok with that if ijc is
16:51 <Diziet> libxl would translate "viridian='on'" to enable==~0
16:52 <Diziet> ijc's proposal seems to be that each individual one would have
its own defbool defaulting to on, gated on the existing defbool
which defaults to 0.
16:53 <Diziet> But that would make it difficult to specify "exactly these"
because there's no comprehensive list of the defbools to set to
false.
16:53 <@ijc> own defbool defaulting to a sane defaultm, no necedssarily on
16:53 <Diziet> ijc: Right.
16:53 <@ijc> A helper "set all to false"?
16:53 <Diziet> How ugly.
16:53 <@ijc> With a bitmap what happens when the 33rd enlightment is added? (or
the 65th, or the 129th...)
16:53 <Diziet> Are we expecting even 32 of these ?
16:54 <Diziet> If we run out of bitmap we make a new one.
16:54 <xadimgnik> not really
16:54 <Diziet> If it might be that a sane default for `I'm running Windows give
me the best thing' is not `all enabled' then my proposal is
insufficiently rich.
16:54 <Diziet> In that case I prefer ijc's proposal but with a bitmap instead
of the defbool. Or we could have an array of defbools indexed
by enum or something.
16:55 <Diziet> s/a bitmap/two bitmaps/
16:55 <Diziet> s/the defbool/&s
16:56 <Diziet> If we go with bitmaps then the enum values should be called
*_shift so that when we fix the API to have the flag values
directly we haven't stolen the proper name.
16:57 <Diziet> (Or we could fix the idl compiler right away, I guess, although
that's a bit of a can of worms at this stage.)
16:58 -!- sstabellini [~sstabelli@c-50-184-120-159.hsd1.ca.comcast.net] has
joined #xendevel
16:59 <Diziet> ijc: ^
17:00 <@ijc> what needs changing in teh IDL?
17:00 <Diziet> You should be able to write .viridian_disable =
some_flag|some_other_flag
17:01 <Diziet> when right now if we just use an enum you would have to write
.viridian_disable = (1UL<<some_flag)|(1UL<<some_other_flag)
17:02 <@ijc> Or declare the enum with values 1, 2, 4 etc. I suppose adding a
"shifted" parameter to the Enumeration class would be reasonably
trivial
17:06 <Diziet> ijc: Either would work. And we could do the former and switch
to the latter later.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-03 16:26 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-08-28 10:38 [PATCH v9 0/2] x86/viridian improvements Paul Durrant
2014-08-28 10:38 ` [PATCH v9 1/2] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
2014-09-03 16:26 ` Ian Jackson
2014-08-28 10:38 ` [PATCH v9 2/2] x86/viridian: Add partition time reference counter MSR support Paul Durrant
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).