* [PATCH v5 0/3] x86/viridian improvements
@ 2014-08-05 13:07 Paul Durrant
2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant
` (3 more replies)
0 siblings, 4 replies; 16+ messages in thread
From: Paul Durrant @ 2014-08-05 13:07 UTC (permalink / raw)
To: xen-devel
This patch series incorporates several improvements to the code
supporting viridian (i.e. hyper-v compatible) enlightenments for
Windows guests:
Patch #1 series lays the foundations for adding new viridian
enlightenments such that they can be optionally enabled, and not
immediately exposed to a guest across a save/restore boundary.
Patch #2 makes logging less verbose
Patch #3 adds support for the 'Partition Time Reference Counter'
enlightenment.
v2:
- Addressed comments from Jan Beulich
- Added patch #2
v3:
- Addressed comments from Andrew Cooper and Jan Beulich
- Re-worked patch #2
- Simplified patch #3 to use guest TSC
v4:
- Added missing domain info to printks in patch #2
v5:
- Clarified comment of patch #1 as suggested by David Vrabel
- More logging tweaks in patch #2 as suggested by Andrew Cooper
^ permalink raw reply [flat|nested] 16+ messages in thread* [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-05 13:07 [PATCH v5 0/3] x86/viridian improvements Paul Durrant @ 2014-08-05 13:07 ` Paul Durrant 2014-08-12 15:25 ` Ian Jackson 2014-08-26 20:28 ` Ian Campbell 2014-08-05 13:07 ` [PATCH v5 2/3] x86/viridian: Make logging less verbose Paul Durrant ` (2 subsequent siblings) 3 siblings, 2 replies; 16+ messages in thread From: Paul Durrant @ 2014-08-05 13:07 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' and 'freq' feature sets. HVM_PARAM_VIRIDIAN is re-purposed as a feature mask. The hypervisor has only ever allowed it ot be set to 0 or 1, so the presence of the base and freq sets are indicated by setting bit 0. The freq set can then be turned off by setting bit 1, thus restoring the pre-Xen-4.4 base set. Newly implemented viridian features can be optionally enabled in future by setting further bits. The viridian option in xl.cfg(5) has also been changed to a string list so that the sets can be individually sepcified. For compatibility, if the option is specified as a boolean, then a true (1) value will be translated to a string list containing "base" and "freq". This patch also alters the allowed write accesses to HVM_PARAM_VIRIDIAN. Currently there is nothing to stop the guest writing this value (which, while harmless to anything else, should not happen) and nothing to stop a toolstack from setting the value back to zero whilst the guest is running, causing CPUID leaves to disappear and MSR accesses to start causing GPFs in the guest. Both of these possibilities are now disallowed: Once the parameter is set to a non-zero value it may not be cleared, and guests no longer have any write access. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Keir Fraser <keir@xen.org> 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 | 5 ++++ 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(+), 34 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..da3acdd 100644 --- a/tools/libxl/libxl.h +++ b/tools/libxl/libxl.h @@ -128,6 +128,11 @@ #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..cc57931 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%"PRIx64")", + feature_mask); +} +#endif + +static void hvm_set_conf_params(libxl__gc *gc, uint32_t domid, libxl_domain_build_info *const info) { - xc_hvm_param_set(handle, domid, HVM_PARAM_PAE_ENABLED, + xc_hvm_param_set(CTX->xch, domid, HVM_PARAM_PAE_ENABLED, libxl_defbool_val(info->u.hvm.pae)); #if defined(__i386__) || defined(__x86_64__) - xc_hvm_param_set(handle, domid, HVM_PARAM_VIRIDIAN, - libxl_defbool_val(info->u.hvm.viridian)); - xc_hvm_param_set(handle, domid, HVM_PARAM_HPET_ENABLED, + 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..d1f890c 100644 --- a/xen/arch/x86/hvm/hvm.c +++ b/xen/arch/x86/hvm/hvm.c @@ -5533,8 +5533,24 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg) rc = -EINVAL; break; case HVM_PARAM_VIRIDIAN: - if ( a.value > 1 ) + /* This should only ever be set once by the tools and read by the guest. */ + rc = -EPERM; + if ( curr_d == d ) + break; + + if ( a.value != d->arch.hvm_domain.params[a.index] ) + { + rc = -EEXIST; + if ( d->arch.hvm_domain.params[a.index] != 0 ) + break; + rc = -EINVAL; + if ( (a.value & ~HVMPV_feature_mask) || + !(a.value & HVMPV_base_freq) ) + break; + } + + rc = 0; break; case HVM_PARAM_IDENT_PT: /* Not reflexive, as we must domain_pause(). */ diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 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] 16+ messages in thread
* Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant @ 2014-08-12 15:25 ` Ian Jackson 2014-08-13 11:09 ` Paul Durrant 2014-08-26 20:28 ` Ian Campbell 1 sibling, 1 reply; 16+ messages in thread From: Ian Jackson @ 2014-08-12 15:25 UTC (permalink / raw) To: Paul Durrant Cc: Stefano Stabellini, Keir Fraser, Ian Campbell, Jan Beulich, xen-devel Paul Durrant writes ("[PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask"): > 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 is correct, I think. But the libxl API should be in numbers, not a string list. Text to string conversion should be done in xl, not libxl. I think that these flag values ought to be represented in the IDL, so that text-to-string conversion can be done automatically. I looked at the existing IDL definitions and there don't seem to be any flags of this kind. So I think this will involve a inventing new kind of IDL type. It might be possible to adapt Enumeration. It might be enough simply to provide a way to make an Enumeration whose corresponding typedef is to a uint32_t. Does this make any kind of sense ? Thanks, Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-12 15:25 ` Ian Jackson @ 2014-08-13 11:09 ` Paul Durrant 2014-08-26 20:24 ` Ian Campbell 0 siblings, 1 reply; 16+ messages in thread From: Paul Durrant @ 2014-08-13 11:09 UTC (permalink / raw) To: Ian Jackson Cc: Stefano Stabellini, Keir (Xen.org), Ian Campbell, Jan Beulich, xen-devel > -----Original Message----- > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] > Sent: 12 August 2014 16:26 > To: Paul Durrant > Cc: xen-devel@lists.xen.org; Keir (Xen.org); Jan Beulich; Ian Campbell; > Stefano Stabellini > Subject: Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to > be a feature mask > > Paul Durrant writes ("[PATCH v5 1/3] x86/viridian: Re-purpose the HVM > parameter to be a feature mask"): > > 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 is correct, I think. But the libxl API should be in numbers, not > a string list. Text to string conversion should be done in xl, not > libxl. > > I think that these flag values ought to be represented in the IDL, so > that text-to-string conversion can be done automatically. > > I looked at the existing IDL definitions and there don't seem to be > any flags of this kind. So I think this will involve a inventing new > kind of IDL type. It might be possible to adapt Enumeration. It > might be enough simply to provide a way to make an Enumeration whose > corresponding typedef is to a uint32_t. > > Does this make any kind of sense ? > Yes, it does. I guess what we really want is an 'enumeration list' then - i.e. strings get translated via enumeration and then concatenated into a list. Is there any precedent for such a thing? Paul > Thanks, > Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-13 11:09 ` Paul Durrant @ 2014-08-26 20:24 ` Ian Campbell 0 siblings, 0 replies; 16+ messages in thread From: Ian Campbell @ 2014-08-26 20:24 UTC (permalink / raw) To: Paul Durrant Cc: Ian Jackson, Stefano Stabellini, Keir (Xen.org), Jan Beulich, xen-devel On Wed, 2014-08-13 at 12:09 +0100, Paul Durrant wrote: > > -----Original Message----- > > From: Ian Jackson [mailto:Ian.Jackson@eu.citrix.com] > > Sent: 12 August 2014 16:26 > > To: Paul Durrant > > Cc: xen-devel@lists.xen.org; Keir (Xen.org); Jan Beulich; Ian Campbell; > > Stefano Stabellini > > Subject: Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to > > be a feature mask > > > > Paul Durrant writes ("[PATCH v5 1/3] x86/viridian: Re-purpose the HVM > > parameter to be a feature mask"): > > > 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 is correct, I think. But the libxl API should be in numbers, not > > a string list. Text to string conversion should be done in xl, not > > libxl. > > > > I think that these flag values ought to be represented in the IDL, so > > that text-to-string conversion can be done automatically. > > > > I looked at the existing IDL definitions and there don't seem to be > > any flags of this kind. So I think this will involve a inventing new > > kind of IDL type. It might be possible to adapt Enumeration. It > > might be enough simply to provide a way to make an Enumeration whose > > corresponding typedef is to a uint32_t. > > > > Does this make any kind of sense ? > > > > Yes, it does. I guess what we really want is an 'enumeration list' > then - i.e. strings get translated via enumeration and then > concatenated into a list. Is there any precedent for such a thing? For singleton enums there is, but not for lists of such things. I don't think. But isn't what you actually want here a struct viridian-ish thing containing a bunch of (def)bools, one per fearture? That would seem like a more natural interface for a libxl app (a list of VIRIDIAN_FEATURE_X, VIRIDIAN_FEATURE_Y doesn't seem quite right to me). Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant 2014-08-12 15:25 ` Ian Jackson @ 2014-08-26 20:28 ` Ian Campbell 2014-08-27 8:11 ` Paul Durrant 1 sibling, 1 reply; 16+ messages in thread From: Ian Campbell @ 2014-08-26 20:28 UTC (permalink / raw) To: Paul Durrant Cc: Ian Jackson, Stefano Stabellini, Keir Fraser, Jan Beulich, xen-devel On Tue, 2014-08-05 at 14:07 +0100, Paul Durrant wrote: Sorry for the delay replying, I've been on vacation and travelling. > 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), Unfortunately this will break the API. specifically existing apps which call libxl_defbool_set on this field will now get a type error. You need to keep the existing viridian field and add a new one with the new semantics and a new name (e.g. viridian_features, or a set of new defbools as I suggested in my previous mail) and define the precedence. See usbdevice vs. usbdevice_list or the (very new) serial one for existing examples of this sort of thing. For your case though you could continue to treat the viridian defbool as a gate on a bunch of viridian_feature_X defbools, IOW if viridian == false the others are ignored, if it is true then the others have the usual defbool behaviour of defaults and overrides. I think that would fall out fairly naturally in the setdefault function. Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask 2014-08-26 20:28 ` Ian Campbell @ 2014-08-27 8:11 ` Paul Durrant 0 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2014-08-27 8:11 UTC (permalink / raw) To: Ian Campbell Cc: Ian Jackson, Stefano Stabellini, Keir (Xen.org), Jan Beulich, xen-devel > -----Original Message----- > From: Ian Campbell [mailto:ian.campbell@citrix.com] > Sent: 26 August 2014 21:29 > To: Paul Durrant > Cc: xen-devel@lists.xen.org; Keir (Xen.org); Jan Beulich; Ian Jackson; Stefano > Stabellini > Subject: Re: [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to > be a feature mask > > On Tue, 2014-08-05 at 14:07 +0100, Paul Durrant wrote: > > Sorry for the delay replying, I've been on vacation and travelling. > > > 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), > > Unfortunately this will break the API. specifically existing apps which > call libxl_defbool_set on this field will now get a type error. > I wasn't sure what was acceptable breakage; there's precedent for changing types with bootloader_args but maybe that came before stabilization of the API. > You need to keep the existing viridian field and add a new one with the > new semantics and a new name (e.g. viridian_features, or a set of new > defbools as I suggested in my previous mail) and define the precedence. > See usbdevice vs. usbdevice_list or the (very new) serial one for > existing examples of this sort of thing. > Ok, so I could deprecate the viridian defbool but leave it in place, then add a new list e.g. viridian_enlightenments. Any app setting the viridian defbool then implies an enlightenment list of [base, freq] otherwise, if the defbool is not set, libxl uses the actual viridian_enlightenment list. > For your case though you could continue to treat the viridian defbool as > a gate on a bunch of viridian_feature_X defbools, IOW if viridian == > false the others are ignored, if it is true then the others have the > usual defbool behaviour of defaults and overrides. I think that would > fall out fairly naturally in the setdefault function. > I actually implemented that at first but it means that, with every new enlightenment, the build info structure has to grow a new defbool. That seems messy, which is why I went for the string list. I've now implemented an integer list type and enlightenment enum due to Ian J's objection about libxl dealing with strings. Hopefully that will be acceptable... Patch coming later today, hopefully. Paul > Ian. > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v5 2/3] x86/viridian: Make logging less verbose 2014-08-05 13:07 [PATCH v5 0/3] x86/viridian improvements Paul Durrant 2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant @ 2014-08-05 13:07 ` Paul Durrant 2014-08-05 13:07 ` [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant 2014-08-12 10:16 ` [PATCH v5 0/3] x86/viridian improvements Paul Durrant 3 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2014-08-05 13:07 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant, Keir Fraser, Jan Beulich The use of gdprintk() adds uninteresting prefixes to the log lines, and there's really too many lines. This patch reduces the verbosity. 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 | 51 ++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c index 31c9656..6fcfa96 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -120,37 +120,42 @@ 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", - d->arch.hvm_domain.viridian.guest_os_id.fields.vendor); - gdprintk(XENLOG_INFO, "\tos: %x\n", - d->arch.hvm_domain.viridian.guest_os_id.fields.os); - gdprintk(XENLOG_INFO, "\tmajor: %x\n", - d->arch.hvm_domain.viridian.guest_os_id.fields.major); - gdprintk(XENLOG_INFO, "\tminor: %x\n", - d->arch.hvm_domain.viridian.guest_os_id.fields.minor); - gdprintk(XENLOG_INFO, "\tsp: %x\n", - d->arch.hvm_domain.viridian.guest_os_id.fields.service_pack); - gdprintk(XENLOG_INFO, "\tbuild: %x\n", - d->arch.hvm_domain.viridian.guest_os_id.fields.build_number); + const union viridian_guest_os_id *goi; + + goi = &d->arch.hvm_domain.viridian.guest_os_id; + + printk(XENLOG_G_INFO "d%d: VIRIDIAN GUEST_OS_ID: vendor: %x os: %x major: %x minor: %x sp: %x build: %x\n", + d->domain_id, + goi->fields.vendor, + goi->fields.os, + goi->fields.major, + goi->fields.minor, + goi->fields.service_pack, + goi->fields.build_number); } static void dump_hypercall(const struct domain *d) { - gdprintk(XENLOG_INFO, "HYPERCALL:\n"); - gdprintk(XENLOG_INFO, "\tenabled: %x\n", - d->arch.hvm_domain.viridian.hypercall_gpa.fields.enabled); - gdprintk(XENLOG_INFO, "\tpfn: %lx\n", - (unsigned long)d->arch.hvm_domain.viridian.hypercall_gpa.fields.pfn); + const union viridian_hypercall_gpa *hg; + + hg = &d->arch.hvm_domain.viridian.hypercall_gpa; + + printk(XENLOG_G_INFO "d%d: VIRIDIAN HYPERCALL: enabled: %x pfn: %lx\n", + d->domain_id, + hg->fields.enabled, + (unsigned long)hg->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", - v->arch.hvm_vcpu.viridian.apic_assist.fields.enabled); - gdprintk(XENLOG_INFO, "\tpfn: %lx\n", - (unsigned long)v->arch.hvm_vcpu.viridian.apic_assist.fields.pfn); + const union viridian_apic_assist *aa; + + aa = &v->arch.hvm_vcpu.viridian.apic_assist; + + printk(XENLOG_G_INFO "%pv: VIRIDIAN APIC_ASSIST: enabled: %x pfn: %lx\n", + v, + aa->fields.enabled, + (unsigned long)aa->fields.pfn); } static void enable_hypercall_page(struct domain *d) -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support 2014-08-05 13:07 [PATCH v5 0/3] x86/viridian improvements Paul Durrant 2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant 2014-08-05 13:07 ` [PATCH v5 2/3] x86/viridian: Make logging less verbose Paul Durrant @ 2014-08-05 13:07 ` Paul Durrant 2014-08-12 15:27 ` Ian Jackson 2014-08-13 15:27 ` Egger, Christoph 2014-08-12 10:16 ` [PATCH v5 0/3] x86/viridian improvements Paul Durrant 3 siblings, 2 replies; 16+ messages in thread From: Paul Durrant @ 2014-08-05 13:07 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. This implementation uses the tsc of vcpu0, which is preserved across save/restore as part of the architectural state, and then converts that to a 100ns tick using the domain's tsc_khz. Signed-off-by: Paul Durrant <paul.durrant@citrix.com> Cc: Keir Fraser <keir@xen.org> 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/viridian.c | 24 +++++++++++++++++++----- xen/include/asm-x86/perfc_defn.h | 1 + xen/include/public/hvm/params.h | 9 ++++++++- 5 files changed, 37 insertions(+), 6 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 cc57931..224a4d9 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/viridian.c b/xen/arch/x86/hvm/viridian.c index 6fcfa96..5e99af2 100644 --- a/xen/arch/x86/hvm/viridian.c +++ b/xen/arch/x86/hvm/viridian.c @@ -36,11 +36,11 @@ #define HvNotifyLongSpinWait 8 /* Viridian CPUID 4000003, Viridian MSR availability. */ -#define CPUID3A_MSR_REF_COUNT (1 << 1) -#define CPUID3A_MSR_APIC_ACCESS (1 << 4) -#define CPUID3A_MSR_HYPERCALL (1 << 5) -#define CPUID3A_MSR_VP_INDEX (1 << 6) -#define CPUID3A_MSR_FREQ (1 << 11) +#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1) +#define CPUID3A_MSR_APIC_ACCESS (1 << 4) +#define CPUID3A_MSR_HYPERCALL (1 << 5) +#define CPUID3A_MSR_VP_INDEX (1 << 6) +#define CPUID3A_MSR_FREQ (1 << 11) /* Viridian CPUID 4000004, Implementation Recommendations. */ #define CPUID4A_MSR_BASED_APIC (1 << 3) @@ -93,6 +93,8 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax, CPUID3A_MSR_VP_INDEX); if ( !(viridian_feature_mask(d) & HVMPV_no_freq) ) *eax |= CPUID3A_MSR_FREQ; + if ( viridian_feature_mask(d) & HVMPV_time_ref_count ) + *eax |= CPUID3A_MSR_TIME_REF_COUNT; break; case 4: /* Recommended hypercall usage. */ @@ -349,6 +351,18 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val) *val = v->arch.hvm_vcpu.viridian.apic_assist.raw; break; + case VIRIDIAN_MSR_TIME_REF_COUNT: + { + uint64_t tsc; + + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) ) + return 0; + + perfc_incr(mshv_rdmsr_time_ref_count); + tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d)); + *val = (tsc * 10000ull) / d->arch.tsc_khz; + break; + } default: return 0; } 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/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] 16+ messages in thread
* Re: [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support 2014-08-05 13:07 ` [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant @ 2014-08-12 15:27 ` Ian Jackson 2014-08-13 15:27 ` Egger, Christoph 1 sibling, 0 replies; 16+ messages in thread From: Ian Jackson @ 2014-08-12 15:27 UTC (permalink / raw) To: Paul Durrant Cc: Stefano Stabellini, Keir Fraser, Ian Campbell, Jan Beulich, xen-devel Paul Durrant writes ("[PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support"): > This patch optionally re-instates support for the partition time reference > counter that was previously introduced by commit > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b and reverted by commit > 1cd4fab14ce25859efa4a2af13475e6650a5506c. The previous implementation was > non-optional and flawed. > > This implementation uses the tsc of vcpu0, which is preserved across > save/restore as part of the architectural state, and then converts that > to a 100ns tick using the domain's tsc_khz. The tools part of this is fine except that it does the string to number conversion in libxl rather than xl, as I have just observed wrt 1/3. Thanks, Ian. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support 2014-08-05 13:07 ` [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant 2014-08-12 15:27 ` Ian Jackson @ 2014-08-13 15:27 ` Egger, Christoph 2014-08-27 14:04 ` Paul Durrant 1 sibling, 1 reply; 16+ messages in thread From: Egger, Christoph @ 2014-08-13 15:27 UTC (permalink / raw) To: Paul Durrant, xen-devel Cc: Ian Jackson, Keir Fraser, Ian Campbell, Jan Beulich, Stefano Stabellini On 05.08.14 15:07, Paul Durrant wrote: > This patch optionally re-instates support for the partition time reference > counter that was previously introduced by commit > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b and reverted by commit > 1cd4fab14ce25859efa4a2af13475e6650a5506c. The previous implementation was > non-optional and flawed. > > This implementation uses the tsc of vcpu0, which is preserved across > save/restore as part of the architectural state, and then converts that > to a 100ns tick using the domain's tsc_khz. > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > Cc: Keir Fraser <keir@xen.org> > 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/viridian.c | 24 +++++++++++++++++++----- > xen/include/asm-x86/perfc_defn.h | 1 + > xen/include/public/hvm/params.h | 9 ++++++++- > 5 files changed, 37 insertions(+), 6 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 cc57931..224a4d9 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/viridian.c b/xen/arch/x86/hvm/viridian.c > index 6fcfa96..5e99af2 100644 > --- a/xen/arch/x86/hvm/viridian.c > +++ b/xen/arch/x86/hvm/viridian.c > @@ -36,11 +36,11 @@ > #define HvNotifyLongSpinWait 8 > > /* Viridian CPUID 4000003, Viridian MSR availability. */ > -#define CPUID3A_MSR_REF_COUNT (1 << 1) > -#define CPUID3A_MSR_APIC_ACCESS (1 << 4) > -#define CPUID3A_MSR_HYPERCALL (1 << 5) > -#define CPUID3A_MSR_VP_INDEX (1 << 6) > -#define CPUID3A_MSR_FREQ (1 << 11) > +#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1) > +#define CPUID3A_MSR_APIC_ACCESS (1 << 4) > +#define CPUID3A_MSR_HYPERCALL (1 << 5) > +#define CPUID3A_MSR_VP_INDEX (1 << 6) > +#define CPUID3A_MSR_FREQ (1 << 11) > > /* Viridian CPUID 4000004, Implementation Recommendations. */ > #define CPUID4A_MSR_BASED_APIC (1 << 3) > @@ -93,6 +93,8 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned int *eax, > CPUID3A_MSR_VP_INDEX); > if ( !(viridian_feature_mask(d) & HVMPV_no_freq) ) > *eax |= CPUID3A_MSR_FREQ; > + if ( viridian_feature_mask(d) & HVMPV_time_ref_count ) > + *eax |= CPUID3A_MSR_TIME_REF_COUNT; > break; > case 4: > /* Recommended hypercall usage. */ > @@ -349,6 +351,18 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val) > *val = v->arch.hvm_vcpu.viridian.apic_assist.raw; > break; > > + case VIRIDIAN_MSR_TIME_REF_COUNT: > + { > + uint64_t tsc; > + > + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) ) > + return 0; > + > + perfc_incr(mshv_rdmsr_time_ref_count); > + tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d)); > + *val = (tsc * 10000ull) / d->arch.tsc_khz; This is correct by math but not in implementation. You need to stay within 64bits. Please add the math formula in a comment. Christoph > + break; > + } > default: > return 0; > } > 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/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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support 2014-08-13 15:27 ` Egger, Christoph @ 2014-08-27 14:04 ` Paul Durrant 0 siblings, 0 replies; 16+ messages in thread From: Paul Durrant @ 2014-08-27 14:04 UTC (permalink / raw) To: Egger, Christoph, xen-devel Cc: Stefano Stabellini, Keir (Xen.org), Ian Campbell, Jan Beulich, Ian Jackson > -----Original Message----- > From: Egger, Christoph [mailto:chegger@amazon.de] > Sent: 13 August 2014 16:28 > 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 v5 3/3] x86/viridian: Add partition time > reference counter MSR support > > On 05.08.14 15:07, Paul Durrant wrote: > > This patch optionally re-instates support for the partition time reference > > counter that was previously introduced by commit > > e36cd2cdc9674a7a4855d21fb7b3e6e17c4bb33b and reverted by commit > > 1cd4fab14ce25859efa4a2af13475e6650a5506c. The previous > implementation was > > non-optional and flawed. > > > > This implementation uses the tsc of vcpu0, which is preserved across > > save/restore as part of the architectural state, and then converts that > > to a 100ns tick using the domain's tsc_khz. > > > > Signed-off-by: Paul Durrant <paul.durrant@citrix.com> > > Cc: Keir Fraser <keir@xen.org> > > 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/viridian.c | 24 +++++++++++++++++++----- > > xen/include/asm-x86/perfc_defn.h | 1 + > > xen/include/public/hvm/params.h | 9 ++++++++- > > 5 files changed, 37 insertions(+), 6 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 cc57931..224a4d9 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/viridian.c b/xen/arch/x86/hvm/viridian.c > > index 6fcfa96..5e99af2 100644 > > --- a/xen/arch/x86/hvm/viridian.c > > +++ b/xen/arch/x86/hvm/viridian.c > > @@ -36,11 +36,11 @@ > > #define HvNotifyLongSpinWait 8 > > > > /* Viridian CPUID 4000003, Viridian MSR availability. */ > > -#define CPUID3A_MSR_REF_COUNT (1 << 1) > > -#define CPUID3A_MSR_APIC_ACCESS (1 << 4) > > -#define CPUID3A_MSR_HYPERCALL (1 << 5) > > -#define CPUID3A_MSR_VP_INDEX (1 << 6) > > -#define CPUID3A_MSR_FREQ (1 << 11) > > +#define CPUID3A_MSR_TIME_REF_COUNT (1 << 1) > > +#define CPUID3A_MSR_APIC_ACCESS (1 << 4) > > +#define CPUID3A_MSR_HYPERCALL (1 << 5) > > +#define CPUID3A_MSR_VP_INDEX (1 << 6) > > +#define CPUID3A_MSR_FREQ (1 << 11) > > > > /* Viridian CPUID 4000004, Implementation Recommendations. */ > > #define CPUID4A_MSR_BASED_APIC (1 << 3) > > @@ -93,6 +93,8 @@ int cpuid_viridian_leaves(unsigned int leaf, unsigned > int *eax, > > CPUID3A_MSR_VP_INDEX); > > if ( !(viridian_feature_mask(d) & HVMPV_no_freq) ) > > *eax |= CPUID3A_MSR_FREQ; > > + if ( viridian_feature_mask(d) & HVMPV_time_ref_count ) > > + *eax |= CPUID3A_MSR_TIME_REF_COUNT; > > break; > > case 4: > > /* Recommended hypercall usage. */ > > @@ -349,6 +351,18 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val) > > *val = v->arch.hvm_vcpu.viridian.apic_assist.raw; > > break; > > > > + case VIRIDIAN_MSR_TIME_REF_COUNT: > > + { > > + uint64_t tsc; > > + > > + if ( !(viridian_feature_mask(d) & HVMPV_time_ref_count) ) > > + return 0; > > + > > + perfc_incr(mshv_rdmsr_time_ref_count); > > + tsc = hvm_get_guest_tsc(pt_global_vcpu_target(d)); > > + *val = (tsc * 10000ull) / d->arch.tsc_khz; > > This is correct by math but not in implementation. > You need to stay within 64bits. Please add the math formula > in a comment. > Good point. I'll use the tsc scale code to do the calculation. Paul > Christoph > > > + break; > > + } > > default: > > return 0; > > } > > 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/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 > > > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 0/3] x86/viridian improvements 2014-08-05 13:07 [PATCH v5 0/3] x86/viridian improvements Paul Durrant ` (2 preceding siblings ...) 2014-08-05 13:07 ` [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant @ 2014-08-12 10:16 ` Paul Durrant 2014-08-12 10:38 ` Jan Beulich 3 siblings, 1 reply; 16+ messages in thread From: Paul Durrant @ 2014-08-12 10:16 UTC (permalink / raw) To: xen-devel; +Cc: Paul Durrant Ping? > -----Original Message----- > From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > bounces@lists.xen.org] On Behalf Of Paul Durrant > Sent: 05 August 2014 14:07 > To: xen-devel@lists.xen.org > Subject: [Xen-devel] [PATCH v5 0/3] x86/viridian improvements > > This patch series incorporates several improvements to the code > supporting viridian (i.e. hyper-v compatible) enlightenments for > Windows guests: > > Patch #1 series lays the foundations for adding new viridian > enlightenments such that they can be optionally enabled, and not > immediately exposed to a guest across a save/restore boundary. > > Patch #2 makes logging less verbose > > Patch #3 adds support for the 'Partition Time Reference Counter' > enlightenment. > > v2: > - Addressed comments from Jan Beulich > - Added patch #2 > > v3: > - Addressed comments from Andrew Cooper and Jan Beulich > - Re-worked patch #2 > - Simplified patch #3 to use guest TSC > > v4: > - Added missing domain info to printks in patch #2 > > v5: > - Clarified comment of patch #1 as suggested by David Vrabel > - More logging tweaks in patch #2 as suggested by Andrew Cooper > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 0/3] x86/viridian improvements 2014-08-12 10:16 ` [PATCH v5 0/3] x86/viridian improvements Paul Durrant @ 2014-08-12 10:38 ` Jan Beulich 2014-08-12 11:49 ` Paul Durrant 0 siblings, 1 reply; 16+ messages in thread From: Jan Beulich @ 2014-08-12 10:38 UTC (permalink / raw) To: Paul Durrant; +Cc: xen-devel >>> On 12.08.14 at 12:16, <Paul.Durrant@citrix.com> wrote: > Ping? I'm afraid just pinging the list rather than specific people not responding won't have much of an effect. Jan >> -----Original Message----- >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- >> bounces@lists.xen.org] On Behalf Of Paul Durrant >> Sent: 05 August 2014 14:07 >> To: xen-devel@lists.xen.org >> Subject: [Xen-devel] [PATCH v5 0/3] x86/viridian improvements >> >> This patch series incorporates several improvements to the code >> supporting viridian (i.e. hyper-v compatible) enlightenments for >> Windows guests: >> >> Patch #1 series lays the foundations for adding new viridian >> enlightenments such that they can be optionally enabled, and not >> immediately exposed to a guest across a save/restore boundary. >> >> Patch #2 makes logging less verbose >> >> Patch #3 adds support for the 'Partition Time Reference Counter' >> enlightenment. >> >> v2: >> - Addressed comments from Jan Beulich >> - Added patch #2 >> >> v3: >> - Addressed comments from Andrew Cooper and Jan Beulich >> - Re-worked patch #2 >> - Simplified patch #3 to use guest TSC >> >> v4: >> - Added missing domain info to printks in patch #2 >> >> v5: >> - Clarified comment of patch #1 as suggested by David Vrabel >> - More logging tweaks in patch #2 as suggested by Andrew Cooper >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 0/3] x86/viridian improvements 2014-08-12 10:38 ` Jan Beulich @ 2014-08-12 11:49 ` Paul Durrant 2014-08-12 12:17 ` Jan Beulich 0 siblings, 1 reply; 16+ messages in thread From: Paul Durrant @ 2014-08-12 11:49 UTC (permalink / raw) To: Jan Beulich, Keir (Xen.org), Ian Campbell, Stefano Stabellini, Ian Jackson Cc: xen-devel > -----Original Message----- > From: Jan Beulich [mailto:JBeulich@suse.com] > Sent: 12 August 2014 11:39 > To: Paul Durrant > Cc: xen-devel@lists.xen.org > Subject: Re: [Xen-devel] [PATCH v5 0/3] x86/viridian improvements > > >>> On 12.08.14 at 12:16, <Paul.Durrant@citrix.com> wrote: > > Ping? > > I'm afraid just pinging the list rather than specific people not responding > won't have much of an effect. > Ok. In that case, I need acks from a Xen maintainer for all patches (Jan or Keir), but patches #1 and #3 need acks from a toolstack maintainer (Stefano, Ian J or Ian C). Paul > Jan > > >> -----Original Message----- > >> From: xen-devel-bounces@lists.xen.org [mailto:xen-devel- > >> bounces@lists.xen.org] On Behalf Of Paul Durrant > >> Sent: 05 August 2014 14:07 > >> To: xen-devel@lists.xen.org > >> Subject: [Xen-devel] [PATCH v5 0/3] x86/viridian improvements > >> > >> This patch series incorporates several improvements to the code > >> supporting viridian (i.e. hyper-v compatible) enlightenments for > >> Windows guests: > >> > >> Patch #1 series lays the foundations for adding new viridian > >> enlightenments such that they can be optionally enabled, and not > >> immediately exposed to a guest across a save/restore boundary. > >> > >> Patch #2 makes logging less verbose > >> > >> Patch #3 adds support for the 'Partition Time Reference Counter' > >> enlightenment. > >> > >> v2: > >> - Addressed comments from Jan Beulich > >> - Added patch #2 > >> > >> v3: > >> - Addressed comments from Andrew Cooper and Jan Beulich > >> - Re-worked patch #2 > >> - Simplified patch #3 to use guest TSC > >> > >> v4: > >> - Added missing domain info to printks in patch #2 > >> > >> v5: > >> - Clarified comment of patch #1 as suggested by David Vrabel > >> - More logging tweaks in patch #2 as suggested by Andrew Cooper > >> > >> > >> _______________________________________________ > >> Xen-devel mailing list > >> Xen-devel@lists.xen.org > >> http://lists.xen.org/xen-devel > > > > _______________________________________________ > > Xen-devel mailing list > > Xen-devel@lists.xen.org > > http://lists.xen.org/xen-devel > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v5 0/3] x86/viridian improvements 2014-08-12 11:49 ` Paul Durrant @ 2014-08-12 12:17 ` Jan Beulich 0 siblings, 0 replies; 16+ messages in thread From: Jan Beulich @ 2014-08-12 12:17 UTC (permalink / raw) To: Paul Durrant Cc: Ian Jackson, Keir (Xen.org), Stefano Stabellini, IanCampbell, xen-devel >>> On 12.08.14 at 13:49, <Paul.Durrant@citrix.com> wrote: > In that case, I need acks from a Xen maintainer for all patches (Jan or > Keir), but patches #1 and #3 need acks from a toolstack maintainer (Stefano, > Ian J or Ian C). As you may have seen, patch 2 went in already. Since the core of the change is really in the hypervisor, I'm expecting to commit the series, and considering that among the people above it was mainly me having commented on the various versions plus you having taken care of all the issues I had pointed out, my going silent can also be interpreted as me just waiting for other comments/acks in order for the series to go in. But yes, from a formal perspective the hypervisor parts pf patches 1 and 3 are Acked-by: Jan Beulich <jbeulich@suse.com> Jan ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2014-08-27 14:04 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2014-08-05 13:07 [PATCH v5 0/3] x86/viridian improvements Paul Durrant 2014-08-05 13:07 ` [PATCH v5 1/3] x86/viridian: Re-purpose the HVM parameter to be a feature mask Paul Durrant 2014-08-12 15:25 ` Ian Jackson 2014-08-13 11:09 ` Paul Durrant 2014-08-26 20:24 ` Ian Campbell 2014-08-26 20:28 ` Ian Campbell 2014-08-27 8:11 ` Paul Durrant 2014-08-05 13:07 ` [PATCH v5 2/3] x86/viridian: Make logging less verbose Paul Durrant 2014-08-05 13:07 ` [PATCH v5 3/3] x86/viridian: Add partition time reference counter MSR support Paul Durrant 2014-08-12 15:27 ` Ian Jackson 2014-08-13 15:27 ` Egger, Christoph 2014-08-27 14:04 ` Paul Durrant 2014-08-12 10:16 ` [PATCH v5 0/3] x86/viridian improvements Paul Durrant 2014-08-12 10:38 ` Jan Beulich 2014-08-12 11:49 ` Paul Durrant 2014-08-12 12:17 ` Jan Beulich
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.