* [PATCH v2 0/3] x86/viridian improvements
@ 2014-08-04 13:12 Paul Durrant
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
` (2 more replies)
0 siblings, 3 replies; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 13:12 UTC (permalink / raw)
To: xen-devel
This patch 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 fixes a possible DoS issue by correctly noting that all the
logging in the viridian code is actually under the control of the guest
Patch #3 adds support for the 'Partition Time Reference Counter'
enlightenment.
v2:
- Addressed comments from Jan Beulich
- Added patch #2
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:12 [PATCH v2 0/3] x86/viridian improvements Paul Durrant
@ 2014-08-04 13:12 ` Paul Durrant
2014-08-04 13:31 ` Andrew Cooper
2014-08-04 13:45 ` Jan Beulich
2014-08-04 13:12 ` [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest Paul Durrant
2014-08-04 13:12 ` [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant
2 siblings, 2 replies; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, Ian Jackson,
Paul Durrant, Jan Beulich
The following commits introduced the time reference counter MSR and
TSC/APIC frequency MSRs into the viridian feature set respectively:
e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b
84657efd9116f40924aa13c9d5a349e007da716f
The time reference counter MSR feature was then reverted by commit
1cd4fab14ce25859efa4a2af13475e6650a5506c
because a flaw in the implementation meant the counter was reset on
migration.
All of these changes were made without any addtional options being
added to the VM configuration, or any compatibility checks being made
in the domain save/restore code. Hence setting the single boolean
'viridian' option in the VM configuration yields a different set of
features depending on which version of Xen the VM is started on, and the
feature set can change across migration (so new MSRs can magically appear).
This patch grandfathers in the current viridian features set and calls them
the 'base' plus 'freq' feature set. HVM_PARAM_VIRIDIAN is re-purposed as
a feature mask. It has only ever been legally set to 0 or 1, so the presence
of the base set and freq set are indicated by setting bit 0. The 'freq' set
can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4
'base' set. Newly implemented viridian features can be optionally enabled
in future by setting further bits.
The viridian option in xl.cfg(5) has also been changed to a string list so
that the sets can be individually sepcified. For compatibility, if the
option is specified as a boolean, then a true (1) value will be translated
to a string list containing "base" and "freq".
This patch also alters the allowed write accesses to HVM_PARAM_VIRIDIAN.
Currently there is nothing to stop the guest writing this value (which,
while harmless to anything else, should not happen) and nothing to
stop a toolstack from setting the value back to zero whilst the guest is
running, causing CPUID leaves to disappear and MSR accesses to start
causing GPFs in the guest. Both of these possibilities are now disallowed:
Once the parameter is set to a non-zero value it may not be cleared, and
guests no longer have any write access.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
docs/man/xl.cfg.pod.5 | 53 ++++++++++++++++++++++++++++++---------
tools/libxc/xc_domain_restore.c | 4 +--
tools/libxl/libxl.h | 6 +++++
tools/libxl/libxl_create.c | 1 -
tools/libxl/libxl_dom.c | 46 ++++++++++++++++++++++++++-------
tools/libxl/libxl_types.idl | 2 +-
tools/libxl/xl_cmdimpl.c | 24 +++++++++++++++++-
tools/libxl/xl_sxp.c | 8 ++++--
xen/arch/x86/hvm/hvm.c | 18 +++++++++++--
xen/arch/x86/hvm/viridian.c | 21 ++++++++++++++--
xen/include/asm-x86/hvm/hvm.h | 7 ++++--
xen/include/public/hvm/params.h | 33 +++++++++++++++++++++++-
12 files changed, 188 insertions(+), 35 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index ff9ea77..a05e831 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1084,18 +1084,47 @@ Windows L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
Setting B<xen_platform_pci=0> with the default device_model "qemu-xen"
requires at least QEMU 1.6.
-=item B<viridian=BOOLEAN>
-
-Turns on or off the exposure of MicroSoft Hyper-V (AKA viridian)
-compatible enlightenments to the guest. These can improve performance
-of Microsoft Windows guests from Windows Vista and Windows 2008
-onwards and setting this option for such guests is strongly
-recommended. This option should be harmless for other versions of
-Windows (although it will not give any benefit) and the majority of
-other non-Windows OSes. However it is known to be incompatible with
-some other Operating Systems and in some circumstance can prevent
-Xen's own paravirtualisation interfaces for HVM guests from being
-used.
+=item B<viridian=[ "SET", "SET", ...]>
+
+The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
+exposed to the guest. The following sets of enlightenments may be
+specified:
+
+=over 4
+
+=item B<base>
+
+This set incorporates the Hypercall MSRs, Virtual processor index MSR,
+and APIC access MSRs. This set is a pre-requisite for all other sets.
+If it is not specified then all enlightenments will be disabled.
+
+These enlightenments can improve performance of Windows Vista and Windows
+Server 2008 onwards and setting this option for such guests is strongly
+recommended.
+
+=item B<freq>
+
+This set incorporates the TSC and APIC frequency MSRs.
+
+This enlightenment can improve performance of Windows 7 and Windows
+Server 2008 R2 onwards.
+
+=back
+
+See the latest version of Microsoft's Hypervisor Top-Level Functional
+Specification for more details.
+
+The enlightenments should be harmless for other versions of Windows
+(although they will not give any benefit) and the majority of other
+non-Windows OSes.
+However it is known that they are incompatible with some other Operating
+Systems and in some circumstance can prevent Xen's own paravirtualisation
+interfaces for HVM guests from being used.
+
+Specifying the viridian option as a boolean is deprecated. However, for
+backwards compatibility, if it is specified as a boolean a value of
+true (1) will cause both the 'base' and 'freq' sets to be exposed to
+the guest, and a value of false (0) will disable all enlightenments.
=back
diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
index e73e0a2..5e7e62d 100644
--- a/tools/libxc/xc_domain_restore.c
+++ b/tools/libxc/xc_domain_restore.c
@@ -922,7 +922,7 @@ static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
{
- PERROR("error read the viridian flag");
+ PERROR("error read the viridian flags");
return -1;
}
return pagebuf_get_one(xch, ctx, buf, fd, dom);
@@ -1733,7 +1733,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
}
if (pagebuf.viridian != 0)
- xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, 1);
+ xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, pagebuf.viridian);
/*
* If we are migrating in from a host that does not support
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 5ae6532..b985f49 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -128,6 +128,12 @@
#define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
/*
+ * The libxl_domain_build_info u.hvm.viridian field is a string list
+ */
+#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_STRING_LIST 1
+
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 0686f96..ee19fc8 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -273,7 +273,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3, true);
libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4, true);
libxl_defbool_setdefault(&b_info->u.hvm.nx, true);
- libxl_defbool_setdefault(&b_info->u.hvm.viridian, false);
libxl_defbool_setdefault(&b_info->u.hvm.hpet, true);
libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm, false);
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index 83eb29a..a15c185 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -209,21 +209,49 @@ static unsigned long timer_mode(const libxl_domain_build_info *info)
return ((unsigned long)mode);
}
-static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
+#if defined(__i386__) || defined(__x86_64__)
+static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
+ libxl_domain_build_info *const info)
+{
+ uint64_t feature_mask = HVMPV_no_freq;
+
+ char **p = info->u.hvm.viridian;
+ while (*p) {
+ LOG(DETAIL, "+%s", *p);
+ if (strcmp(*p, "base") == 0)
+ feature_mask |= HVMPV_base_freq;
+ else if (strcmp(*p, "freq") == 0)
+ feature_mask &= ~HVMPV_no_freq;
+ p++;
+ }
+
+ if (xc_hvm_param_set(CTX->xch,
+ domid,
+ HVM_PARAM_VIRIDIAN,
+ feature_mask) != 0)
+ LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_WARNING,
+ "Couldn't set viridian feature mask (0x%lx)",
+ feature_mask);
+}
+#endif
+
+static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
libxl_domain_build_info *const info)
{
- xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED,
libxl_defbool_val(info->u.hvm.pae));
#if defined(__i386__) || defined(__x86_64__)
- xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN,
- libxl_defbool_val(info->u.hvm.viridian));
- xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
+ if (libxl_string_list_length(&info->u.hvm.viridian) != 0)
+ hvm_set_viridian_features(gc, domid, info);
+
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_HPET_ENABLED,
libxl_defbool_val(info->u.hvm.hpet));
#endif
- xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE, timer_mode(info));
- xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_TIMER_MODE,
+ timer_mode(info));
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_VPT_ALIGN,
libxl_defbool_val(info->u.hvm.vpt_align));
- xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
+ xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_NESTEDHVM,
libxl_defbool_val(info->u.hvm.nested_hvm));
}
@@ -306,7 +334,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
if (info->type == LIBXL_DOMAIN_TYPE_HVM)
- hvm_set_conf_params(ctx->xch, domid, info);
+ hvm_set_conf_params(gc, domid, info);
rc = libxl__arch_domain_create(gc, d_config, domid);
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a412f9c..6c5d7e0 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -363,7 +363,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
("acpi_s3", libxl_defbool),
("acpi_s4", libxl_defbool),
("nx", libxl_defbool),
- ("viridian", libxl_defbool),
+ ("viridian", libxl_string_list),
("timeoffset", string),
("hpet", libxl_defbool),
("vpt_align", libxl_defbool),
diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
index 01bce2f..960f626 100644
--- a/tools/libxl/xl_cmdimpl.c
+++ b/tools/libxl/xl_cmdimpl.c
@@ -951,10 +951,32 @@ static void parse_config_data(const char *config_source,
xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
- xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 0);
xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
+ switch (xlu_cfg_get_list_as_string_list(config, "viridian",
+ &b_info->u.hvm.viridian, 1))
+ {
+ case 0: break; /* Success */
+ case ESRCH: break; /* Option not present */
+ case EINVAL: {
+ libxl_defbool b;
+
+ xlu_cfg_get_defbool(config, "viridian", &b, 1);
+ if (libxl_defbool_val(b)) {
+ fprintf(stderr, "WARNING: Specifying \"viridian\""
+ " as a boolean is deprecated. "
+ "Please use a list of features.\n");
+ split_string_into_string_list("base freq", " \n",
+ &b_info->u.hvm.viridian);
+ }
+ break;
+ }
+ default:
+ fprintf(stderr,"xl: Unable to parse bootloader_args.\n");
+ exit(-ERROR_FAIL);
+ }
+
if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
const char *s = libxl_timer_mode_to_string(l);
fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer is deprecated. "
diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
index 48eb608..347526f 100644
--- a/tools/libxl/xl_sxp.c
+++ b/tools/libxl/xl_sxp.c
@@ -97,8 +97,12 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config)
printf("\t\t\t(acpi %s)\n",
libxl_defbool_to_string(b_info->u.hvm.acpi));
printf("\t\t\t(nx %s)\n", libxl_defbool_to_string(b_info->u.hvm.nx));
- printf("\t\t\t(viridian %s)\n",
- libxl_defbool_to_string(b_info->u.hvm.viridian));
+ if (b_info->u.hvm.viridian) {
+ printf("\t\t\t(viridian");
+ for (i=0; b_info->u.hvm.viridian[i]; i++)
+ printf(" %s", b_info->u.hvm.viridian[i]);
+ printf(")\n");
+ }
printf("\t\t\t(hpet %s)\n",
libxl_defbool_to_string(b_info->u.hvm.hpet));
printf("\t\t\t(vpt_align %s)\n",
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index fba13e0..04d43d0 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
rc = -EINVAL;
break;
case HVM_PARAM_VIRIDIAN:
- if ( a.value > 1 )
- rc = -EINVAL;
+ /* This should only ever be set once by the tools and read by the guest. */
+ rc = -EPERM;
+ if ( curr_d == d )
+ break;
+
+ rc = -EPERM;
+ if ( d->arch.hvm_domain.params[a.index] &&
+ a.value != d->arch.hvm_domain.params[a.index] )
+ break;
+
+ 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 0fcbfd8..31c9656 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
/* Which hypervisor MSRs are available to the guest */
*eax = (CPUID3A_MSR_APIC_ACCESS |
CPUID3A_MSR_HYPERCALL |
- CPUID3A_MSR_VP_INDEX |
- CPUID3A_MSR_FREQ);
+ CPUID3A_MSR_VP_INDEX);
+ if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
+ *eax |= CPUID3A_MSR_FREQ;
break;
case 4:
/* Recommended hypercall usage. */
@@ -312,11 +313,17 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
break;
case VIRIDIAN_MSR_TSC_FREQUENCY:
+ if ( viridian_feature_mask(d) & HVMPV_no_freq )
+ return 0;
+
perfc_incr(mshv_rdmsr_tsc_frequency);
*val = (uint64_t)d->arch.tsc_khz * 1000ull;
break;
case VIRIDIAN_MSR_APIC_FREQUENCY:
+ if ( viridian_feature_mask(d) & HVMPV_no_freq )
+ return 0;
+
perfc_incr(mshv_rdmsr_apic_frequency);
*val = 1000000000ull / APIC_BUS_CYCLE_NS;
break;
@@ -489,3 +496,13 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
index 0ebd478..24e513b 100644
--- a/xen/include/asm-x86/hvm/hvm.h
+++ b/xen/include/asm-x86/hvm/hvm.h
@@ -344,8 +344,11 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
return hvm_funcs.get_shadow_gs_base(v);
}
-#define is_viridian_domain(_d) \
- (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
+#define viridian_feature_mask(d) \
+ ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
+
+#define is_viridian_domain(d) \
+ (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq))
void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
uint32_t *eax, uint32_t *ebx,
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 614ff5f..68d26fd 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -56,9 +56,40 @@
#if defined(__i386__) || defined(__x86_64__)
-/* Expose Viridian interfaces to this HVM guest? */
+/*
+ * Viridian enlightenments
+ *
+ * (See http://download.microsoft.com/download/A/B/4/AB43A34E-BDD0-4FA6-BDEF-79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.docx)
+ *
+ * To expose viridian enlightenments to the guest set this parameter
+ * to the desired feature mask. The base feature set must be present
+ * in any valid feature mask.
+ */
#define HVM_PARAM_VIRIDIAN 9
+/* Base+Freq viridian feature sets:
+ *
+ * - Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and HV_X64_MSR_HYPERCALL)
+ * - APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR)
+ * - Virtual Processor index MSR (HV_X64_MSR_VP_INDEX)
+ * - Timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
+ * HV_X64_MSR_APIC_FREQUENCY)
+ */
+#define _HVMPV_base_freq 0
+#define HVMPV_base_freq (1 << _HVMPV_base_freq)
+
+/* Feature set modifications */
+
+/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
+ * HV_X64_MSR_APIC_FREQUENCY).
+ * This modification restores the viridian feature set to the
+ * original 'base' set exposed in releases prior to Xen 4.4.
+ */
+#define _HVMPV_no_freq 1
+#define HVMPV_no_freq (1 << _HVMPV_no_freq)
+
+#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
+
#endif
/*
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest
2014-08-04 13:12 [PATCH v2 0/3] x86/viridian improvements Paul Durrant
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
@ 2014-08-04 13:12 ` Paul Durrant
2014-08-04 13:38 ` Andrew Cooper
2014-08-04 13:12 ` [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant
2 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 13:12 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant, Keir Fraser, Jan Beulich
All viridian logging is actually in response to an action (MSR read/write,
etc.) in the guest. As such XENLOG_G_ log levels should be used.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/hvm/viridian.c | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 31c9656..2d79403 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -120,36 +120,36 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
static void dump_guest_os_id(const struct domain *d)
{
- gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
- gdprintk(XENLOG_INFO, "\tvendor: %x\n",
+ gdprintk(XENLOG_G_INFO, "GUEST_OS_ID:\n");
+ gdprintk(XENLOG_G_INFO, "\tvendor: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.vendor);
- gdprintk(XENLOG_INFO, "\tos: %x\n",
+ gdprintk(XENLOG_G_INFO, "\tos: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.os);
- gdprintk(XENLOG_INFO, "\tmajor: %x\n",
+ gdprintk(XENLOG_G_INFO, "\tmajor: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.major);
- gdprintk(XENLOG_INFO, "\tminor: %x\n",
+ gdprintk(XENLOG_G_INFO, "\tminor: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.minor);
- gdprintk(XENLOG_INFO, "\tsp: %x\n",
+ gdprintk(XENLOG_G_INFO, "\tsp: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.service_pack);
- gdprintk(XENLOG_INFO, "\tbuild: %x\n",
+ gdprintk(XENLOG_G_INFO, "\tbuild: %x\n",
d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
}
static void dump_hypercall(const struct domain *d)
{
- gdprintk(XENLOG_INFO, "HYPERCALL:\n");
- gdprintk(XENLOG_INFO, "\tenabled: %x\n",
+ gdprintk(XENLOG_G_INFO, "HYPERCALL:\n");
+ gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
d->arch.hvm_domain.viridian.hypercall_gpa.fields.enabled);
- gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
+ gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
(unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
}
static void dump_apic_assist(const struct vcpu *v)
{
- gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
- gdprintk(XENLOG_INFO, "\tenabled: %x\n",
+ gdprintk(XENLOG_G_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
+ gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
v->arch.hvm_vcpu.viridian.apic_assist.fields.enabled);
- gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
+ gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
(unsigned long)v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn);
}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 13:12 [PATCH v2 0/3] x86/viridian improvements Paul Durrant
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
2014-08-04 13:12 ` [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest Paul Durrant
@ 2014-08-04 13:12 ` Paul Durrant
2014-08-04 14:01 ` Jan Beulich
2 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 13:12 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Ian Campbell, Stefano Stabellini, Ian Jackson,
Paul Durrant, Jan Beulich
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.
The counter value used in this implementation properly fulfils the
documented semantics of the MSR.
The per-domain viridian save record is extended to preserve the reference
counter across save/restore. For compatibility therefore, the restore
zero extends any older save record. This is safe as, if the reference
counter substructure is not present, it cannot possibly have been in use
by the guest prior to the domain save and thus will not be accessed after
domain restore.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Ian Campbell <ian.campbell@citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
docs/man/xl.cfg.pod.5 | 7 ++++
tools/libxl/libxl_dom.c | 2 +
xen/arch/x86/hvm/hvm.c | 3 ++
xen/arch/x86/hvm/viridian.c | 69 +++++++++++++++++++++++++++-----
xen/include/asm-x86/hvm/viridian.h | 22 ++++++++++
xen/include/asm-x86/perfc_defn.h | 1 +
xen/include/public/arch-x86/hvm/save.h | 11 +++++
xen/include/public/hvm/params.h | 9 ++++-
8 files changed, 113 insertions(+), 11 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
index a05e831..e4692982 100644
--- a/docs/man/xl.cfg.pod.5
+++ b/docs/man/xl.cfg.pod.5
@@ -1109,6 +1109,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 a15c185..1ebdecd 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -222,6 +222,8 @@ static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
feature_mask |= HVMPV_base_freq;
else if (strcmp(*p, "freq") == 0)
feature_mask &= ~HVMPV_no_freq;
+ else if (strcmp(*p, "time-ref-count") == 0)
+ feature_mask |= HVMPV_time_ref_count;
p++;
}
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 04d43d0..69dec3e 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5548,6 +5548,9 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
!(a.value & HVMPV_base_freq) )
break;
+ if ( !d->arch.hvm_domain.params[a.index] )
+ initialize_viridian(d, a.value);
+
rc = 0;
break;
case HVM_PARAM_IDENT_PT:
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 2d79403..3f9d4ae 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)
@@ -51,6 +51,43 @@
#define CPUID6A_MSR_BITMAPS (1 << 1)
#define CPUID6A_NESTED_PAGING (1 << 3)
+static void initialize_time_ref_count(struct domain *d)
+{
+ struct viridian_time_ref_count *trc;
+
+ trc = &d->arch.hvm_domain.viridian.time_ref_count;
+
+ spin_lock_init(&trc->lock);
+}
+
+static s_time_t get_time_ref_count(struct domain *d)
+{
+ s_time_t now = get_s_time() / 100;
+ struct viridian_time_ref_count *trc;
+
+ trc = &d->arch.hvm_domain.viridian.time_ref_count;
+
+ spin_lock(&trc->lock);
+
+ if ( trc->last == 0 )
+ gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER accessed\n");
+
+ if ( (now - trc->last) > 0 )
+ trc->last = now;
+ else
+ now = ++trc->last;
+
+ spin_unlock(&trc->lock);
+
+ return now;
+}
+
+void initialize_viridian(struct domain *d, uint64_t feature_mask)
+{
+ if ( feature_mask & HVMPV_time_ref_count )
+ initialize_time_ref_count(d);
+}
+
int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
unsigned int *ebx, unsigned int *ecx,
unsigned int *edx)
@@ -93,6 +130,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 +383,14 @@ 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:
+ if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
+ return 0;
+
+ perfc_incr(mshv_rdmsr_time_ref_count);
+ *val = (uint64_t)get_time_ref_count(d);
+ break;
+
default:
return 0;
}
@@ -431,8 +478,9 @@ static int viridian_save_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
if ( !is_viridian_domain(d) )
return 0;
- ctxt.hypercall_gpa = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
- ctxt.guest_os_id = d->arch.hvm_domain.viridian.guest_os_id.raw;
+ ctxt.hypercall_gpa = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
+ ctxt.guest_os_id = d->arch.hvm_domain.viridian.guest_os_id.raw;
+ ctxt.time_ref_count = d->arch.hvm_domain.viridian.time_ref_count.last;
return (hvm_save_entry(VIRIDIAN_DOMAIN, 0, h, &ctxt) != 0);
}
@@ -441,11 +489,12 @@ static int viridian_load_domain_ctxt(struct domain *d, hvm_domain_context_t *h)
{
struct hvm_viridian_domain_context ctxt;
- if ( hvm_load_entry(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
+ if ( hvm_load_entry_zeroextend(VIRIDIAN_DOMAIN, h, &ctxt) != 0 )
return -EINVAL;
- d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
- d->arch.hvm_domain.viridian.guest_os_id.raw = ctxt.guest_os_id;
+ d->arch.hvm_domain.viridian.hypercall_gpa.raw = ctxt.hypercall_gpa;
+ d->arch.hvm_domain.viridian.guest_os_id.raw = ctxt.guest_os_id;
+ d->arch.hvm_domain.viridian.time_ref_count.last = ctxt.time_ref_count;
return 0;
}
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 496da33..9565eea 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -48,12 +48,24 @@ union viridian_hypercall_gpa
} fields;
};
+struct viridian_time_ref_count
+{
+ spinlock_t lock;
+ s_time_t last;
+};
+
struct viridian_domain
{
union viridian_guest_os_id guest_os_id;
union viridian_hypercall_gpa hypercall_gpa;
+ struct viridian_time_ref_count time_ref_count;
};
+void
+initialize_viridian(
+ struct domain *d,
+ uint64_t feature_mask);
+
int
cpuid_viridian_leaves(
unsigned int leaf,
@@ -76,3 +88,13 @@ int
viridian_hypercall(struct cpu_user_regs *regs);
#endif /* __ASM_X86_HVM_VIRIDIAN_H__ */
+
+/*
+ * 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/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/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 16d85a3..d1fd11b 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -568,6 +568,7 @@ struct hvm_hw_cpu_xsave {
struct hvm_viridian_domain_context {
uint64_t hypercall_gpa;
uint64_t guest_os_id;
+ uint64_t time_ref_count;
};
DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context);
@@ -616,3 +617,13 @@ struct hvm_msr {
#define HVM_SAVE_CODE_MAX 20
#endif /* __XEN_PUBLIC_HVM_SAVE_X86_H__ */
+
+/*
+ * 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/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] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
@ 2014-08-04 13:31 ` Andrew Cooper
2014-08-04 13:50 ` Jan Beulich
2014-08-04 17:19 ` Paul Durrant
2014-08-04 13:45 ` Jan Beulich
1 sibling, 2 replies; 18+ messages in thread
From: Andrew Cooper @ 2014-08-04 13:31 UTC (permalink / raw)
To: Paul Durrant, xen-devel
Cc: Ian Jackson, Keir Fraser, Ian Campbell, Jan Beulich, Stefano Stabellini
On 04/08/14 14:12, Paul Durrant wrote:
> The following commits introduced the time reference counter MSR and
> TSC/APIC frequency MSRs into the viridian feature set respectively:
>
> e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b
> 84657efd9116f40924aa13c9d5a349e007da716f
>
> The time reference counter MSR feature was then reverted by commit
>
> 1cd4fab14ce25859efa4a2af13475e6650a5506c
>
> because a flaw in the implementation meant the counter was reset on
> migration.
>
> All of these changes were made without any addtional options being
> added to the VM configuration, or any compatibility checks being made
> in the domain save/restore code. Hence setting the single boolean
> 'viridian' option in the VM configuration yields a different set of
> features depending on which version of Xen the VM is started on, and the
> feature set can change across migration (so new MSRs can magically appear).
>
> This patch grandfathers in the current viridian features set and calls them
> the 'base' plus 'freq' feature set. HVM_PARAM_VIRIDIAN is re-purposed as
> a feature mask. It has only ever been legally set to 0 or 1, so the presence
> of the base set and freq set are indicated by setting bit 0. The 'freq' set
> can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4
> 'base' set. Newly implemented viridian features can be optionally enabled
> in future by setting further bits.
>
> The viridian option in xl.cfg(5) has also been changed to a string list so
> that the sets can be individually sepcified. For compatibility, if the
> option is specified as a boolean, then a true (1) value will be translated
> to a string list containing "base" and "freq".
>
> This patch also alters the allowed write accesses to HVM_PARAM_VIRIDIAN.
> Currently there is nothing to stop the guest writing this value (which,
> while harmless to anything else, should not happen) and nothing to
> stop a toolstack from setting the value back to zero whilst the guest is
> running, causing CPUID leaves to disappear and MSR accesses to start
> causing GPFs in the guest. Both of these possibilities are now disallowed:
> Once the parameter is set to a non-zero value it may not be cleared, and
> guests no longer have any write access.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Ian Campbell <ian.campbell@citrix.com>
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> ---
> docs/man/xl.cfg.pod.5 | 53 ++++++++++++++++++++++++++++++---------
> tools/libxc/xc_domain_restore.c | 4 +--
> tools/libxl/libxl.h | 6 +++++
> tools/libxl/libxl_create.c | 1 -
> tools/libxl/libxl_dom.c | 46 ++++++++++++++++++++++++++-------
> tools/libxl/libxl_types.idl | 2 +-
> tools/libxl/xl_cmdimpl.c | 24 +++++++++++++++++-
> tools/libxl/xl_sxp.c | 8 ++++--
> xen/arch/x86/hvm/hvm.c | 18 +++++++++++--
> xen/arch/x86/hvm/viridian.c | 21 ++++++++++++++--
> xen/include/asm-x86/hvm/hvm.h | 7 ++++--
> xen/include/public/hvm/params.h | 33 +++++++++++++++++++++++-
> 12 files changed, 188 insertions(+), 35 deletions(-)
>
> diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> index ff9ea77..a05e831 100644
> --- a/docs/man/xl.cfg.pod.5
> +++ b/docs/man/xl.cfg.pod.5
> @@ -1084,18 +1084,47 @@ Windows L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
> Setting B<xen_platform_pci=0> with the default device_model "qemu-xen"
> requires at least QEMU 1.6.
>
> -=item B<viridian=BOOLEAN>
> -
> -Turns on or off the exposure of MicroSoft Hyper-V (AKA viridian)
> -compatible enlightenments to the guest. These can improve performance
> -of Microsoft Windows guests from Windows Vista and Windows 2008
> -onwards and setting this option for such guests is strongly
> -recommended. This option should be harmless for other versions of
> -Windows (although it will not give any benefit) and the majority of
> -other non-Windows OSes. However it is known to be incompatible with
> -some other Operating Systems and in some circumstance can prevent
> -Xen's own paravirtualisation interfaces for HVM guests from being
> -used.
> +=item B<viridian=[ "SET", "SET", ...]>
> +
> +The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> +exposed to the guest. The following sets of enlightenments may be
> +specified:
> +
> +=over 4
> +
> +=item B<base>
> +
> +This set incorporates the Hypercall MSRs, Virtual processor index MSR,
> +and APIC access MSRs. This set is a pre-requisite for all other sets.
> +If it is not specified then all enlightenments will be disabled.
> +
> +These enlightenments can improve performance of Windows Vista and Windows
> +Server 2008 onwards and setting this option for such guests is strongly
> +recommended.
> +
> +=item B<freq>
> +
> +This set incorporates the TSC and APIC frequency MSRs.
> +
> +This enlightenment can improve performance of Windows 7 and Windows
> +Server 2008 R2 onwards.
> +
> +=back
> +
> +See the latest version of Microsoft's Hypervisor Top-Level Functional
> +Specification for more details.
> +
> +The enlightenments should be harmless for other versions of Windows
> +(although they will not give any benefit) and the majority of other
> +non-Windows OSes.
> +However it is known that they are incompatible with some other Operating
> +Systems and in some circumstance can prevent Xen's own paravirtualisation
> +interfaces for HVM guests from being used.
> +
> +Specifying the viridian option as a boolean is deprecated. However, for
> +backwards compatibility, if it is specified as a boolean a value of
> +true (1) will cause both the 'base' and 'freq' sets to be exposed to
> +the guest, and a value of false (0) will disable all enlightenments.
>
> =back
>
> diff --git a/tools/libxc/xc_domain_restore.c b/tools/libxc/xc_domain_restore.c
> index e73e0a2..5e7e62d 100644
> --- a/tools/libxc/xc_domain_restore.c
> +++ b/tools/libxc/xc_domain_restore.c
> @@ -922,7 +922,7 @@ static int pagebuf_get_one(xc_interface *xch, struct restore_ctx *ctx,
> if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> {
> - PERROR("error read the viridian flag");
> + PERROR("error read the viridian flags");
> return -1;
> }
> return pagebuf_get_one(xch, ctx, buf, fd, dom);
> @@ -1733,7 +1733,7 @@ int xc_domain_restore(xc_interface *xch, int io_fd, uint32_t dom,
> }
>
> if (pagebuf.viridian != 0)
> - xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> + xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, pagebuf.viridian);
>
> /*
> * If we are migrating in from a host that does not support
> diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> index 5ae6532..b985f49 100644
> --- a/tools/libxl/libxl.h
> +++ b/tools/libxl/libxl.h
> @@ -128,6 +128,12 @@
> #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
>
> /*
> + * The libxl_domain_build_info u.hvm.viridian field is a string list
> + */
> +#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_STRING_LIST 1
> +
> +
> +/*
> * libxl ABI compatibility
> *
> * The only guarantee which libxl makes regarding ABI compatibility
> diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> index 0686f96..ee19fc8 100644
> --- a/tools/libxl/libxl_create.c
> +++ b/tools/libxl/libxl_create.c
> @@ -273,7 +273,6 @@ int libxl__domain_build_info_setdefault(libxl__gc *gc,
> libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3, true);
> libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4, true);
> libxl_defbool_setdefault(&b_info->u.hvm.nx, true);
> - libxl_defbool_setdefault(&b_info->u.hvm.viridian, false);
> libxl_defbool_setdefault(&b_info->u.hvm.hpet, true);
> libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
> libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm, false);
> diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> index 83eb29a..a15c185 100644
> --- a/tools/libxl/libxl_dom.c
> +++ b/tools/libxl/libxl_dom.c
> @@ -209,21 +209,49 @@ static unsigned long timer_mode(const libxl_domain_build_info *info)
> return ((unsigned long)mode);
> }
>
> -static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
> +#if defined(__i386__) || defined(__x86_64__)
> +static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
> + libxl_domain_build_info *const info)
> +{
> + uint64_t feature_mask = HVMPV_no_freq;
> +
> + char **p = info->u.hvm.viridian;
> + while (*p) {
> + LOG(DETAIL, "+%s", *p);
> + if (strcmp(*p, "base") == 0)
> + feature_mask |= HVMPV_base_freq;
> + else if (strcmp(*p, "freq") == 0)
> + feature_mask &= ~HVMPV_no_freq;
> + p++;
> + }
> +
> + if (xc_hvm_param_set(CTX->xch,
> + domid,
> + HVM_PARAM_VIRIDIAN,
> + feature_mask) != 0)
> + LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_WARNING,
> + "Couldn't set viridian feature mask (0x%lx)",
PRIx64, or this will break 32bit builds.
> + feature_mask);
> +}
> +#endif
> +
> +static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
> libxl_domain_build_info *const info)
const libxl_domain_build_info *info ?
> {
> - xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
> + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED,
> libxl_defbool_val(info->u.hvm.pae));
> #if defined(__i386__) || defined(__x86_64__)
> - xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN,
> - libxl_defbool_val(info->u.hvm.viridian));
> - xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
> + if (libxl_string_list_length(&info->u.hvm.viridian) != 0)
> + hvm_set_viridian_features(gc, domid, info);
> +
> + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_HPET_ENABLED,
> libxl_defbool_val(info->u.hvm.hpet));
> #endif
> - xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE, timer_mode(info));
> - xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
> + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_TIMER_MODE,
> + timer_mode(info));
> + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_VPT_ALIGN,
> libxl_defbool_val(info->u.hvm.vpt_align));
> - xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
> + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_NESTEDHVM,
> libxl_defbool_val(info->u.hvm.nested_hvm));
> }
>
> @@ -306,7 +334,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid, state->console_domid);
>
> if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> - hvm_set_conf_params(ctx->xch, domid, info);
> + hvm_set_conf_params(gc, domid, info);
>
> rc = libxl__arch_domain_create(gc, d_config, domid);
>
> diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> index a412f9c..6c5d7e0 100644
> --- a/tools/libxl/libxl_types.idl
> +++ b/tools/libxl/libxl_types.idl
> @@ -363,7 +363,7 @@ libxl_domain_build_info = Struct("domain_build_info",[
> ("acpi_s3", libxl_defbool),
> ("acpi_s4", libxl_defbool),
> ("nx", libxl_defbool),
> - ("viridian", libxl_defbool),
> + ("viridian", libxl_string_list),
> ("timeoffset", string),
> ("hpet", libxl_defbool),
> ("vpt_align", libxl_defbool),
> diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> index 01bce2f..960f626 100644
> --- a/tools/libxl/xl_cmdimpl.c
> +++ b/tools/libxl/xl_cmdimpl.c
> @@ -951,10 +951,32 @@ static void parse_config_data(const char *config_source,
> xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
> xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
> xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
> - xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 0);
> xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
> xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align, 0);
>
> + switch (xlu_cfg_get_list_as_string_list(config, "viridian",
> + &b_info->u.hvm.viridian, 1))
> + {
> + case 0: break; /* Success */
> + case ESRCH: break; /* Option not present */
> + case EINVAL: {
> + libxl_defbool b;
> +
> + xlu_cfg_get_defbool(config, "viridian", &b, 1);
> + if (libxl_defbool_val(b)) {
> + fprintf(stderr, "WARNING: Specifying \"viridian\""
> + " as a boolean is deprecated. "
> + "Please use a list of features.\n");
> + split_string_into_string_list("base freq", " \n",
> + &b_info->u.hvm.viridian);
> + }
> + break;
> + }
> + default:
> + fprintf(stderr,"xl: Unable to parse bootloader_args.\n");
> + exit(-ERROR_FAIL);
> + }
> +
> if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
> const char *s = libxl_timer_mode_to_string(l);
> fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer is deprecated. "
> diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
> index 48eb608..347526f 100644
> --- a/tools/libxl/xl_sxp.c
> +++ b/tools/libxl/xl_sxp.c
> @@ -97,8 +97,12 @@ void printf_info_sexp(int domid, libxl_domain_config *d_config)
> printf("\t\t\t(acpi %s)\n",
> libxl_defbool_to_string(b_info->u.hvm.acpi));
> printf("\t\t\t(nx %s)\n", libxl_defbool_to_string(b_info->u.hvm.nx));
> - printf("\t\t\t(viridian %s)\n",
> - libxl_defbool_to_string(b_info->u.hvm.viridian));
> + if (b_info->u.hvm.viridian) {
> + printf("\t\t\t(viridian");
> + for (i=0; b_info->u.hvm.viridian[i]; i++)
> + printf(" %s", b_info->u.hvm.viridian[i]);
> + printf(")\n");
> + }
> printf("\t\t\t(hpet %s)\n",
> libxl_defbool_to_string(b_info->u.hvm.hpet));
> printf("\t\t\t(vpt_align %s)\n",
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index fba13e0..04d43d0 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> rc = -EINVAL;
> break;
> case HVM_PARAM_VIRIDIAN:
> - if ( a.value > 1 )
> - rc = -EINVAL;
> + /* This should only ever be set once by the tools and read by the guest. */
> + rc = -EPERM;
> + if ( curr_d == d )
> + break;
> +
> + rc = -EPERM;
> + if ( d->arch.hvm_domain.params[a.index] &&
> + a.value != d->arch.hvm_domain.params[a.index] )
> + break;
Setting it twice should be an error, even if it is set to the same value
again. Also, EEXIST would be a better error.
> +
> + rc = -EINVAL;
> + if ( a.value & ~HVMPV_feature_mask ||
> + !(a.value & HVMPV_base_freq) )
> + break;
> +
> + rc = 0;
To save bloating the code here if/when new combinations are introduced,
would it be better to have a "rc = viridian_initialise(d, a.value);"
which validates the featureset and sets it if necessary. The above
EEXISTs check could be included as well.
> break;
> case HVM_PARAM_IDENT_PT:
> /* Not reflexive, as we must domain_pause(). */
> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> index 0fcbfd8..31c9656 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
> /* Which hypervisor MSRs are available to the guest */
> *eax = (CPUID3A_MSR_APIC_ACCESS |
> CPUID3A_MSR_HYPERCALL |
> - CPUID3A_MSR_VP_INDEX |
> - CPUID3A_MSR_FREQ);
> + CPUID3A_MSR_VP_INDEX);
> + if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> + *eax |= CPUID3A_MSR_FREQ;
> break;
> case 4:
> /* Recommended hypercall usage. */
> @@ -312,11 +313,17 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> break;
>
> case VIRIDIAN_MSR_TSC_FREQUENCY:
> + if ( viridian_feature_mask(d) & HVMPV_no_freq )
> + return 0;
> +
> perfc_incr(mshv_rdmsr_tsc_frequency);
> *val = (uint64_t)d->arch.tsc_khz * 1000ull;
> break;
>
> case VIRIDIAN_MSR_APIC_FREQUENCY:
> + if ( viridian_feature_mask(d) & HVMPV_no_freq )
> + return 0;
> +
> perfc_incr(mshv_rdmsr_apic_frequency);
> *val = 1000000000ull / APIC_BUS_CYCLE_NS;
> break;
> @@ -489,3 +496,13 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
>
> HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU, viridian_save_vcpu_ctxt,
> viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-x86/hvm/hvm.h
> index 0ebd478..24e513b 100644
> --- a/xen/include/asm-x86/hvm/hvm.h
> +++ b/xen/include/asm-x86/hvm/hvm.h
> @@ -344,8 +344,11 @@ static inline unsigned long hvm_get_shadow_gs_base(struct vcpu *v)
> return hvm_funcs.get_shadow_gs_base(v);
> }
>
> -#define is_viridian_domain(_d) \
> - (is_hvm_domain(_d) && ((_d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
> +#define viridian_feature_mask(d) \
> + ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> +
> +#define is_viridian_domain(d) \
> + (is_hvm_domain(d) && (viridian_feature_mask(d) & HVMPV_base_freq))
>
> void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
> uint32_t *eax, uint32_t *ebx,
> diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
> index 614ff5f..68d26fd 100644
> --- a/xen/include/public/hvm/params.h
> +++ b/xen/include/public/hvm/params.h
> @@ -56,9 +56,40 @@
>
> #if defined(__i386__) || defined(__x86_64__)
>
> -/* Expose Viridian interfaces to this HVM guest? */
> +/*
> + * Viridian enlightenments
> + *
> + * (See http://download.microsoft.com/download/A/B/4/AB43A34E-BDD0-4FA6-BDEF-79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%20v4.0.docx)
> + *
> + * To expose viridian enlightenments to the guest set this parameter
> + * to the desired feature mask. The base feature set must be present
> + * in any valid feature mask.
> + */
> #define HVM_PARAM_VIRIDIAN 9
>
> +/* Base+Freq viridian feature sets:
> + *
> + * - Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and HV_X64_MSR_HYPERCALL)
> + * - APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and HV_X64_MSR_TPR)
> + * - Virtual Processor index MSR (HV_X64_MSR_VP_INDEX)
> + * - Timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> + * HV_X64_MSR_APIC_FREQUENCY)
> + */
> +#define _HVMPV_base_freq 0
> +#define HVMPV_base_freq (1 << _HVMPV_base_freq)
> +
> +/* Feature set modifications */
> +
> +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> + * HV_X64_MSR_APIC_FREQUENCY).
> + * This modification restores the viridian feature set to the
> + * original 'base' set exposed in releases prior to Xen 4.4.
> + */
> +#define _HVMPV_no_freq 1
> +#define HVMPV_no_freq (1 << _HVMPV_no_freq)
> +
> +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
Please could we avoid introducing new negated-sense booleans, especially
when their predominant use is "if ( !$FOO_no_BAR )"
~Andrew
> +
> #endif
>
> /*
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest
2014-08-04 13:12 ` [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest Paul Durrant
@ 2014-08-04 13:38 ` Andrew Cooper
2014-08-04 13:45 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2014-08-04 13:38 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Keir Fraser, Jan Beulich
On 04/08/14 14:12, Paul Durrant wrote:
> All viridian logging is actually in response to an action (MSR read/write,
> etc.) in the guest. As such XENLOG_G_ log levels should be used.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
gdprintk() already makes them guest log levels, so this change is a
functional noop.
However, the __file__/__LINE__ references are quite useless in the
presented information, so can safely be dropped by switching to regular
printk(). It would also be nice to reduce the number of individual
lines requires. dump_guest_os_id could easily be one or two lines
instead of 7.
~Andrew
> ---
> xen/arch/x86/hvm/viridian.c | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> index 31c9656..2d79403 100644
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -120,36 +120,36 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax,
>
> static void dump_guest_os_id(const struct domain *d)
> {
> - gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
> - gdprintk(XENLOG_INFO, "\tvendor: %x\n",
> + gdprintk(XENLOG_G_INFO, "GUEST_OS_ID:\n");
> + gdprintk(XENLOG_G_INFO, "\tvendor: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.vendor);
> - gdprintk(XENLOG_INFO, "\tos: %x\n",
> + gdprintk(XENLOG_G_INFO, "\tos: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.os);
> - gdprintk(XENLOG_INFO, "\tmajor: %x\n",
> + gdprintk(XENLOG_G_INFO, "\tmajor: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.major);
> - gdprintk(XENLOG_INFO, "\tminor: %x\n",
> + gdprintk(XENLOG_G_INFO, "\tminor: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.minor);
> - gdprintk(XENLOG_INFO, "\tsp: %x\n",
> + gdprintk(XENLOG_G_INFO, "\tsp: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.service_pack);
> - gdprintk(XENLOG_INFO, "\tbuild: %x\n",
> + gdprintk(XENLOG_G_INFO, "\tbuild: %x\n",
> d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
> }
>
> static void dump_hypercall(const struct domain *d)
> {
> - gdprintk(XENLOG_INFO, "HYPERCALL:\n");
> - gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> + gdprintk(XENLOG_G_INFO, "HYPERCALL:\n");
> + gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
> d->arch.hvm_domain.viridian.hypercall_gpa.fields.enabled);
> - gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> + gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
> (unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
> }
>
> static void dump_apic_assist(const struct vcpu *v)
> {
> - gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
> - gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> + gdprintk(XENLOG_G_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
> + gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
> v->arch.hvm_vcpu.viridian.apic_assist.fields.enabled);
> - gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> + gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
> (unsigned long)v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn);
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
2014-08-04 13:31 ` Andrew Cooper
@ 2014-08-04 13:45 ` Jan Beulich
1 sibling, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-08-04 13:45 UTC (permalink / raw)
To: Paul Durrant
Cc: Keir Fraser, xen-devel, IanJackson, Ian Campbell, Stefano Stabellini
>>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
> rc = -EINVAL;
> break;
> case HVM_PARAM_VIRIDIAN:
> - if ( a.value > 1 )
> - rc = -EINVAL;
> + /* This should only ever be set once by the tools and read by the guest. */
> + rc = -EPERM;
> + if ( curr_d == d )
> + break;
> +
> + rc = -EPERM;
> + if ( d->arch.hvm_domain.params[a.index] &&
> + a.value != d->arch.hvm_domain.params[a.index] )
I was actually expecting this to succeed if the value is the same as the
currently stored one, rather than fail when they're different. The
subtle difference is that this would allow the tools to set the value to
zero when it currently is zero, which the check below would otherwise
refuse afaict.
Apart from that, if the code was left as is the second of the -EPERM
assignments would be redundant.
> + break;
> +
There is still a line with only blanks here.
> + rc = -EINVAL;
> + if ( a.value & ~HVMPV_feature_mask ||
We commonly parenthesize such expressions when combined with
&& or ||.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest
2014-08-04 13:38 ` Andrew Cooper
@ 2014-08-04 13:45 ` Paul Durrant
0 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 13:45 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Keir (Xen.org), Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 04 August 2014 14:38
> To: Paul Durrant; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v2 2/3] x86/viridian: Note that logging is
> under control of the guest
>
> On 04/08/14 14:12, Paul Durrant wrote:
> > All viridian logging is actually in response to an action (MSR read/write,
> > etc.) in the guest. As such XENLOG_G_ log levels should be used.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
>
> gdprintk() already makes them guest log levels, so this change is a
> functional noop.
Ah, good point. Just me being overly paranoid.
>
> However, the __file__/__LINE__ references are quite useless in the
> presented information, so can safely be dropped by switching to regular
> printk(). It would also be nice to reduce the number of individual
> lines requires. dump_guest_os_id could easily be one or two lines
> instead of 7.
>
Yeah; it's a bit verbose.
Paul
> ~Andrew
>
> > ---
> > xen/arch/x86/hvm/viridian.c | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> > index 31c9656..2d79403 100644
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -120,36 +120,36 @@ int cpuid_viridian_leaves(unsigned int leaf,
> unsigned int *eax,
> >
> > static void dump_guest_os_id(const struct domain *d)
> > {
> > - gdprintk(XENLOG_INFO, "GUEST_OS_ID:\n");
> > - gdprintk(XENLOG_INFO, "\tvendor: %x\n",
> > + gdprintk(XENLOG_G_INFO, "GUEST_OS_ID:\n");
> > + gdprintk(XENLOG_G_INFO, "\tvendor: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.vendor);
> > - gdprintk(XENLOG_INFO, "\tos: %x\n",
> > + gdprintk(XENLOG_G_INFO, "\tos: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.os);
> > - gdprintk(XENLOG_INFO, "\tmajor: %x\n",
> > + gdprintk(XENLOG_G_INFO, "\tmajor: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.major);
> > - gdprintk(XENLOG_INFO, "\tminor: %x\n",
> > + gdprintk(XENLOG_G_INFO, "\tminor: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.minor);
> > - gdprintk(XENLOG_INFO, "\tsp: %x\n",
> > + gdprintk(XENLOG_G_INFO, "\tsp: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.service_pack);
> > - gdprintk(XENLOG_INFO, "\tbuild: %x\n",
> > + gdprintk(XENLOG_G_INFO, "\tbuild: %x\n",
> > d->arch.hvm_domain.viridian.guest_os_id.fields.build_number);
> > }
> >
> > static void dump_hypercall(const struct domain *d)
> > {
> > - gdprintk(XENLOG_INFO, "HYPERCALL:\n");
> > - gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> > + gdprintk(XENLOG_G_INFO, "HYPERCALL:\n");
> > + gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
> > d->arch.hvm_domain.viridian.hypercall_gpa.fields.enabled);
> > - gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> > + gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
> > (unsigned long)d-
> >arch.hvm_domain.viridian.hypercall_gpa.fields.pfn);
> > }
> >
> > static void dump_apic_assist(const struct vcpu *v)
> > {
> > - gdprintk(XENLOG_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
> > - gdprintk(XENLOG_INFO, "\tenabled: %x\n",
> > + gdprintk(XENLOG_G_INFO, "APIC_ASSIST[%d]:\n", v->vcpu_id);
> > + gdprintk(XENLOG_G_INFO, "\tenabled: %x\n",
> > v->arch.hvm_vcpu.viridian.apic_assist.fields.enabled);
> > - gdprintk(XENLOG_INFO, "\tpfn: %lx\n",
> > + gdprintk(XENLOG_G_INFO, "\tpfn: %lx\n",
> > (unsigned long)v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn);
> > }
> >
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:31 ` Andrew Cooper
@ 2014-08-04 13:50 ` Jan Beulich
2014-08-04 14:50 ` Andrew Cooper
2014-08-04 17:19 ` Paul Durrant
1 sibling, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2014-08-04 13:50 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant
Cc: Keir Fraser, xen-devel, Ian Jackson, Ian Campbell, Stefano Stabellini
>>> On 04.08.14 at 15:31, <andrew.cooper3@citrix.com> wrote:
> On 04/08/14 14:12, Paul Durrant wrote:
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>> rc = -EINVAL;
>> break;
>> case HVM_PARAM_VIRIDIAN:
>> - if ( a.value > 1 )
>> - rc = -EINVAL;
>> + /* This should only ever be set once by the tools and read by the guest. */
>> + rc = -EPERM;
>> + if ( curr_d == d )
>> + break;
>> +
>> + rc = -EPERM;
>> + if ( d->arch.hvm_domain.params[a.index] &&
>> + a.value != d->arch.hvm_domain.params[a.index] )
>> + break;
>
> Setting it twice should be an error, even if it is set to the same value
> again.
I specifically asked for it to be done this way, such that redundant
calls wouldn't needlessly fail. Remember that we're altering an
existing interface, and hence should be careful about breaking
existing callers.
> Also, EEXIST would be a better error.
I agree with this part.
>> +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
>> + * HV_X64_MSR_APIC_FREQUENCY).
>> + * This modification restores the viridian feature set to the
>> + * original 'base' set exposed in releases prior to Xen 4.4.
>> + */
>> +#define _HVMPV_no_freq 1
>> +#define HVMPV_no_freq (1 << _HVMPV_no_freq)
>> +
>> +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
>
> Please could we avoid introducing new negated-sense booleans, especially
> when their predominant use is "if ( !$FOO_no_BAR )"
What would your alternative proposal be, again keeping in mind that
existing callers must not observe a behavioral change?
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 13:12 ` [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant
@ 2014-08-04 14:01 ` Jan Beulich
2014-08-04 14:12 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2014-08-04 14:01 UTC (permalink / raw)
To: Paul Durrant
Cc: Keir Fraser, xen-devel, IanJackson, Ian Campbell, Stefano Stabellini
>>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> +static s_time_t get_time_ref_count(struct domain *d)
> +{
> + s_time_t now = get_s_time() / 100;
> + struct viridian_time_ref_count *trc;
> +
> + trc = &d->arch.hvm_domain.viridian.time_ref_count;
> +
> + spin_lock(&trc->lock);
How well do you think this will scale, taking into consideration our
recent observations with certain Windows versions having all its
CPUs race for time stamps over apparently extended periods of
time? It would seem to me that a cmpxchg() based approach
might be preferable here.
> +
> + if ( trc->last == 0 )
> + gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER accessed\n");
As per the comments on patch 2, this needs to be either printk() or
the _G_ in the middle converted to just _.
> +
> + if ( (now - trc->last) > 0 )
> + trc->last = now;
> + else
> + now = ++trc->last;
And how well is that going to work (and match Windows'
expectations) when the migration destination host's NOW() is
significantly smaller than the source host's?
> @@ -344,6 +383,14 @@ 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:
> + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> + return 0;
> +
> + perfc_incr(mshv_rdmsr_time_ref_count);
> + *val = (uint64_t)get_time_ref_count(d);
Pointless cast.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 14:01 ` Jan Beulich
@ 2014-08-04 14:12 ` Paul Durrant
2014-08-04 14:47 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 14:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Jackson, Keir (Xen.org), Stefano Stabellini, Ian Campbell, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 04 August 2014 15:01
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@lists.xen.org;
> Keir (Xen.org)
> Subject: Re: [PATCH v2 3/3] x86/viridian: Add partition time reference
> counter MSR support
>
> >>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> > +static s_time_t get_time_ref_count(struct domain *d)
> > +{
> > + s_time_t now = get_s_time() / 100;
> > + struct viridian_time_ref_count *trc;
> > +
> > + trc = &d->arch.hvm_domain.viridian.time_ref_count;
> > +
> > + spin_lock(&trc->lock);
>
> How well do you think this will scale, taking into consideration our
> recent observations with certain Windows versions having all its
> CPUs race for time stamps over apparently extended periods of
> time? It would seem to me that a cmpxchg() based approach
> might be preferable here.
Yes, that may well be true.
>
> > +
> > + if ( trc->last == 0 )
> > + gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER
> accessed\n");
>
> As per the comments on patch 2, this needs to be either printk() or
> the _G_ in the middle converted to just _.
>
Ok.
> > +
> > + if ( (now - trc->last) > 0 )
> > + trc->last = now;
> > + else
> > + now = ++trc->last;
>
> And how well is that going to work (and match Windows'
> expectations) when the migration destination host's NOW() is
> significantly smaller than the source host's?
>
I misunderstood what get_s_time() is returning so I think you're correct that this will not work will in that case. It's basically the same as the PV rdtsc code; but maybe that won't work either.
Paul
> > @@ -344,6 +383,14 @@ 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:
> > + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> > + return 0;
> > +
> > + perfc_incr(mshv_rdmsr_time_ref_count);
> > + *val = (uint64_t)get_time_ref_count(d);
>
> Pointless cast.
>
> Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 14:12 ` Paul Durrant
@ 2014-08-04 14:47 ` Paul Durrant
2014-08-04 15:11 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 14:47 UTC (permalink / raw)
To: Paul Durrant, Jan Beulich
Cc: Ian Jackson, Keir (Xen.org), Ian Campbell, Stefano Stabellini, xen-devel
> -----Original Message-----
> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> bounces@lists.xen.org] On Behalf Of Paul Durrant
> Sent: 04 August 2014 15:12
> To: Jan Beulich
> Cc: Ian Jackson; Keir (Xen.org); Stefano Stabellini; Ian Campbell; xen-
> devel@lists.xen.org
> Subject: Re: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time
> reference counter MSR support
>
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 04 August 2014 15:01
> > To: Paul Durrant
> > Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@lists.xen.org;
> > Keir (Xen.org)
> > Subject: Re: [PATCH v2 3/3] x86/viridian: Add partition time reference
> > counter MSR support
> >
> > >>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> > > +static s_time_t get_time_ref_count(struct domain *d)
> > > +{
> > > + s_time_t now = get_s_time() / 100;
> > > + struct viridian_time_ref_count *trc;
> > > +
> > > + trc = &d->arch.hvm_domain.viridian.time_ref_count;
> > > +
> > > + spin_lock(&trc->lock);
> >
> > How well do you think this will scale, taking into consideration our
> > recent observations with certain Windows versions having all its
> > CPUs race for time stamps over apparently extended periods of
> > time? It would seem to me that a cmpxchg() based approach
> > might be preferable here.
>
> Yes, that may well be true.
>
> >
> > > +
> > > + if ( trc->last == 0 )
> > > + gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER
> > accessed\n");
> >
> > As per the comments on patch 2, this needs to be either printk() or
> > the _G_ in the middle converted to just _.
> >
>
> Ok.
>
> > > +
> > > + if ( (now - trc->last) > 0 )
> > > + trc->last = now;
> > > + else
> > > + now = ++trc->last;
> >
> > And how well is that going to work (and match Windows'
> > expectations) when the migration destination host's NOW() is
> > significantly smaller than the source host's?
> >
>
> I misunderstood what get_s_time() is returning so I think you're correct that
> this will not work will in that case. It's basically the same as the PV rdtsc code;
> but maybe that won't work either.
>
I think that hvm_get_guest_time() should have the right semantics after all, in that it appears to be ns since a vcpu was onlined and is preserved across save/restore.
Paul
> Paul
>
> > > @@ -344,6 +383,14 @@ 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:
> > > + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> > > + return 0;
> > > +
> > > + perfc_incr(mshv_rdmsr_time_ref_count);
> > > + *val = (uint64_t)get_time_ref_count(d);
> >
> > Pointless cast.
> >
> > Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:50 ` Jan Beulich
@ 2014-08-04 14:50 ` Andrew Cooper
2014-08-04 15:13 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Andrew Cooper @ 2014-08-04 14:50 UTC (permalink / raw)
To: Jan Beulich, Paul Durrant
Cc: Ian Jackson, Stefano Stabellini, Keir Fraser, Ian Campbell, xen-devel
On 04/08/14 14:50, Jan Beulich wrote:
>>>> On 04.08.14 at 15:31, <andrew.cooper3@citrix.com> wrote:
>> On 04/08/14 14:12, Paul Durrant wrote:
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>> rc = -EINVAL;
>>> break;
>>> case HVM_PARAM_VIRIDIAN:
>>> - if ( a.value > 1 )
>>> - rc = -EINVAL;
>>> + /* This should only ever be set once by the tools and read by the guest. */
>>> + rc = -EPERM;
>>> + if ( curr_d == d )
>>> + break;
>>> +
>>> + rc = -EPERM;
>>> + if ( d->arch.hvm_domain.params[a.index] &&
>>> + a.value != d->arch.hvm_domain.params[a.index] )
>>> + break;
>> Setting it twice should be an error, even if it is set to the same value
>> again.
> I specifically asked for it to be done this way, such that redundant
> calls wouldn't needlessly fail. Remember that we're altering an
> existing interface, and hence should be careful about breaking
> existing callers.
The only valid users are domain builder parts of the toolstack, which
necessarily needs to be in sync with Xen. All current in-tree callers
are ok. While in general I would agree, we are already changing the
interface quite substantially. A stricter interface is easier to
augment later if the need arises, and here I feel there is sufficient
change to warrant doing the interface properly rather than leaving this
quirk around forevermore.
In a more general sense, having worked on the migration code, I was
considering that it would be a *very* good thing to move all of this
logic into the toolstack, with Xen interacting with a pristine set of
up-to-latest-interface state. This would reduce the amount of Xen code
doing input sanitisation/manipulation, and moves all of the backwards
compatibility cruft into a safer context to run.
One frequently requested feature of XenServer (which has a number of
large obstacles, but is sane in principle) is the ability to migrate
backwards. The usecase is for this is being able to undo a half-upgrade
which has gone very wrong. With all the compatibility code in the
toolstack rather than Xen, this would be feasible to implement.
~Andrew
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 14:47 ` Paul Durrant
@ 2014-08-04 15:11 ` Paul Durrant
2014-08-04 15:15 ` Jan Beulich
0 siblings, 1 reply; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 15:11 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Jackson, Keir (Xen.org), Ian Campbell, Stefano Stabellini, xen-devel
> -----Original Message-----
> From: Paul Durrant
> Sent: 04 August 2014 15:47
> To: Paul Durrant; Jan Beulich
> Cc: Ian Jackson; Keir (Xen.org); Stefano Stabellini; Ian Campbell; xen-
> devel@lists.xen.org
> Subject: RE: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time
> reference counter MSR support
>
> > -----Original Message-----
> > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel-
> > bounces@lists.xen.org] On Behalf Of Paul Durrant
> > Sent: 04 August 2014 15:12
> > To: Jan Beulich
> > Cc: Ian Jackson; Keir (Xen.org); Stefano Stabellini; Ian Campbell; xen-
> > devel@lists.xen.org
> > Subject: Re: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time
> > reference counter MSR support
> >
> > > -----Original Message-----
> > > From: Jan Beulich [mailto:JBeulich@suse.com]
> > > Sent: 04 August 2014 15:01
> > > To: Paul Durrant
> > > Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-
> devel@lists.xen.org;
> > > Keir (Xen.org)
> > > Subject: Re: [PATCH v2 3/3] x86/viridian: Add partition time reference
> > > counter MSR support
> > >
> > > >>> On 04.08.14 at 15:12, <paul.durrant@citrix.com> wrote:
> > > > +static s_time_t get_time_ref_count(struct domain *d)
> > > > +{
> > > > + s_time_t now = get_s_time() / 100;
> > > > + struct viridian_time_ref_count *trc;
> > > > +
> > > > + trc = &d->arch.hvm_domain.viridian.time_ref_count;
> > > > +
> > > > + spin_lock(&trc->lock);
> > >
> > > How well do you think this will scale, taking into consideration our
> > > recent observations with certain Windows versions having all its
> > > CPUs race for time stamps over apparently extended periods of
> > > time? It would seem to me that a cmpxchg() based approach
> > > might be preferable here.
> >
> > Yes, that may well be true.
> >
> > >
> > > > +
> > > > + if ( trc->last == 0 )
> > > > + gdprintk(XENLOG_G_INFO, "TIME_REFERENCE_COUNTER
> > > accessed\n");
> > >
> > > As per the comments on patch 2, this needs to be either printk() or
> > > the _G_ in the middle converted to just _.
> > >
> >
> > Ok.
> >
> > > > +
> > > > + if ( (now - trc->last) > 0 )
> > > > + trc->last = now;
> > > > + else
> > > > + now = ++trc->last;
> > >
> > > And how well is that going to work (and match Windows'
> > > expectations) when the migration destination host's NOW() is
> > > significantly smaller than the source host's?
> > >
> >
> > I misunderstood what get_s_time() is returning so I think you're correct
> that
> > this will not work will in that case. It's basically the same as the PV rdtsc
> code;
> > but maybe that won't work either.
> >
>
> I think that hvm_get_guest_time() should have the right semantics after all,
> in that it appears to be ns since a vcpu was onlined and is preserved across
> save/restore.
>
Actually, I'm no longer convinced about that second part. It looks like only TSC is preserved across save/restore.
Paul
> Paul
>
> > Paul
> >
> > > > @@ -344,6 +383,14 @@ 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:
> > > > + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) )
> > > > + return 0;
> > > > +
> > > > + perfc_incr(mshv_rdmsr_time_ref_count);
> > > > + *val = (uint64_t)get_time_ref_count(d);
> > >
> > > Pointless cast.
> > >
> > > Jan
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 14:50 ` Andrew Cooper
@ 2014-08-04 15:13 ` Jan Beulich
0 siblings, 0 replies; 18+ messages in thread
From: Jan Beulich @ 2014-08-04 15:13 UTC (permalink / raw)
To: Andrew Cooper, Paul Durrant
Cc: Keir Fraser, xen-devel, Ian Jackson, Ian Campbell, StefanoStabellini
>>> On 04.08.14 at 16:50, <andrew.cooper3@citrix.com> wrote:
> On 04/08/14 14:50, Jan Beulich wrote:
>>>>> On 04.08.14 at 15:31, <andrew.cooper3@citrix.com> wrote:
>>> On 04/08/14 14:12, Paul Durrant wrote:
>>>> --- a/xen/arch/x86/hvm/hvm.c
>>>> +++ b/xen/arch/x86/hvm/hvm.c
>>>> @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>> rc = -EINVAL;
>>>> break;
>>>> case HVM_PARAM_VIRIDIAN:
>>>> - if ( a.value > 1 )
>>>> - rc = -EINVAL;
>>>> + /* This should only ever be set once by the tools and read by the guest. */
>>>> + rc = -EPERM;
>>>> + if ( curr_d == d )
>>>> + break;
>>>> +
>>>> + rc = -EPERM;
>>>> + if ( d->arch.hvm_domain.params[a.index] &&
>>>> + a.value != d->arch.hvm_domain.params[a.index] )
>>>> + break;
>>> Setting it twice should be an error, even if it is set to the same value
>>> again.
>> I specifically asked for it to be done this way, such that redundant
>> calls wouldn't needlessly fail. Remember that we're altering an
>> existing interface, and hence should be careful about breaking
>> existing callers.
>
> The only valid users are domain builder parts of the toolstack, which
> necessarily needs to be in sync with Xen. All current in-tree callers
> are ok. While in general I would agree, we are already changing the
> interface quite substantially. A stricter interface is easier to
> augment later if the need arises, and here I feel there is sufficient
> change to warrant doing the interface properly rather than leaving this
> quirk around forevermore.
Let me put it that way: I'd prefer this to be done as asked for earlier,
but I also wouldn't veto it being done the way you want it.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 15:11 ` Paul Durrant
@ 2014-08-04 15:15 ` Jan Beulich
2014-08-04 15:23 ` Paul Durrant
0 siblings, 1 reply; 18+ messages in thread
From: Jan Beulich @ 2014-08-04 15:15 UTC (permalink / raw)
To: Paul Durrant
Cc: Ian Jackson, Keir (Xen.org), Stefano Stabellini, Ian Campbell, xen-devel
>>> On 04.08.14 at 17:11, <Paul.Durrant@citrix.com> wrote:
>> From: Paul Durrant
>> I think that hvm_get_guest_time() should have the right semantics after all,
>> in that it appears to be ns since a vcpu was onlined and is preserved across
>> save/restore.
>
> Actually, I'm no longer convinced about that second part. It looks like only
> TSC is preserved across save/restore.
Indeed the saving/restoring would need to be written, similar to what
is done for the other virtual time sources. And just like for those other
system wide time sources, you'd have to pick a reference vCPU.
Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support
2014-08-04 15:15 ` Jan Beulich
@ 2014-08-04 15:23 ` Paul Durrant
0 siblings, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 15:23 UTC (permalink / raw)
To: Jan Beulich
Cc: Ian Jackson, Keir (Xen.org), Stefano Stabellini, Ian Campbell, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 04 August 2014 16:16
> To: Paul Durrant
> Cc: Ian Campbell; Ian Jackson; Stefano Stabellini; xen-devel@lists.xen.org;
> Keir (Xen.org)
> Subject: RE: [Xen-devel] [PATCH v2 3/3] x86/viridian: Add partition time
> reference counter MSR support
>
> >>> On 04.08.14 at 17:11, <Paul.Durrant@citrix.com> wrote:
> >> From: Paul Durrant
> >> I think that hvm_get_guest_time() should have the right semantics after
> all,
> >> in that it appears to be ns since a vcpu was onlined and is preserved across
> >> save/restore.
> >
> > Actually, I'm no longer convinced about that second part. It looks like only
> > TSC is preserved across save/restore.
>
> Indeed the saving/restoring would need to be written, similar to what
> is done for the other virtual time sources. And just like for those other
> system wide time sources, you'd have to pick a reference vCPU.
>
Since tsc *is* saved and restored then I should be able to use hvm_get_guest_tsc() and convert back to ns, I think.
Paul
> Jan
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask
2014-08-04 13:31 ` Andrew Cooper
2014-08-04 13:50 ` Jan Beulich
@ 2014-08-04 17:19 ` Paul Durrant
1 sibling, 0 replies; 18+ messages in thread
From: Paul Durrant @ 2014-08-04 17:19 UTC (permalink / raw)
To: Andrew Cooper, xen-devel
Cc: Stefano Stabellini, Keir (Xen.org),
Ian Campbell, Jan Beulich, Ian Jackson
> -----Original Message-----
> From: Andrew Cooper
> Sent: 04 August 2014 14:31
> To: Paul Durrant; xen-devel@lists.xen.org
> Cc: Keir (Xen.org); Ian Campbell; Stefano Stabellini; Ian Jackson; Jan Beulich
> Subject: Re: [Xen-devel] [PATCH v2 1/3] x86/viridian: Re-purpose the HVM
> parameter to be a feature mask
>
> On 04/08/14 14:12, Paul Durrant wrote:
> > The following commits introduced the time reference counter MSR and
> > TSC/APIC frequency MSRs into the viridian feature set respectively:
> >
> > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b
> > 84657efd9116f40924aa13c9d5a349e007da716f
> >
> > The time reference counter MSR feature was then reverted by commit
> >
> > 1cd4fab14ce25859efa4a2af13475e6650a5506c
> >
> > because a flaw in the implementation meant the counter was reset on
> > migration.
> >
> > All of these changes were made without any addtional options being
> > added to the VM configuration, or any compatibility checks being made
> > in the domain save/restore code. Hence setting the single boolean
> > 'viridian' option in the VM configuration yields a different set of
> > features depending on which version of Xen the VM is started on, and the
> > feature set can change across migration (so new MSRs can magically
> appear).
> >
> > This patch grandfathers in the current viridian features set and calls them
> > the 'base' plus 'freq' feature set. HVM_PARAM_VIRIDIAN is re-purposed as
> > a feature mask. It has only ever been legally set to 0 or 1, so the presence
> > of the base set and freq set are indicated by setting bit 0. The 'freq' set
> > can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4
> > 'base' set. Newly implemented viridian features can be optionally enabled
> > in future by setting further bits.
> >
> > The viridian option in xl.cfg(5) has also been changed to a string list so
> > that the sets can be individually sepcified. For compatibility, if the
> > option is specified as a boolean, then a true (1) value will be translated
> > to a string list containing "base" and "freq".
> >
> > This patch also alters the allowed write accesses to
> HVM_PARAM_VIRIDIAN.
> > Currently there is nothing to stop the guest writing this value (which,
> > while harmless to anything else, should not happen) and nothing to
> > stop a toolstack from setting the value back to zero whilst the guest is
> > running, causing CPUID leaves to disappear and MSR accesses to start
> > causing GPFs in the guest. Both of these possibilities are now disallowed:
> > Once the parameter is set to a non-zero value it may not be cleared, and
> > guests no longer have any write access.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Cc: Ian Campbell <ian.campbell@citrix.com>
> > Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> > Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> > ---
> > docs/man/xl.cfg.pod.5 | 53 ++++++++++++++++++++++++++++++--
> -------
> > tools/libxc/xc_domain_restore.c | 4 +--
> > tools/libxl/libxl.h | 6 +++++
> > tools/libxl/libxl_create.c | 1 -
> > tools/libxl/libxl_dom.c | 46 ++++++++++++++++++++++++++-------
> > tools/libxl/libxl_types.idl | 2 +-
> > tools/libxl/xl_cmdimpl.c | 24 +++++++++++++++++-
> > tools/libxl/xl_sxp.c | 8 ++++--
> > xen/arch/x86/hvm/hvm.c | 18 +++++++++++--
> > xen/arch/x86/hvm/viridian.c | 21 ++++++++++++++--
> > xen/include/asm-x86/hvm/hvm.h | 7 ++++--
> > xen/include/public/hvm/params.h | 33 +++++++++++++++++++++++-
> > 12 files changed, 188 insertions(+), 35 deletions(-)
> >
> > diff --git a/docs/man/xl.cfg.pod.5 b/docs/man/xl.cfg.pod.5
> > index ff9ea77..a05e831 100644
> > --- a/docs/man/xl.cfg.pod.5
> > +++ b/docs/man/xl.cfg.pod.5
> > @@ -1084,18 +1084,47 @@ Windows
> L<http://wiki.xen.org/wiki/XenWindowsGplPv>.
> > Setting B<xen_platform_pci=0> with the default device_model "qemu-
> xen"
> > requires at least QEMU 1.6.
> >
> > -=item B<viridian=BOOLEAN>
> > -
> > -Turns on or off the exposure of MicroSoft Hyper-V (AKA viridian)
> > -compatible enlightenments to the guest. These can improve performance
> > -of Microsoft Windows guests from Windows Vista and Windows 2008
> > -onwards and setting this option for such guests is strongly
> > -recommended. This option should be harmless for other versions of
> > -Windows (although it will not give any benefit) and the majority of
> > -other non-Windows OSes. However it is known to be incompatible with
> > -some other Operating Systems and in some circumstance can prevent
> > -Xen's own paravirtualisation interfaces for HVM guests from being
> > -used.
> > +=item B<viridian=[ "SET", "SET", ...]>
> > +
> > +The sets of Microsoft Hyper-V (AKA viridian) compatible enlightenments
> > +exposed to the guest. The following sets of enlightenments may be
> > +specified:
> > +
> > +=over 4
> > +
> > +=item B<base>
> > +
> > +This set incorporates the Hypercall MSRs, Virtual processor index MSR,
> > +and APIC access MSRs. This set is a pre-requisite for all other sets.
> > +If it is not specified then all enlightenments will be disabled.
> > +
> > +These enlightenments can improve performance of Windows Vista and
> Windows
> > +Server 2008 onwards and setting this option for such guests is strongly
> > +recommended.
> > +
> > +=item B<freq>
> > +
> > +This set incorporates the TSC and APIC frequency MSRs.
> > +
> > +This enlightenment can improve performance of Windows 7 and Windows
> > +Server 2008 R2 onwards.
> > +
> > +=back
> > +
> > +See the latest version of Microsoft's Hypervisor Top-Level Functional
> > +Specification for more details.
> > +
> > +The enlightenments should be harmless for other versions of Windows
> > +(although they will not give any benefit) and the majority of other
> > +non-Windows OSes.
> > +However it is known that they are incompatible with some other
> Operating
> > +Systems and in some circumstance can prevent Xen's own
> paravirtualisation
> > +interfaces for HVM guests from being used.
> > +
> > +Specifying the viridian option as a boolean is deprecated. However, for
> > +backwards compatibility, if it is specified as a boolean a value of
> > +true (1) will cause both the 'base' and 'freq' sets to be exposed to
> > +the guest, and a value of false (0) will disable all enlightenments.
> >
> > =back
> >
> > diff --git a/tools/libxc/xc_domain_restore.c
> b/tools/libxc/xc_domain_restore.c
> > index e73e0a2..5e7e62d 100644
> > --- a/tools/libxc/xc_domain_restore.c
> > +++ b/tools/libxc/xc_domain_restore.c
> > @@ -922,7 +922,7 @@ static int pagebuf_get_one(xc_interface *xch,
> struct restore_ctx *ctx,
> > if ( RDEXACT(fd, &buf->viridian, sizeof(uint32_t)) ||
> > RDEXACT(fd, &buf->viridian, sizeof(uint64_t)) )
> > {
> > - PERROR("error read the viridian flag");
> > + PERROR("error read the viridian flags");
> > return -1;
> > }
> > return pagebuf_get_one(xch, ctx, buf, fd, dom);
> > @@ -1733,7 +1733,7 @@ int xc_domain_restore(xc_interface *xch, int
> io_fd, uint32_t dom,
> > }
> >
> > if (pagebuf.viridian != 0)
> > - xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN, 1);
> > + xc_hvm_param_set(xch, dom, HVM_PARAM_VIRIDIAN,
> pagebuf.viridian);
> >
> > /*
> > * If we are migrating in from a host that does not support
> > diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
> > index 5ae6532..b985f49 100644
> > --- a/tools/libxl/libxl.h
> > +++ b/tools/libxl/libxl.h
> > @@ -128,6 +128,12 @@
> > #define LIBXL_HAVE_LIBXL_DEVICE_DISK_DISCARD_ENABLE 1
> >
> > /*
> > + * The libxl_domain_build_info u.hvm.viridian field is a string list
> > + */
> > +#define LIBXL_HAVE_BUILDINFO_HVM_VIRIDIAN_STRING_LIST 1
> > +
> > +
> > +/*
> > * libxl ABI compatibility
> > *
> > * The only guarantee which libxl makes regarding ABI compatibility
> > diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
> > index 0686f96..ee19fc8 100644
> > --- a/tools/libxl/libxl_create.c
> > +++ b/tools/libxl/libxl_create.c
> > @@ -273,7 +273,6 @@ int libxl__domain_build_info_setdefault(libxl__gc
> *gc,
> > libxl_defbool_setdefault(&b_info->u.hvm.acpi_s3, true);
> > libxl_defbool_setdefault(&b_info->u.hvm.acpi_s4, true);
> > libxl_defbool_setdefault(&b_info->u.hvm.nx, true);
> > - libxl_defbool_setdefault(&b_info->u.hvm.viridian, false);
> > libxl_defbool_setdefault(&b_info->u.hvm.hpet, true);
> > libxl_defbool_setdefault(&b_info->u.hvm.vpt_align, true);
> > libxl_defbool_setdefault(&b_info->u.hvm.nested_hvm, false);
> > diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
> > index 83eb29a..a15c185 100644
> > --- a/tools/libxl/libxl_dom.c
> > +++ b/tools/libxl/libxl_dom.c
> > @@ -209,21 +209,49 @@ static unsigned long timer_mode(const
> libxl_domain_build_info *info)
> > return ((unsigned long)mode);
> > }
> >
> > -static void hvm_set_conf_params(xc_interface *handle, uint32_t domid,
> > +#if defined(__i386__) || defined(__x86_64__)
> > +static void hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
> > + libxl_domain_build_info *const info)
> > +{
> > + uint64_t feature_mask = HVMPV_no_freq;
> > +
> > + char **p = info->u.hvm.viridian;
> > + while (*p) {
> > + LOG(DETAIL, "+%s", *p);
> > + if (strcmp(*p, "base") == 0)
> > + feature_mask |= HVMPV_base_freq;
> > + else if (strcmp(*p, "freq") == 0)
> > + feature_mask &= ~HVMPV_no_freq;
> > + p++;
> > + }
> > +
> > + if (xc_hvm_param_set(CTX->xch,
> > + domid,
> > + HVM_PARAM_VIRIDIAN,
> > + feature_mask) != 0)
> > + LIBXL__LOG_ERRNO(CTX, LIBXL__LOG_WARNING,
> > + "Couldn't set viridian feature mask (0x%lx)",
>
> PRIx64, or this will break 32bit builds.
>
Ok. Done.
> > + feature_mask);
> > +}
> > +#endif
> > +
> > +static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid,
> > libxl_domain_build_info *const info)
>
> const libxl_domain_build_info *info ?
>
I didn't actually change this - it's code movement.
> > {
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED,
> > + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED,
> > libxl_defbool_val(info->u.hvm.pae));
> > #if defined(__i386__) || defined(__x86_64__)
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN,
> > - libxl_defbool_val(info->u.hvm.viridian));
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED,
> > + if (libxl_string_list_length(&info->u.hvm.viridian) != 0)
> > + hvm_set_viridian_features(gc, domid, info);
> > +
> > + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_HPET_ENABLED,
> > libxl_defbool_val(info->u.hvm.hpet));
> > #endif
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_TIMER_MODE,
> timer_mode(info));
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_VPT_ALIGN,
> > + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_TIMER_MODE,
> > + timer_mode(info));
> > + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_VPT_ALIGN,
> > libxl_defbool_val(info->u.hvm.vpt_align));
> > - xc_hvm_param_set(handle, domid, HVM_PARAM_NESTEDHVM,
> > + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_NESTEDHVM,
> > libxl_defbool_val(info->u.hvm.nested_hvm));
> > }
> >
> > @@ -306,7 +334,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
> > state->console_port = xc_evtchn_alloc_unbound(ctx->xch, domid,
> state->console_domid);
> >
> > if (info->type == LIBXL_DOMAIN_TYPE_HVM)
> > - hvm_set_conf_params(ctx->xch, domid, info);
> > + hvm_set_conf_params(gc, domid, info);
> >
> > rc = libxl__arch_domain_create(gc, d_config, domid);
> >
> > diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
> > index a412f9c..6c5d7e0 100644
> > --- a/tools/libxl/libxl_types.idl
> > +++ b/tools/libxl/libxl_types.idl
> > @@ -363,7 +363,7 @@ libxl_domain_build_info =
> Struct("domain_build_info",[
> > ("acpi_s3", libxl_defbool),
> > ("acpi_s4", libxl_defbool),
> > ("nx", libxl_defbool),
> > - ("viridian", libxl_defbool),
> > + ("viridian", libxl_string_list),
> > ("timeoffset", string),
> > ("hpet", libxl_defbool),
> > ("vpt_align", libxl_defbool),
> > diff --git a/tools/libxl/xl_cmdimpl.c b/tools/libxl/xl_cmdimpl.c
> > index 01bce2f..960f626 100644
> > --- a/tools/libxl/xl_cmdimpl.c
> > +++ b/tools/libxl/xl_cmdimpl.c
> > @@ -951,10 +951,32 @@ static void parse_config_data(const char
> *config_source,
> > xlu_cfg_get_defbool(config, "acpi_s3", &b_info->u.hvm.acpi_s3, 0);
> > xlu_cfg_get_defbool(config, "acpi_s4", &b_info->u.hvm.acpi_s4, 0);
> > xlu_cfg_get_defbool(config, "nx", &b_info->u.hvm.nx, 0);
> > - xlu_cfg_get_defbool(config, "viridian", &b_info->u.hvm.viridian, 0);
> > xlu_cfg_get_defbool(config, "hpet", &b_info->u.hvm.hpet, 0);
> > xlu_cfg_get_defbool(config, "vpt_align", &b_info->u.hvm.vpt_align,
> 0);
> >
> > + switch (xlu_cfg_get_list_as_string_list(config, "viridian",
> > + &b_info->u.hvm.viridian, 1))
> > + {
> > + case 0: break; /* Success */
> > + case ESRCH: break; /* Option not present */
> > + case EINVAL: {
> > + libxl_defbool b;
> > +
> > + xlu_cfg_get_defbool(config, "viridian", &b, 1);
> > + if (libxl_defbool_val(b)) {
> > + fprintf(stderr, "WARNING: Specifying \"viridian\""
> > + " as a boolean is deprecated. "
> > + "Please use a list of features.\n");
> > + split_string_into_string_list("base freq", " \n",
> > + &b_info->u.hvm.viridian);
> > + }
> > + break;
> > + }
> > + default:
> > + fprintf(stderr,"xl: Unable to parse bootloader_args.\n");
> > + exit(-ERROR_FAIL);
> > + }
> > +
> > if (!xlu_cfg_get_long(config, "timer_mode", &l, 1)) {
> > const char *s = libxl_timer_mode_to_string(l);
> > fprintf(stderr, "WARNING: specifying \"timer_mode\" as an integer
> is deprecated. "
> > diff --git a/tools/libxl/xl_sxp.c b/tools/libxl/xl_sxp.c
> > index 48eb608..347526f 100644
> > --- a/tools/libxl/xl_sxp.c
> > +++ b/tools/libxl/xl_sxp.c
> > @@ -97,8 +97,12 @@ void printf_info_sexp(int domid, libxl_domain_config
> *d_config)
> > printf("\t\t\t(acpi %s)\n",
> > libxl_defbool_to_string(b_info->u.hvm.acpi));
> > printf("\t\t\t(nx %s)\n", libxl_defbool_to_string(b_info->u.hvm.nx));
> > - printf("\t\t\t(viridian %s)\n",
> > - libxl_defbool_to_string(b_info->u.hvm.viridian));
> > + if (b_info->u.hvm.viridian) {
> > + printf("\t\t\t(viridian");
> > + for (i=0; b_info->u.hvm.viridian[i]; i++)
> > + printf(" %s", b_info->u.hvm.viridian[i]);
> > + printf(")\n");
> > + }
> > printf("\t\t\t(hpet %s)\n",
> > libxl_defbool_to_string(b_info->u.hvm.hpet));
> > printf("\t\t\t(vpt_align %s)\n",
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index fba13e0..04d43d0 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5533,8 +5533,22 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> > rc = -EINVAL;
> > break;
> > case HVM_PARAM_VIRIDIAN:
> > - if ( a.value > 1 )
> > - rc = -EINVAL;
> > + /* This should only ever be set once by the tools and read by the
> guest. */
> > + rc = -EPERM;
> > + if ( curr_d == d )
> > + break;
> > +
> > + rc = -EPERM;
> > + if ( d->arch.hvm_domain.params[a.index] &&
> > + a.value != d->arch.hvm_domain.params[a.index] )
> > + break;
>
> Setting it twice should be an error, even if it is set to the same value
> again. Also, EEXIST would be a better error.
>
I've opted for EEXIST but, as Jan requested, setting to the same value is allowed.
> > +
> > + rc = -EINVAL;
> > + if ( a.value & ~HVMPV_feature_mask ||
> > + !(a.value & HVMPV_base_freq) )
> > + break;
> > +
> > + rc = 0;
>
> To save bloating the code here if/when new combinations are introduced,
> would it be better to have a "rc = viridian_initialise(d, a.value);"
> which validates the featureset and sets it if necessary. The above
> EEXISTs check could be included as well.
This statement won't grow with new features (since they are added to the mask), so probably not worth it.
>
> > break;
> > case HVM_PARAM_IDENT_PT:
> > /* Not reflexive, as we must domain_pause(). */
> > diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
> > index 0fcbfd8..31c9656 100644
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -90,8 +90,9 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned
> int *eax,
> > /* Which hypervisor MSRs are available to the guest */
> > *eax = (CPUID3A_MSR_APIC_ACCESS |
> > CPUID3A_MSR_HYPERCALL |
> > - CPUID3A_MSR_VP_INDEX |
> > - CPUID3A_MSR_FREQ);
> > + CPUID3A_MSR_VP_INDEX);
> > + if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> > + *eax |= CPUID3A_MSR_FREQ;
> > break;
> > case 4:
> > /* Recommended hypercall usage. */
> > @@ -312,11 +313,17 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t
> *val)
> > break;
> >
> > case VIRIDIAN_MSR_TSC_FREQUENCY:
> > + if ( viridian_feature_mask(d) & HVMPV_no_freq )
> > + return 0;
> > +
> > perfc_incr(mshv_rdmsr_tsc_frequency);
> > *val = (uint64_t)d->arch.tsc_khz * 1000ull;
> > break;
> >
> > case VIRIDIAN_MSR_APIC_FREQUENCY:
> > + if ( viridian_feature_mask(d) & HVMPV_no_freq )
> > + return 0;
> > +
> > perfc_incr(mshv_rdmsr_apic_frequency);
> > *val = 1000000000ull / APIC_BUS_CYCLE_NS;
> > break;
> > @@ -489,3 +496,13 @@ static int viridian_load_vcpu_ctxt(struct domain
> *d, hvm_domain_context_t *h)
> >
> > HVM_REGISTER_SAVE_RESTORE(VIRIDIAN_VCPU,
> viridian_save_vcpu_ctxt,
> > viridian_load_vcpu_ctxt, 1, HVMSR_PER_VCPU);
> > +
> > +/*
> > + * Local variables:
> > + * mode: C
> > + * c-file-style: "BSD"
> > + * c-basic-offset: 4
> > + * tab-width: 4
> > + * indent-tabs-mode: nil
> > + * End:
> > + */
> > diff --git a/xen/include/asm-x86/hvm/hvm.h b/xen/include/asm-
> x86/hvm/hvm.h
> > index 0ebd478..24e513b 100644
> > --- a/xen/include/asm-x86/hvm/hvm.h
> > +++ b/xen/include/asm-x86/hvm/hvm.h
> > @@ -344,8 +344,11 @@ static inline unsigned long
> hvm_get_shadow_gs_base(struct vcpu *v)
> > return hvm_funcs.get_shadow_gs_base(v);
> > }
> >
> > -#define is_viridian_domain(_d) \
> > - (is_hvm_domain(_d) && ((_d)-
> >arch.hvm_domain.params[HVM_PARAM_VIRIDIAN]))
> > +#define viridian_feature_mask(d) \
> > + ((d)->arch.hvm_domain.params[HVM_PARAM_VIRIDIAN])
> > +
> > +#define is_viridian_domain(d) \
> > + (is_hvm_domain(d) && (viridian_feature_mask(d) &
> HVMPV_base_freq))
> >
> > void hvm_hypervisor_cpuid_leaf(uint32_t sub_idx,
> > uint32_t *eax, uint32_t *ebx,
> > diff --git a/xen/include/public/hvm/params.h
> b/xen/include/public/hvm/params.h
> > index 614ff5f..68d26fd 100644
> > --- a/xen/include/public/hvm/params.h
> > +++ b/xen/include/public/hvm/params.h
> > @@ -56,9 +56,40 @@
> >
> > #if defined(__i386__) || defined(__x86_64__)
> >
> > -/* Expose Viridian interfaces to this HVM guest? */
> > +/*
> > + * Viridian enlightenments
> > + *
> > + * (See http://download.microsoft.com/download/A/B/4/AB43A34E-
> BDD0-4FA6-BDEF-
> 79EEF16E880B/Hypervisor%20Top%20Level%20Functional%20Specification%2
> 0v4.0.docx)
> > + *
> > + * To expose viridian enlightenments to the guest set this parameter
> > + * to the desired feature mask. The base feature set must be present
> > + * in any valid feature mask.
> > + */
> > #define HVM_PARAM_VIRIDIAN 9
> >
> > +/* Base+Freq viridian feature sets:
> > + *
> > + * - Hypercall MSRs (HV_X64_MSR_GUEST_OS_ID and
> HV_X64_MSR_HYPERCALL)
> > + * - APIC access MSRs (HV_X64_MSR_EOI, HV_X64_MSR_ICR and
> HV_X64_MSR_TPR)
> > + * - Virtual Processor index MSR (HV_X64_MSR_VP_INDEX)
> > + * - Timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> > + * HV_X64_MSR_APIC_FREQUENCY)
> > + */
> > +#define _HVMPV_base_freq 0
> > +#define HVMPV_base_freq (1 << _HVMPV_base_freq)
> > +
> > +/* Feature set modifications */
> > +
> > +/* Disable timer frequency MSRs (HV_X64_MSR_TSC_FREQUENCY and
> > + * HV_X64_MSR_APIC_FREQUENCY).
> > + * This modification restores the viridian feature set to the
> > + * original 'base' set exposed in releases prior to Xen 4.4.
> > + */
> > +#define _HVMPV_no_freq 1
> > +#define HVMPV_no_freq (1 << _HVMPV_no_freq)
> > +
> > +#define HVMPV_feature_mask (HVMPV_base_freq|HVMPV_no_freq)
>
> Please could we avoid introducing new negated-sense booleans, especially
> when their predominant use is "if ( !$FOO_no_BAR )"
>
As Jan points out, I have no choice if I am to maintain compatibility but still allow the frequency MSRs to be optional - as I think they should be.
Paul
> ~Andrew
>
> > +
> > #endif
> >
> > /*
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2014-08-04 17:19 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-04 13:12 [PATCH v2 0/3] x86/viridian improvements Paul Durrant
2014-08-04 13:12 ` [PATCH v2 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
2014-08-04 13:31 ` Andrew Cooper
2014-08-04 13:50 ` Jan Beulich
2014-08-04 14:50 ` Andrew Cooper
2014-08-04 15:13 ` Jan Beulich
2014-08-04 17:19 ` Paul Durrant
2014-08-04 13:45 ` Jan Beulich
2014-08-04 13:12 ` [PATCH v2 2/3] x86/viridian: Note that logging is under control of the guest Paul Durrant
2014-08-04 13:38 ` Andrew Cooper
2014-08-04 13:45 ` Paul Durrant
2014-08-04 13:12 ` [PATCH v2 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant
2014-08-04 14:01 ` Jan Beulich
2014-08-04 14:12 ` Paul Durrant
2014-08-04 14:47 ` Paul Durrant
2014-08-04 15:11 ` Paul Durrant
2014-08-04 15:15 ` Jan Beulich
2014-08-04 15:23 ` Paul Durrant
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.