* [PATCH 0/7] x86/viridian updates
@ 2017-03-17 9:57 Paul Durrant
2017-03-17 9:57 ` [PATCH 1/7] x86/viridian: update to version 5.0a of the specification Paul Durrant
` (6 more replies)
0 siblings, 7 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Paul Durrant
Paul Durrant (7):
x86/viridian: update to version 5.0a of the specification
x86/viridian: fix xen-hvmcrash when vp_assist page is present
x86/viridian: don't put Xen version information in CPUID leaf 2
x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2
x86/viridian: add warnings for unimplemented hypercalls and MSRs
x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
x86/viridian: implement the crash MSRs
docs/man/xl.cfg.pod.5.in | 10 +-
docs/misc/xen-command-line.markdown | 8 +
tools/libxl/libxl.h | 6 +
tools/libxl/libxl_dom.c | 4 +
tools/libxl/libxl_types.idl | 1 +
tools/misc/xen-hvmctx.c | 6 +-
xen/arch/x86/hvm/viridian.c | 461 ++++++++++++++++++++++++---------
xen/include/asm-x86/hvm/viridian.h | 8 +-
xen/include/public/arch-x86/hvm/save.h | 4 +-
xen/include/public/hvm/params.h | 7 +-
10 files changed, 381 insertions(+), 134 deletions(-)
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* [PATCH 1/7] x86/viridian: update to version 5.0a of the specification
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 11:27 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present Paul Durrant
` (5 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Paul Durrant, Ian Jackson, Jan Beulich, Andrew Cooper
The Hypervisor Top Level Functional Specification v5.0a has many differences
from previous versions and introduces whole new sections.
This patch:
- Updates the URL at the top of the source.
- Fixes up section references accordingly.
- Modifies the MSR naming convention in the code to match the specification.
- Rename the apic_assist page to the vp_assist page to reflect the change
in the specification.
(The APIC assist feature itself is inconsistently named in the
specification so stick wth the current feature name).
- Updates the handling of CPUID leaf 3.
There is one functional change in this patch: The vp_assist page is
mapped (and completely zeroed) regardless of whether the APIC assist
feature is enabled. This reflects its new wider remit and simplifies the
code slightly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
tools/misc/xen-hvmctx.c | 6 +-
xen/arch/x86/hvm/viridian.c | 250 ++++++++++++++++++++-------------
xen/include/asm-x86/hvm/viridian.h | 6 +-
xen/include/public/arch-x86/hvm/save.h | 4 +-
4 files changed, 158 insertions(+), 108 deletions(-)
diff --git a/tools/misc/xen-hvmctx.c b/tools/misc/xen-hvmctx.c
index 32be120..bde41f3 100644
--- a/tools/misc/xen-hvmctx.c
+++ b/tools/misc/xen-hvmctx.c
@@ -379,9 +379,9 @@ static void dump_viridian_vcpu(void)
{
HVM_SAVE_TYPE(VIRIDIAN_VCPU) p;
READ(p);
- printf(" VIRIDIAN_VCPU: apic_assist_msr 0x%llx, apic_assist_vector 0x%x\n",
- (unsigned long long) p.apic_assist_msr,
- p.apic_assist_vector);
+ printf(" VIRIDIAN_VCPU: vp_assist_msr 0x%llx, vp_assist_vector 0x%x\n",
+ (unsigned long long) p.vp_assist_msr,
+ p.vp_assist_vector);
}
static void dump_vmce_vcpu(void)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 8aca4dd..d741e81 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -2,9 +2,9 @@
* viridian.c
*
* An implementation of some Viridian enlightenments. See Microsoft's
- * Hypervisor Top Level Functional Specification (v4.0b) at:
+ * Hypervisor Top Level Functional Specification (v5.0a) at:
*
- * https://msdn.microsoft.com/en-us/virtualization/hyperv_on_windows/develop/tlfs
+ * https://github.com/Microsoft/Virtualization-Documentation/raw/master/tlfs/Hypervisor%20Top%20Level%20Functional%20Specification%20v5.0.pdf
*
* for more information.
*/
@@ -23,17 +23,17 @@
#include <public/hvm/hvm_op.h>
/* Viridian MSR numbers. */
-#define VIRIDIAN_MSR_GUEST_OS_ID 0x40000000
-#define VIRIDIAN_MSR_HYPERCALL 0x40000001
-#define VIRIDIAN_MSR_VP_INDEX 0x40000002
-#define VIRIDIAN_MSR_TIME_REF_COUNT 0x40000020
-#define VIRIDIAN_MSR_REFERENCE_TSC 0x40000021
-#define VIRIDIAN_MSR_TSC_FREQUENCY 0x40000022
-#define VIRIDIAN_MSR_APIC_FREQUENCY 0x40000023
-#define VIRIDIAN_MSR_EOI 0x40000070
-#define VIRIDIAN_MSR_ICR 0x40000071
-#define VIRIDIAN_MSR_TPR 0x40000072
-#define VIRIDIAN_MSR_APIC_ASSIST 0x40000073
+#define HV_X64_MSR_GUEST_OS_ID 0x40000000
+#define HV_X64_MSR_HYPERCALL 0x40000001
+#define HV_X64_MSR_VP_INDEX 0x40000002
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+#define HV_X64_MSR_REFERENCE_TSC 0x40000021
+#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
+#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
+#define HV_X64_MSR_EOI 0x40000070
+#define HV_X64_MSR_ICR 0x40000071
+#define HV_X64_MSR_TPR 0x40000072
+#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073
/* Viridian Hypercall Status Codes. */
#define HV_STATUS_SUCCESS 0x0000
@@ -48,20 +48,60 @@
/* Viridian Hypercall Flags. */
#define HV_FLUSH_ALL_PROCESSORS 1
-/* Viridian CPUID 4000003, Viridian MSR availability. */
-#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_REFERENCE_TSC (1 << 9)
-#define CPUID3A_MSR_FREQ (1 << 11)
-
-/* Viridian CPUID 4000004, Implementation Recommendations. */
+/*
+ * Viridian Partition Privilege Flags.
+ *
+ * This is taken from section 4.2.2 of the specification, and fixed for
+ * style and correctness.
+ */
+typedef struct {
+ /* Access to virtual MSRs */
+ uint64_t AccessVpRunTimeReg:1;
+ uint64_t AccessPartitionReferenceCounter:1;
+ uint64_t AccessSynicRegs:1;
+ uint64_t AccessSyntheticTimerRegs:1;
+ uint64_t AccessIntrCtrlRegs:1;
+ uint64_t AccessHypercallMsrs:1;
+ uint64_t AccessVpIndex:1;
+ uint64_t AccessResetReg:1;
+ uint64_t AccessStatsReg:1;
+ uint64_t AccessPartitionReferenceTsc:1;
+ uint64_t AccessGuestIdleReg:1;
+ uint64_t AccessFrequencyRegs:1;
+ uint64_t AccessDebugRegs:1;
+ uint64_t Reserved1:19;
+
+ /* Access to hypercalls */
+ uint64_t CreatePartitions:1;
+ uint64_t AccessPartitionId:1;
+ uint64_t AccessMemoryPool:1;
+ uint64_t AdjustMessageBuffers:1;
+ uint64_t PostMessages:1;
+ uint64_t SignalEvents:1;
+ uint64_t CreatePort:1;
+ uint64_t ConnectPort:1;
+ uint64_t AccessStats:1;
+ uint64_t Reserved2:2;
+ uint64_t Debugging:1;
+ uint64_t CpuManagement:1;
+ uint64_t Reserved3:1;
+ uint64_t Reserved4:1;
+ uint64_t Reserved5:1;
+ uint64_t AccessVSM:1;
+ uint64_t AccessVpRegisters:1;
+ uint64_t Reserved6:1;
+ uint64_t Reserved7:1;
+ uint64_t EnableExtendedHypercalls:1;
+ uint64_t StartVirtualProcessor:1;
+ uint64_t Reserved8:10;
+} HV_PARTITION_PRIVILEGE_MASK;
+
+/* Viridian CPUID leaf 4: Implementation Recommendations. */
#define CPUID4A_HCALL_REMOTE_TLB_FLUSH (1 << 2)
#define CPUID4A_MSR_BASED_APIC (1 << 3)
#define CPUID4A_RELAX_TIMER_INT (1 << 5)
-/* Viridian CPUID 4000006, Implementation HW features detected and in use. */
+/* Viridian CPUID leaf 6: Implementation HW features detected and in use. */
#define CPUID6A_APIC_OVERLAY (1 << 0)
#define CPUID6A_MSR_BITMAPS (1 << 1)
#define CPUID6A_NESTED_PAGING (1 << 3)
@@ -101,17 +141,35 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
break;
case 3:
- /* Which hypervisor MSRs are available to the guest */
- res->a = (CPUID3A_MSR_APIC_ACCESS |
- CPUID3A_MSR_HYPERCALL |
- CPUID3A_MSR_VP_INDEX);
+ {
+ /*
+ * Section 2.4.4 details this leaf and states that EAX and EBX
+ * are defined to the the low and high parts of the partition
+ * privilege mask respectively.
+ */
+ HV_PARTITION_PRIVILEGE_MASK mask = {
+ .AccessIntrCtrlRegs = 1,
+ .AccessHypercallMsrs = 1,
+ .AccessVpIndex = 1,
+ };
+ union {
+ HV_PARTITION_PRIVILEGE_MASK mask;
+ uint32_t lo, hi;
+ } u;
+
if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
- res->a |= CPUID3A_MSR_FREQ;
+ mask.AccessFrequencyRegs = 1;
if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
- res->a |= CPUID3A_MSR_TIME_REF_COUNT;
+ mask.AccessPartitionReferenceCounter = 1;
if ( viridian_feature_mask(d) & HVMPV_reference_tsc )
- res->a |= CPUID3A_MSR_REFERENCE_TSC;
+ mask.AccessPartitionReferenceTsc = 1;
+
+ u.mask = mask;
+
+ res->a = u.lo;
+ res->b = u.hi;
break;
+ }
case 4:
/* Recommended hypercall usage. */
@@ -163,14 +221,14 @@ static void dump_hypercall(const struct domain *d)
hg->fields.enabled, (unsigned long)hg->fields.pfn);
}
-static void dump_apic_assist(const struct vcpu *v)
+static void dump_vp_assist(const struct vcpu *v)
{
- const union viridian_apic_assist *aa;
+ const union viridian_vp_assist *va;
- aa = &v->arch.hvm_vcpu.viridian.apic_assist.msr;
+ va = &v->arch.hvm_vcpu.viridian.vp_assist.msr;
- printk(XENLOG_G_INFO "%pv: VIRIDIAN APIC_ASSIST: enabled: %x pfn: %lx\n",
- v, aa->fields.enabled, (unsigned long)aa->fields.pfn);
+ printk(XENLOG_G_INFO "%pv: VIRIDIAN VP_ASSIST_PAGE: enabled: %x pfn: %lx\n",
+ v, va->fields.enabled, (unsigned long)va->fields.pfn);
}
static void dump_reference_tsc(const struct domain *d)
@@ -218,15 +276,15 @@ static void enable_hypercall_page(struct domain *d)
put_page_and_type(page);
}
-static void initialize_apic_assist(struct vcpu *v)
+static void initialize_vp_assist(struct vcpu *v)
{
struct domain *d = v->domain;
- unsigned long gmfn = v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.pfn;
+ unsigned long gmfn = v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.pfn;
struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
void *va;
/*
- * See section 13.3.4.1 of the specification for details of this
+ * See section 7.8.7 of the specification for details of this
* enlightenment.
*/
@@ -246,24 +304,17 @@ static void initialize_apic_assist(struct vcpu *v)
goto fail;
}
- *(uint32_t *)va = 0;
-
- if ( viridian_feature_mask(v->domain) & HVMPV_apic_assist )
- {
- /*
- * If we overwrite an existing address here then something has
- * gone wrong and a domain page will leak. Instead crash the
- * domain to make the problem obvious.
- */
- if ( v->arch.hvm_vcpu.viridian.apic_assist.va )
- domain_crash(d);
+ clear_page(va);
- v->arch.hvm_vcpu.viridian.apic_assist.va = va;
- return;
- }
+ /*
+ * If we overwrite an existing address here then something has
+ * gone wrong and a domain page will leak. Instead crash the
+ * domain to make the problem obvious.
+ */
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
+ domain_crash(d);
- unmap_domain_page_global(va);
- put_page_and_type(page);
+ v->arch.hvm_vcpu.viridian.vp_assist.va = va;
return;
fail:
@@ -271,15 +322,15 @@ static void initialize_apic_assist(struct vcpu *v)
page ? page_to_mfn(page) : mfn_x(INVALID_MFN));
}
-static void teardown_apic_assist(struct vcpu *v)
+static void teardown_vp_assist(struct vcpu *v)
{
- void *va = v->arch.hvm_vcpu.viridian.apic_assist.va;
+ void *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
struct page_info *page;
if ( !va )
return;
- v->arch.hvm_vcpu.viridian.apic_assist.va = NULL;
+ v->arch.hvm_vcpu.viridian.vp_assist.va = NULL;
page = mfn_to_page(domain_page_map_to_mfn(va));
@@ -289,7 +340,7 @@ static void teardown_apic_assist(struct vcpu *v)
void viridian_start_apic_assist(struct vcpu *v, int vector)
{
- uint32_t *va = v->arch.hvm_vcpu.viridian.apic_assist.va;
+ uint32_t *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
if ( !va )
return;
@@ -302,16 +353,16 @@ void viridian_start_apic_assist(struct vcpu *v, int vector)
* wrong and the VM will most likely hang so force a crash now
* to make the problem clear.
*/
- if ( v->arch.hvm_vcpu.viridian.apic_assist.vector )
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.vector )
domain_crash(v->domain);
- v->arch.hvm_vcpu.viridian.apic_assist.vector = vector;
+ v->arch.hvm_vcpu.viridian.vp_assist.vector = vector;
*va |= 1u;
}
int viridian_complete_apic_assist(struct vcpu *v)
{
- uint32_t *va = v->arch.hvm_vcpu.viridian.apic_assist.va;
+ uint32_t *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
int vector;
if ( !va )
@@ -320,21 +371,21 @@ int viridian_complete_apic_assist(struct vcpu *v)
if ( *va & 1u )
return 0; /* Interrupt not yet processed by the guest. */
- vector = v->arch.hvm_vcpu.viridian.apic_assist.vector;
- v->arch.hvm_vcpu.viridian.apic_assist.vector = 0;
+ vector = v->arch.hvm_vcpu.viridian.vp_assist.vector;
+ v->arch.hvm_vcpu.viridian.vp_assist.vector = 0;
return vector;
}
void viridian_abort_apic_assist(struct vcpu *v)
{
- uint32_t *va = v->arch.hvm_vcpu.viridian.apic_assist.va;
+ uint32_t *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
if ( !va )
return;
*va &= ~1u;
- v->arch.hvm_vcpu.viridian.apic_assist.vector = 0;
+ v->arch.hvm_vcpu.viridian.vp_assist.vector = 0;
}
static void update_reference_tsc(struct domain *d, bool_t initialize)
@@ -420,13 +471,13 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
switch ( idx )
{
- case VIRIDIAN_MSR_GUEST_OS_ID:
+ case HV_X64_MSR_GUEST_OS_ID:
perfc_incr(mshv_wrmsr_osid);
d->arch.hvm_domain.viridian.guest_os_id.raw = val;
dump_guest_os_id(d);
break;
- case VIRIDIAN_MSR_HYPERCALL:
+ case HV_X64_MSR_HYPERCALL:
perfc_incr(mshv_wrmsr_hc_page);
d->arch.hvm_domain.viridian.hypercall_gpa.raw = val;
dump_hypercall(d);
@@ -434,16 +485,16 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
enable_hypercall_page(d);
break;
- case VIRIDIAN_MSR_VP_INDEX:
+ case HV_X64_MSR_VP_INDEX:
perfc_incr(mshv_wrmsr_vp_index);
break;
- case VIRIDIAN_MSR_EOI:
+ case HV_X64_MSR_EOI:
perfc_incr(mshv_wrmsr_eoi);
vlapic_EOI_set(vcpu_vlapic(v));
break;
- case VIRIDIAN_MSR_ICR: {
+ case HV_X64_MSR_ICR: {
u32 eax = (u32)val, edx = (u32)(val >> 32);
struct vlapic *vlapic = vcpu_vlapic(v);
perfc_incr(mshv_wrmsr_icr);
@@ -455,21 +506,21 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
break;
}
- case VIRIDIAN_MSR_TPR:
+ case HV_X64_MSR_TPR:
perfc_incr(mshv_wrmsr_tpr);
vlapic_set_reg(vcpu_vlapic(v), APIC_TASKPRI, (uint8_t)val);
break;
- case VIRIDIAN_MSR_APIC_ASSIST:
+ case HV_X64_MSR_VP_ASSIST_PAGE:
perfc_incr(mshv_wrmsr_apic_msr);
- teardown_apic_assist(v); /* release any previous mapping */
- v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = val;
- dump_apic_assist(v);
- if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled )
- initialize_apic_assist(v);
+ teardown_vp_assist(v); /* release any previous mapping */
+ v->arch.hvm_vcpu.viridian.vp_assist.msr.raw = val;
+ dump_vp_assist(v);
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.enabled )
+ initialize_vp_assist(v);
break;
- case VIRIDIAN_MSR_REFERENCE_TSC:
+ case HV_X64_MSR_REFERENCE_TSC:
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
return 0;
@@ -530,22 +581,22 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
switch ( idx )
{
- case VIRIDIAN_MSR_GUEST_OS_ID:
+ case HV_X64_MSR_GUEST_OS_ID:
perfc_incr(mshv_rdmsr_osid);
*val = d->arch.hvm_domain.viridian.guest_os_id.raw;
break;
- case VIRIDIAN_MSR_HYPERCALL:
+ case HV_X64_MSR_HYPERCALL:
perfc_incr(mshv_rdmsr_hc_page);
*val = d->arch.hvm_domain.viridian.hypercall_gpa.raw;
break;
- case VIRIDIAN_MSR_VP_INDEX:
+ case HV_X64_MSR_VP_INDEX:
perfc_incr(mshv_rdmsr_vp_index);
*val = v->vcpu_id;
break;
- case VIRIDIAN_MSR_TSC_FREQUENCY:
+ case HV_X64_MSR_TSC_FREQUENCY:
if ( viridian_feature_mask(d) & HVMPV_no_freq )
return 0;
@@ -553,7 +604,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
*val = (uint64_t)d->arch.tsc_khz * 1000ull;
break;
- case VIRIDIAN_MSR_APIC_FREQUENCY:
+ case HV_X64_MSR_APIC_FREQUENCY:
if ( viridian_feature_mask(d) & HVMPV_no_freq )
return 0;
@@ -561,23 +612,23 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
*val = 1000000000ull / APIC_BUS_CYCLE_NS;
break;
- case VIRIDIAN_MSR_ICR:
+ case HV_X64_MSR_ICR:
perfc_incr(mshv_rdmsr_icr);
*val = (((uint64_t)vlapic_get_reg(vcpu_vlapic(v), APIC_ICR2) << 32) |
vlapic_get_reg(vcpu_vlapic(v), APIC_ICR));
break;
- case VIRIDIAN_MSR_TPR:
+ case HV_X64_MSR_TPR:
perfc_incr(mshv_rdmsr_tpr);
*val = vlapic_get_reg(vcpu_vlapic(v), APIC_TASKPRI);
break;
- case VIRIDIAN_MSR_APIC_ASSIST:
+ case HV_X64_MSR_VP_ASSIST_PAGE:
perfc_incr(mshv_rdmsr_apic_msr);
- *val = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw;
+ *val = v->arch.hvm_vcpu.viridian.vp_assist.msr.raw;
break;
- case VIRIDIAN_MSR_REFERENCE_TSC:
+ case HV_X64_MSR_REFERENCE_TSC:
if ( !(viridian_feature_mask(d) & HVMPV_reference_tsc) )
return 0;
@@ -585,7 +636,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
*val = d->arch.hvm_domain.viridian.reference_tsc.raw;
break;
- case VIRIDIAN_MSR_TIME_REF_COUNT:
+ case HV_X64_MSR_TIME_REF_COUNT:
{
struct viridian_time_ref_count *trc;
@@ -612,7 +663,7 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
void viridian_vcpu_deinit(struct vcpu *v)
{
- teardown_apic_assist(v);
+ teardown_vp_assist(v);
}
void viridian_domain_deinit(struct domain *d)
@@ -620,7 +671,7 @@ void viridian_domain_deinit(struct domain *d)
struct vcpu *v;
for_each_vcpu ( d, v )
- teardown_apic_assist(v);
+ teardown_vp_assist(v);
}
static DEFINE_PER_CPU(cpumask_t, ipi_cpumask);
@@ -678,7 +729,7 @@ int viridian_hypercall(struct cpu_user_regs *regs)
{
case HvNotifyLongSpinWait:
/*
- * See Microsoft Hypervisor Top Level Spec. section 18.5.1.
+ * See section 14.5.1 of the specification.
*/
perfc_incr(mshv_call_long_wait);
do_sched_op(SCHEDOP_yield, guest_handle_from_ptr(NULL, void));
@@ -697,8 +748,7 @@ int viridian_hypercall(struct cpu_user_regs *regs)
} input_params;
/*
- * See Microsoft Hypervisor Top Level Spec. sections 12.4.2
- * and 12.4.3.
+ * See sections 9.4.2 and 9.4.4 of the specification.
*/
perfc_incr(mshv_call_flush);
@@ -822,8 +872,8 @@ static int viridian_save_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
for_each_vcpu( d, v ) {
struct hvm_viridian_vcpu_context ctxt = {
- .apic_assist_msr = v->arch.hvm_vcpu.viridian.apic_assist.msr.raw,
- .apic_assist_vector = v->arch.hvm_vcpu.viridian.apic_assist.vector,
+ .vp_assist_msr = v->arch.hvm_vcpu.viridian.vp_assist.msr.raw,
+ .vp_assist_vector = v->arch.hvm_vcpu.viridian.vp_assist.vector,
};
if ( hvm_save_entry(VIRIDIAN_VCPU, v->vcpu_id, h, &ctxt) != 0 )
@@ -853,11 +903,11 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
if ( memcmp(&ctxt._pad, zero_page, sizeof(ctxt._pad)) )
return -EINVAL;
- v->arch.hvm_vcpu.viridian.apic_assist.msr.raw = ctxt.apic_assist_msr;
- if ( v->arch.hvm_vcpu.viridian.apic_assist.msr.fields.enabled )
- initialize_apic_assist(v);
+ v->arch.hvm_vcpu.viridian.vp_assist.msr.raw = ctxt.vp_assist_msr;
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.enabled )
+ initialize_vp_assist(v);
- v->arch.hvm_vcpu.viridian.apic_assist.vector = ctxt.apic_assist_vector;
+ v->arch.hvm_vcpu.viridian.vp_assist.vector = ctxt.vp_assist_vector;
return 0;
}
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 8c45d0f..271c36d 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -9,7 +9,7 @@
#ifndef __ASM_X86_HVM_VIRIDIAN_H__
#define __ASM_X86_HVM_VIRIDIAN_H__
-union viridian_apic_assist
+union viridian_vp_assist
{ uint64_t raw;
struct
{
@@ -22,10 +22,10 @@ union viridian_apic_assist
struct viridian_vcpu
{
struct {
- union viridian_apic_assist msr;
+ union viridian_vp_assist msr;
void *va;
int vector;
- } apic_assist;
+ } vp_assist;
};
union viridian_guest_os_id
diff --git a/xen/include/public/arch-x86/hvm/save.h b/xen/include/public/arch-x86/hvm/save.h
index 419a3b2..66ae1a2 100644
--- a/xen/include/public/arch-x86/hvm/save.h
+++ b/xen/include/public/arch-x86/hvm/save.h
@@ -588,8 +588,8 @@ struct hvm_viridian_domain_context {
DECLARE_HVM_SAVE_TYPE(VIRIDIAN_DOMAIN, 15, struct hvm_viridian_domain_context);
struct hvm_viridian_vcpu_context {
- uint64_t apic_assist_msr;
- uint8_t apic_assist_vector;
+ uint64_t vp_assist_msr;
+ uint8_t vp_assist_vector;
uint8_t _pad[7];
};
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
2017-03-17 9:57 ` [PATCH 1/7] x86/viridian: update to version 5.0a of the specification Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 11:36 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2 Paul Durrant
` (4 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
Currently use of xen-hvmcrash will cause an immediate domain_crash() in
initialize_vp_assist() because it is called from viridian_load_vcpu_ctxt()
without having first cleared any previous mapping.
This patch makes initialize_vp_assist() responsible for clearing previous
mappings, if necessary.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/viridian.c | 50 +++++++++++++++++++-------------------
xen/include/asm-x86/hvm/viridian.h | 1 +
2 files changed, 26 insertions(+), 25 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index d741e81..59d76d5 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -276,6 +276,22 @@ static void enable_hypercall_page(struct domain *d)
put_page_and_type(page);
}
+static void teardown_vp_assist(struct vcpu *v)
+{
+ void *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
+ struct page_info *page;
+
+ if ( !va )
+ return;
+
+ v->arch.hvm_vcpu.viridian.vp_assist.va = NULL;
+
+ page = mfn_to_page(domain_page_map_to_mfn(va));
+
+ unmap_domain_page_global(va);
+ put_page_and_type(page);
+}
+
static void initialize_vp_assist(struct vcpu *v)
{
struct domain *d = v->domain;
@@ -288,6 +304,14 @@ static void initialize_vp_assist(struct vcpu *v)
* enlightenment.
*/
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
+ {
+ if ( v->arch.hvm_vcpu.viridian.vp_assist.gmfn == gmfn )
+ return;
+
+ teardown_vp_assist(v);
+ }
+
if ( !page )
goto fail;
@@ -306,15 +330,8 @@ static void initialize_vp_assist(struct vcpu *v)
clear_page(va);
- /*
- * If we overwrite an existing address here then something has
- * gone wrong and a domain page will leak. Instead crash the
- * domain to make the problem obvious.
- */
- if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
- domain_crash(d);
-
v->arch.hvm_vcpu.viridian.vp_assist.va = va;
+ v->arch.hvm_vcpu.viridian.vp_assist.gmfn = gmfn;
return;
fail:
@@ -322,22 +339,6 @@ static void initialize_vp_assist(struct vcpu *v)
page ? page_to_mfn(page) : mfn_x(INVALID_MFN));
}
-static void teardown_vp_assist(struct vcpu *v)
-{
- void *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
- struct page_info *page;
-
- if ( !va )
- return;
-
- v->arch.hvm_vcpu.viridian.vp_assist.va = NULL;
-
- page = mfn_to_page(domain_page_map_to_mfn(va));
-
- unmap_domain_page_global(va);
- put_page_and_type(page);
-}
-
void viridian_start_apic_assist(struct vcpu *v, int vector)
{
uint32_t *va = v->arch.hvm_vcpu.viridian.vp_assist.va;
@@ -513,7 +514,6 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
case HV_X64_MSR_VP_ASSIST_PAGE:
perfc_incr(mshv_wrmsr_apic_msr);
- teardown_vp_assist(v); /* release any previous mapping */
v->arch.hvm_vcpu.viridian.vp_assist.msr.raw = val;
dump_vp_assist(v);
if ( v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.enabled )
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 271c36d..18f1f1a 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -23,6 +23,7 @@ struct viridian_vcpu
{
struct {
union viridian_vp_assist msr;
+ unsigned long gmfn;
void *va;
int vector;
} vp_assist;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
2017-03-17 9:57 ` [PATCH 1/7] x86/viridian: update to version 5.0a of the specification Paul Durrant
2017-03-17 9:57 ` [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 11:41 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2 Paul Durrant
` (3 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
The Hypervisor Top Level Functional Specification v5.0a states in section
2.5:
"The hypervisor version information is encoded in leaf 0x40000002. Two
version numbers are provided: the main version and the service version.
The main version includes a major and minor version number and a build
number. These correspond to Microsoft Windows release numbers."
It also goes on to advise clients (i.e. guest versions of Windows) to use
the following algorithm to determine compatibility with the hypervisor
enlightenments:
if <your-main-version> greater than <hypervisor-main-version>
{
your version is compatible
}
else if <your-main-version> equal to <hypervisor-main-version> and
<your-service-version> greater than or equal to <hypervisor-service-version>
{
your version is compatible
}
else
{
your version is NOT compatible
}
So, clearly putting Xen hypervisor version information in that leaf is
spurious, but we probably get away with it because Xen's major version
is lower than the major version of Windows in which Hyper-V first
appeared (Server 2008).
This patch changes the leaf to hard-code the kernel major and minor
versions from Windows Server 2008.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/viridian.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 59d76d5..433035e 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
own version number. */
if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
break;
- res->a = 1; /* Build number */
- res->b = (xen_major_version() << 16) | xen_minor_version();
+ res->a = 0; /* Build number */
+ res->b = 0x00060000; /* Windows Server 2008 */
res->c = 0; /* SP */
res->d = 0; /* Service branch and number */
break;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
` (2 preceding siblings ...)
2017-03-17 9:57 ` [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2 Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 12:15 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs Paul Durrant
` (2 subsequent siblings)
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
The numbers correspond to ASCII characters so just use appropriate
character strings directly.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/viridian.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 433035e..4151ba5 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -119,14 +119,16 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
switch ( leaf )
{
case 0:
+ /* See section 2.4.1 of the specification */
res->a = 0x40000006; /* Maximum leaf */
- res->b = 0x7263694d; /* Magic numbers */
- res->c = 0x666F736F;
- res->d = 0x76482074;
+ res->b = *(uint32_t *)"Micr";
+ res->c = *(uint32_t *)"osof";
+ res->d = *(uint32_t *)"t Hv";
break;
case 1:
- res->a = 0x31237648; /* Version number */
+ /* See section 2.4.2 of the specification */
+ res->a = *(uint32_t *)"Hv#1";
break;
case 2:
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
` (3 preceding siblings ...)
2017-03-17 9:57 ` [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2 Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 12:21 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable Paul Durrant
2017-03-17 9:57 ` [PATCH 7/7] x86/viridian: implement the crash MSRs Paul Durrant
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
These warnings can be useful when Microsoft updates Windows.
In the past there have been several cases when Windows erroneously uses
hypercalls and MSRs that should be gated on CPUID flags than Xen does
not set. The usual symptom is a guest crash with little or no information
in the hypervisor log. Adding these warnings at least gives a clue as to
what might be happening in such cases.
Some versions of Windows do currently issue hypercalls that they should
not, so this patch whitelists those to avoid the warnings as the lack
of implementation is clearly proved not to be a problem to the guest.
The warnings are rate limited so a malicious guest cannot use them to
as a DoS.
NOTE: Because the MSR warnings need to be gated on range checking the
MSR address this patch imports the up-to-date definitions of all
the viridian MSRs from the specification.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
xen/arch/x86/hvm/viridian.c | 102 ++++++++++++++++++++++++++++++++++++++------
1 file changed, 88 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 4151ba5..deb57f9 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -23,17 +23,73 @@
#include <public/hvm/hvm_op.h>
/* Viridian MSR numbers. */
-#define HV_X64_MSR_GUEST_OS_ID 0x40000000
-#define HV_X64_MSR_HYPERCALL 0x40000001
-#define HV_X64_MSR_VP_INDEX 0x40000002
-#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
-#define HV_X64_MSR_REFERENCE_TSC 0x40000021
-#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
-#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
-#define HV_X64_MSR_EOI 0x40000070
-#define HV_X64_MSR_ICR 0x40000071
-#define HV_X64_MSR_TPR 0x40000072
-#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073
+#define HV_X64_MSR_GUEST_OS_ID 0x40000000
+#define HV_X64_MSR_HYPERCALL 0x40000001
+#define HV_X64_MSR_VP_INDEX 0x40000002
+#define HV_X64_MSR_RESET 0x40000003
+#define HV_X64_MSR_VP_RUNTIME 0x40000010
+#define HV_X64_MSR_TIME_REF_COUNT 0x40000020
+#define HV_X64_MSR_REFERENCE_TSC 0x40000021
+#define HV_X64_MSR_TSC_FREQUENCY 0x40000022
+#define HV_X64_MSR_APIC_FREQUENCY 0x40000023
+#define HV_X64_MSR_EOI 0x40000070
+#define HV_X64_MSR_ICR 0x40000071
+#define HV_X64_MSR_TPR 0x40000072
+#define HV_X64_MSR_VP_ASSIST_PAGE 0x40000073
+#define HV_X64_MSR_SCONTROL 0x40000080
+#define HV_X64_MSR_SVERSION 0x40000081
+#define HV_X64_MSR_SIEFP 0x40000082
+#define HV_X64_MSR_SIMP 0x40000083
+#define HV_X64_MSR_EOM 0x40000084
+#define HV_X64_MSR_SINT0 0x40000090
+#define HV_X64_MSR_SINT1 0x40000091
+#define HV_X64_MSR_SINT2 0x40000092
+#define HV_X64_MSR_SINT3 0x40000093
+#define HV_X64_MSR_SINT4 0x40000094
+#define HV_X64_MSR_SINT5 0x40000095
+#define HV_X64_MSR_SINT6 0x40000096
+#define HV_X64_MSR_SINT7 0x40000097
+#define HV_X64_MSR_SINT8 0x40000098
+#define HV_X64_MSR_SINT9 0x40000099
+#define HV_X64_MSR_SINT10 0x4000009A
+#define HV_X64_MSR_SINT11 0x4000009B
+#define HV_X64_MSR_SINT12 0x4000009C
+#define HV_X64_MSR_SINT13 0x4000009D
+#define HV_X64_MSR_SINT14 0x4000009E
+#define HV_X64_MSR_SINT15 0x4000009F
+#define HV_X64_MSR_STIMER0_CONFIG 0x400000B0
+#define HV_X64_MSR_STIMER0_COUNT 0x400000B1
+#define HV_X64_MSR_STIMER1_CONFIG 0x400000B2
+#define HV_X64_MSR_STIMER1_COUNT 0x400000B3
+#define HV_X64_MSR_STIMER2_CONFIG 0x400000B4
+#define HV_X64_MSR_STIMER2_COUNT 0x400000B5
+#define HV_X64_MSR_STIMER3_CONFIG 0x400000B6
+#define HV_X64_MSR_STIMER3_COUNT 0x400000B7
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C1 0x400000C1
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C2 0x400000C2
+#define HV_X64_MSR_POWER_STATE_TRIGGER_C3 0x400000C3
+#define HV_X64_MSR_POWER_STATE_CONFIG_C1 0x400000D1
+#define HV_X64_MSR_POWER_STATE_CONFIG_C2 0x400000D2
+#define HV_X64_MSR_POWER_STATE_CONFIG_C3 0x400000D3
+#define HV_X64_MSR_STATS_PARTITION_RETAIL_PAGE 0x400000E0
+#define HV_X64_MSR_STATS_PARTITION_INTERNAL_PAGE 0x400000E1
+#define HV_X64_MSR_STATS_VP_RETAIL_PAGE 0x400000E2
+#define HV_X64_MSR_STATS_VP_INTERNAL_PAGE 0x400000E3
+#define HV_X64_MSR_GUEST_IDLE 0x400000F0
+#define HV_X64_MSR_SYNTH_DEBUG_CONTROL 0x400000F1
+#define HV_X64_MSR_SYNTH_DEBUG_STATUS 0x400000F2
+#define HV_X64_MSR_SYNTH_DEBUG_SEND_BUFFER 0x400000F3
+#define HV_X64_MSR_SYNTH_DEBUG_RECEIVE_BUFFER 0x400000F4
+#define HV_X64_MSR_SYNTH_DEBUG_PENDING_BUFFER 0x400000F5
+#define HV_X64_MSR_CRASH_P0 0x40000100
+#define HV_X64_MSR_CRASH_P1 0x40000101
+#define HV_X64_MSR_CRASH_P2 0x40000102
+#define HV_X64_MSR_CRASH_P3 0x40000103
+#define HV_X64_MSR_CRASH_P4 0x40000104
+#define HV_X64_MSR_CRASH_CTL 0x40000105
+
+#define VIRIDIAN_MSR_MIN HV_X64_MSR_GUEST_OS_ID
+#define VIRIDIAN_MSR_MAX HV_X64_MSR_CRASH_CTL
/* Viridian Hypercall Status Codes. */
#define HV_STATUS_SUCCESS 0x0000
@@ -41,9 +97,11 @@
#define HV_STATUS_INVALID_PARAMETER 0x0005
/* Viridian Hypercall Codes. */
-#define HvFlushVirtualAddressSpace 2
-#define HvFlushVirtualAddressList 3
-#define HvNotifyLongSpinWait 8
+#define HvFlushVirtualAddressSpace 0x0002
+#define HvFlushVirtualAddressList 0x0003
+#define HvNotifyLongSpinWait 0x0008
+#define HvGetPartitionId 0x0046
+#define HvExtCallQueryCapabilities 0x8001
/* Viridian Hypercall Flags. */
#define HV_FLUSH_ALL_PROCESSORS 1
@@ -534,6 +592,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
break;
default:
+ if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
+ gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
+ idx);
+
return 0;
}
@@ -657,6 +719,10 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
}
default:
+ if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
+ gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
+ idx);
+
return 0;
}
@@ -809,7 +875,15 @@ int viridian_hypercall(struct cpu_user_regs *regs)
break;
}
+ case HvGetPartitionId:
+ case HvExtCallQueryCapabilities:
+ status = HV_STATUS_INVALID_HYPERCALL_CODE;
+ break;
+
default:
+ gdprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
+ input.call_code);
+
status = HV_STATUS_INVALID_HYPERCALL_CODE;
break;
}
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
` (4 preceding siblings ...)
2017-03-17 9:57 ` [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 12:26 ` Jan Beulich
2017-03-20 17:03 ` Andrew Cooper
2017-03-17 9:57 ` [PATCH 7/7] x86/viridian: implement the crash MSRs Paul Durrant
6 siblings, 2 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Paul Durrant, Jan Beulich
The current threshold before the guest issues the hypercall is, and always
has been, hard-coded to 2047. It is not clear where this number came
from so, to at least allow for ease of experimentation, this patch makes
the threshold tunable via the Xen command line.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
docs/misc/xen-command-line.markdown | 8 ++++++++
xen/arch/x86/hvm/viridian.c | 24 +++++++++++++++++++++++-
2 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 4daf5b5..7f7e0d9 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1623,6 +1623,14 @@ The optional `keep` parameter causes Xen to continue using the vga
console even after dom0 has been started. The default behaviour is to
relinquish control to dom0.
+### viridian\_spinlock\_retry\_count
+> `= <integer>`
+
+> Default: `2047`
+
+Specify the maximum number of retries before an enlightened Windows
+guest will notify Xen that it has failed to acquire a spinlock.
+
### vpid (Intel)
> `= <boolean>`
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index deb57f9..e7cc4e4 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -22,6 +22,12 @@
#include <public/sched.h>
#include <public/hvm/hvm_op.h>
+#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047
+
+static int __read_mostly viridian_spinlock_retry_count;
+integer_param("viridian_spinlock_retry_count",
+ viridian_spinlock_retry_count);
+
/* Viridian MSR numbers. */
#define HV_X64_MSR_GUEST_OS_ID 0x40000000
#define HV_X64_MSR_HYPERCALL 0x40000001
@@ -241,7 +247,13 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
res->a |= CPUID4A_HCALL_REMOTE_TLB_FLUSH;
if ( !cpu_has_vmx_apic_reg_virt )
res->a |= CPUID4A_MSR_BASED_APIC;
- res->b = 2047; /* long spin count */
+
+ /*
+ * This value is the recommended number of attempts to try to
+ * acquire a spinlock before notifying the hypervisor via the
+ * HvNotifyLongSpinWait hypercall.
+ */
+ res->b = viridian_spinlock_retry_count;
break;
case 6:
@@ -991,6 +1003,16 @@ 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);
+static int __init viridian_init(void)
+{
+ if ( !viridian_spinlock_retry_count )
+ viridian_spinlock_retry_count =
+ VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT;
+
+ return 0;
+}
+__initcall(viridian_init);
+
/*
* Local variables:
* mode: C
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* [PATCH 7/7] x86/viridian: implement the crash MSRs
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
` (5 preceding siblings ...)
2017-03-17 9:57 ` [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable Paul Durrant
@ 2017-03-17 9:57 ` Paul Durrant
2017-03-20 12:38 ` Jan Beulich
6 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-17 9:57 UTC (permalink / raw)
To: xen-devel; +Cc: Wei Liu, Paul Durrant, Ian Jackson, Jan Beulich, Andrew Cooper
Section 2.4.4 of the Hypervisor Top Level Functional Specification states
that enabling bit 10 in EDX of CPUID leaf 3 advertises to Windows a set
of MSRs into which it can write crash information.
This patch advertises that bit and implements the MSRs such that Xen can
log the information if a Windows guest crashes.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
docs/man/xl.cfg.pod.5.in | 10 ++++--
tools/libxl/libxl.h | 6 ++++
tools/libxl/libxl_dom.c | 4 +++
tools/libxl/libxl_types.idl | 1 +
xen/arch/x86/hvm/viridian.c | 67 ++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/hvm/viridian.h | 1 +
xen/include/public/hvm/params.h | 7 +++-
7 files changed, 93 insertions(+), 3 deletions(-)
diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 505c111..3522294 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1605,11 +1605,17 @@ per-vcpu event channel upcall vectors.
Note that this enlightenment will have no effect if the guest is
using APICv posted interrupts.
+=item B<crash_ctl>
+
+This group incorporates the crash control MSRs. These enlightenments
+allow Windows to write crash information such that it can be logged
+by Xen.
+
=item B<defaults>
This is a special value that enables the default set of groups, which
-is currently the B<base>, B<freq>, B<time_ref_count> and B<apic_assist>
-groups.
+is currently the B<base>, B<freq>, B<time_ref_count>, B<apic_assist>
+and B<crash_ctl> groups.
=item B<all>
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 92f1751..55ed872 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -288,6 +288,12 @@
#define LIBXL_HAVE_SCHED_CREDIT2_PARAMS 1
/*
+ * LIBXL_HAVE_CRASH_CTL indicates that the 'crash_ctl' value
+ * is present in the viridian enlightenment enumeration.
+ */
+#define LIBXL_HAVE_CRASH_CTL 1
+
+/*
* libxl ABI compatibility
*
* The only guarantee which libxl makes regarding ABI compatibility
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index d519c8d..fd77c21 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -214,6 +214,7 @@ static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_FREQ);
libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_TIME_REF_COUNT);
libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST);
+ libxl_bitmap_set(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL);
}
libxl_for_each_set_bit(v, info->u.hvm.viridian_enable) {
@@ -259,6 +260,9 @@ static int hvm_set_viridian_features(libxl__gc *gc, uint32_t domid,
if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_APIC_ASSIST))
mask |= HVMPV_apic_assist;
+ if (libxl_bitmap_test(&enlightenments, LIBXL_VIRIDIAN_ENLIGHTENMENT_CRASH_CTL))
+ mask |= HVMPV_crash_ctl;
+
if (mask != 0 &&
xc_hvm_param_set(CTX->xch,
domid,
diff --git a/tools/libxl/libxl_types.idl b/tools/libxl/libxl_types.idl
index a612d1f..e5c39e9 100644
--- a/tools/libxl/libxl_types.idl
+++ b/tools/libxl/libxl_types.idl
@@ -224,6 +224,7 @@ libxl_viridian_enlightenment = Enumeration("viridian_enlightenment", [
(3, "reference_tsc"),
(4, "hcall_remote_tlb_flush"),
(5, "apic_assist"),
+ (6, "crash_ctl"),
])
libxl_hdtype = Enumeration("hdtype", [
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index e7cc4e4..65a50a6 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -160,6 +160,19 @@ typedef struct {
uint64_t Reserved8:10;
} HV_PARTITION_PRIVILEGE_MASK;
+typedef union _HV_CRASH_CTL_REG_CONTENTS
+{
+ uint64_t AsUINT64;
+ struct
+ {
+ uint64_t Reserved:63;
+ uint64_t CrashNotify:1;
+ };
+} HV_CRASH_CTL_REG_CONTENTS;
+
+/* Viridian CPUID leaf 3, Hypervisor Feature Indication */
+#define CPUID3D_CRASH_MSRS (1 << 10)
+
/* Viridian CPUID leaf 4: Implementation Recommendations. */
#define CPUID4A_HCALL_REMOTE_TLB_FLUSH (1 << 2)
#define CPUID4A_MSR_BASED_APIC (1 << 3)
@@ -234,6 +247,10 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
res->a = u.lo;
res->b = u.hi;
+
+ if ( viridian_feature_mask(d) & HVMPV_crash_ctl )
+ res->d = CPUID3D_CRASH_MSRS;
+
break;
}
@@ -603,6 +620,37 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
update_reference_tsc(d, 1);
break;
+ case HV_X64_MSR_CRASH_P0:
+ case HV_X64_MSR_CRASH_P1:
+ case HV_X64_MSR_CRASH_P2:
+ case HV_X64_MSR_CRASH_P3:
+ case HV_X64_MSR_CRASH_P4:
+ BUILD_BUG_ON(HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0 >
+ ARRAY_SIZE(d->arch.hvm_domain.viridian.crash_param));
+
+ idx -= HV_X64_MSR_CRASH_P0;
+ d->arch.hvm_domain.viridian.crash_param[idx] = val;
+ break;
+
+ case HV_X64_MSR_CRASH_CTL:
+ {
+ HV_CRASH_CTL_REG_CONTENTS ctl;
+
+ ctl.AsUINT64 = val;
+
+ if ( !ctl.CrashNotify )
+ break;
+
+ printk(XENLOG_G_INFO "d%d: VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
+ d->domain_id,
+ d->arch.hvm_domain.viridian.crash_param[0],
+ d->arch.hvm_domain.viridian.crash_param[1],
+ d->arch.hvm_domain.viridian.crash_param[2],
+ d->arch.hvm_domain.viridian.crash_param[3],
+ d->arch.hvm_domain.viridian.crash_param[4]);
+ break;
+ }
+
default:
if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
@@ -730,6 +778,25 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
break;
}
+ case HV_X64_MSR_CRASH_P0:
+ case HV_X64_MSR_CRASH_P1:
+ case HV_X64_MSR_CRASH_P2:
+ case HV_X64_MSR_CRASH_P3:
+ case HV_X64_MSR_CRASH_P4:
+ idx -= HV_X64_MSR_CRASH_P0;
+ *val = d->arch.hvm_domain.viridian.crash_param[idx];
+ break;
+
+ case HV_X64_MSR_CRASH_CTL:
+ {
+ HV_CRASH_CTL_REG_CONTENTS ctl;
+
+ ctl.CrashNotify = 1;
+
+ *val = ctl.AsUINT64;
+ break;
+ }
+
default:
if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 18f1f1a..6de8a05 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -96,6 +96,7 @@ struct viridian_domain
union viridian_hypercall_gpa hypercall_gpa;
struct viridian_time_ref_count time_ref_count;
union viridian_reference_tsc reference_tsc;
+ uint64_t crash_param[5];
};
void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
diff --git a/xen/include/public/hvm/params.h b/xen/include/public/hvm/params.h
index 58c8478..19c9eb8 100644
--- a/xen/include/public/hvm/params.h
+++ b/xen/include/public/hvm/params.h
@@ -135,13 +135,18 @@
#define _HVMPV_apic_assist 5
#define HVMPV_apic_assist (1 << _HVMPV_apic_assist)
+/* Enable crash MSRs */
+#define _HVMPV_crash_ctl 6
+#define HVMPV_crash_ctl (1 << _HVMPV_crash_ctl)
+
#define HVMPV_feature_mask \
(HVMPV_base_freq | \
HVMPV_no_freq | \
HVMPV_time_ref_count | \
HVMPV_reference_tsc | \
HVMPV_hcall_remote_tlb_flush | \
- HVMPV_apic_assist)
+ HVMPV_apic_assist | \
+ HVMPV_crash_ctl)
#endif
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the specification
2017-03-17 9:57 ` [PATCH 1/7] x86/viridian: update to version 5.0a of the specification Paul Durrant
@ 2017-03-20 11:27 ` Jan Beulich
2017-03-20 11:43 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 11:27 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> @@ -48,20 +48,60 @@
> /* Viridian Hypercall Flags. */
> #define HV_FLUSH_ALL_PROCESSORS 1
>
> -/* Viridian CPUID 4000003, Viridian MSR availability. */
> -#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_REFERENCE_TSC (1 << 9)
> -#define CPUID3A_MSR_FREQ (1 << 11)
> -
> -/* Viridian CPUID 4000004, Implementation Recommendations. */
> +/*
> + * Viridian Partition Privilege Flags.
> + *
> + * This is taken from section 4.2.2 of the specification, and fixed for
> + * style and correctness.
> + */
> +typedef struct {
> + /* Access to virtual MSRs */
> + uint64_t AccessVpRunTimeReg:1;
> + uint64_t AccessPartitionReferenceCounter:1;
> + uint64_t AccessSynicRegs:1;
> + uint64_t AccessSyntheticTimerRegs:1;
> + uint64_t AccessIntrCtrlRegs:1;
> + uint64_t AccessHypercallMsrs:1;
> + uint64_t AccessVpIndex:1;
> + uint64_t AccessResetReg:1;
> + uint64_t AccessStatsReg:1;
> + uint64_t AccessPartitionReferenceTsc:1;
> + uint64_t AccessGuestIdleReg:1;
> + uint64_t AccessFrequencyRegs:1;
> + uint64_t AccessDebugRegs:1;
> + uint64_t Reserved1:19;
> +
> + /* Access to hypercalls */
> + uint64_t CreatePartitions:1;
> + uint64_t AccessPartitionId:1;
> + uint64_t AccessMemoryPool:1;
> + uint64_t AdjustMessageBuffers:1;
> + uint64_t PostMessages:1;
> + uint64_t SignalEvents:1;
> + uint64_t CreatePort:1;
> + uint64_t ConnectPort:1;
> + uint64_t AccessStats:1;
> + uint64_t Reserved2:2;
> + uint64_t Debugging:1;
> + uint64_t CpuManagement:1;
> + uint64_t Reserved3:1;
> + uint64_t Reserved4:1;
> + uint64_t Reserved5:1;
> + uint64_t AccessVSM:1;
> + uint64_t AccessVpRegisters:1;
> + uint64_t Reserved6:1;
> + uint64_t Reserved7:1;
> + uint64_t EnableExtendedHypercalls:1;
> + uint64_t StartVirtualProcessor:1;
> + uint64_t Reserved8:10;
> +} HV_PARTITION_PRIVILEGE_MASK;
I can see the use of uint64_t here matching the spec's UINT64, but
I don't see why we need a 64-bit (or even fixed width) type here.
Personally I'd also prefer if reserved entries in bit fields were simply
left unnamed.
> @@ -101,17 +141,35 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> break;
>
> case 3:
> - /* Which hypervisor MSRs are available to the guest */
> - res->a = (CPUID3A_MSR_APIC_ACCESS |
> - CPUID3A_MSR_HYPERCALL |
> - CPUID3A_MSR_VP_INDEX);
> + {
> + /*
> + * Section 2.4.4 details this leaf and states that EAX and EBX
> + * are defined to the the low and high parts of the partition
... to be the ...
> + * privilege mask respectively.
> + */
> + HV_PARTITION_PRIVILEGE_MASK mask = {
> + .AccessIntrCtrlRegs = 1,
> + .AccessHypercallMsrs = 1,
> + .AccessVpIndex = 1,
> + };
> + union {
> + HV_PARTITION_PRIVILEGE_MASK mask;
> + uint32_t lo, hi;
> + } u;
> +
> if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> - res->a |= CPUID3A_MSR_FREQ;
> + mask.AccessFrequencyRegs = 1;
> if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
> - res->a |= CPUID3A_MSR_TIME_REF_COUNT;
> + mask.AccessPartitionReferenceCounter = 1;
> if ( viridian_feature_mask(d) & HVMPV_reference_tsc )
> - res->a |= CPUID3A_MSR_REFERENCE_TSC;
> + mask.AccessPartitionReferenceTsc = 1;
> +
> + u.mask = mask;
> +
> + res->a = u.lo;
> + res->b = u.hi;
> break;
> + }
I think the code would be more clear without the "mask" helper
variable. But you're the maintainer ... (But please let me know
whether you want to do a v2 or rather have this committed as
is.)
Other than these minor items
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present
2017-03-17 9:57 ` [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present Paul Durrant
@ 2017-03-20 11:36 ` Jan Beulich
2017-03-20 11:50 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 11:36 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> @@ -288,6 +304,14 @@ static void initialize_vp_assist(struct vcpu *v)
> * enlightenment.
> */
>
> + if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
> + {
> + if ( v->arch.hvm_vcpu.viridian.vp_assist.gmfn == gmfn )
> + return;
Is this shortcut valid? I.e. is it not valid for the guest to expect the
VP assist state to be fully reset if it calls this more than once on a
vCPU, yet possibly with the same GFN? (It also looks like this isn't
really part of the corrections you want to make here, according to
the description.)
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -23,6 +23,7 @@ struct viridian_vcpu
> {
> struct {
> union viridian_vp_assist msr;
> + unsigned long gmfn;
gfn_t ?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-17 9:57 ` [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2 Paul Durrant
@ 2017-03-20 11:41 ` Jan Beulich
2017-03-20 11:57 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 11:41 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> The Hypervisor Top Level Functional Specification v5.0a states in section
> 2.5:
>
> "The hypervisor version information is encoded in leaf 0x40000002. Two
> version numbers are provided: the main version and the service version.
> The main version includes a major and minor version number and a build
> number. These correspond to Microsoft Windows release numbers."
>
> It also goes on to advise clients (i.e. guest versions of Windows) to use
> the following algorithm to determine compatibility with the hypervisor
> enlightenments:
>
> if <your-main-version> greater than <hypervisor-main-version>
> {
> your version is compatible
> }
> else if <your-main-version> equal to <hypervisor-main-version> and
> <your-service-version> greater than or equal to <hypervisor-service-version>
> {
> your version is compatible
> }
> else
> {
> your version is NOT compatible
> }
>
> So, clearly putting Xen hypervisor version information in that leaf is
> spurious, but we probably get away with it because Xen's major version
> is lower than the major version of Windows in which Hyper-V first
> appeared (Server 2008).
>
> This patch changes the leaf to hard-code the kernel major and minor
> versions from Windows Server 2008.
It's not really clear to me what the version numbers are supposed
to be used for: Is there any functionality availability of which can
be determined solely by looking at version? If so, wouldn't
hard coding a specific Windows Server version limit ourselves? If
not, does it really matter what we put there?
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> own version number. */
> if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
> break;
> - res->a = 1; /* Build number */
> - res->b = (xen_major_version() << 16) | xen_minor_version();
> + res->a = 0; /* Build number */
Is a build number of 0 valid at all?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the specification
2017-03-20 11:27 ` Jan Beulich
@ 2017-03-20 11:43 ` Paul Durrant
2017-03-20 11:54 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 11:43 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, Wei Liu, xen-devel, Ian Jackson
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 11:27
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the
> specification
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > @@ -48,20 +48,60 @@
> > /* Viridian Hypercall Flags. */
> > #define HV_FLUSH_ALL_PROCESSORS 1
> >
> > -/* Viridian CPUID 4000003, Viridian MSR availability. */
> > -#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_REFERENCE_TSC (1 << 9)
> > -#define CPUID3A_MSR_FREQ (1 << 11)
> > -
> > -/* Viridian CPUID 4000004, Implementation Recommendations. */
> > +/*
> > + * Viridian Partition Privilege Flags.
> > + *
> > + * This is taken from section 4.2.2 of the specification, and fixed for
> > + * style and correctness.
> > + */
> > +typedef struct {
> > + /* Access to virtual MSRs */
> > + uint64_t AccessVpRunTimeReg:1;
> > + uint64_t AccessPartitionReferenceCounter:1;
> > + uint64_t AccessSynicRegs:1;
> > + uint64_t AccessSyntheticTimerRegs:1;
> > + uint64_t AccessIntrCtrlRegs:1;
> > + uint64_t AccessHypercallMsrs:1;
> > + uint64_t AccessVpIndex:1;
> > + uint64_t AccessResetReg:1;
> > + uint64_t AccessStatsReg:1;
> > + uint64_t AccessPartitionReferenceTsc:1;
> > + uint64_t AccessGuestIdleReg:1;
> > + uint64_t AccessFrequencyRegs:1;
> > + uint64_t AccessDebugRegs:1;
> > + uint64_t Reserved1:19;
> > +
> > + /* Access to hypercalls */
> > + uint64_t CreatePartitions:1;
> > + uint64_t AccessPartitionId:1;
> > + uint64_t AccessMemoryPool:1;
> > + uint64_t AdjustMessageBuffers:1;
> > + uint64_t PostMessages:1;
> > + uint64_t SignalEvents:1;
> > + uint64_t CreatePort:1;
> > + uint64_t ConnectPort:1;
> > + uint64_t AccessStats:1;
> > + uint64_t Reserved2:2;
> > + uint64_t Debugging:1;
> > + uint64_t CpuManagement:1;
> > + uint64_t Reserved3:1;
> > + uint64_t Reserved4:1;
> > + uint64_t Reserved5:1;
> > + uint64_t AccessVSM:1;
> > + uint64_t AccessVpRegisters:1;
> > + uint64_t Reserved6:1;
> > + uint64_t Reserved7:1;
> > + uint64_t EnableExtendedHypercalls:1;
> > + uint64_t StartVirtualProcessor:1;
> > + uint64_t Reserved8:10;
> > +} HV_PARTITION_PRIVILEGE_MASK;
>
> I can see the use of uint64_t here matching the spec's UINT64, but
> I don't see why we need a 64-bit (or even fixed width) type here.
> Personally I'd also prefer if reserved entries in bit fields were simply
> left unnamed.
I'm trying not deviate from the spec too much. I resisted typdef-ing UINT64 but the spec does use that type and it does (albeit incorrectly in some cases ) name its reserved fields, so I'd rather leave this as-is.
>
> > @@ -101,17 +141,35 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> uint32_t leaf,
> > break;
> >
> > case 3:
> > - /* Which hypervisor MSRs are available to the guest */
> > - res->a = (CPUID3A_MSR_APIC_ACCESS |
> > - CPUID3A_MSR_HYPERCALL |
> > - CPUID3A_MSR_VP_INDEX);
> > + {
> > + /*
> > + * Section 2.4.4 details this leaf and states that EAX and EBX
> > + * are defined to the the low and high parts of the partition
>
> ... to be the ...
Oops, yes.
>
> > + * privilege mask respectively.
> > + */
> > + HV_PARTITION_PRIVILEGE_MASK mask = {
> > + .AccessIntrCtrlRegs = 1,
> > + .AccessHypercallMsrs = 1,
> > + .AccessVpIndex = 1,
> > + };
> > + union {
> > + HV_PARTITION_PRIVILEGE_MASK mask;
> > + uint32_t lo, hi;
> > + } u;
> > +
> > if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
> > - res->a |= CPUID3A_MSR_FREQ;
> > + mask.AccessFrequencyRegs = 1;
> > if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
> > - res->a |= CPUID3A_MSR_TIME_REF_COUNT;
> > + mask.AccessPartitionReferenceCounter = 1;
> > if ( viridian_feature_mask(d) & HVMPV_reference_tsc )
> > - res->a |= CPUID3A_MSR_REFERENCE_TSC;
> > + mask.AccessPartitionReferenceTsc = 1;
> > +
> > + u.mask = mask;
> > +
> > + res->a = u.lo;
> > + res->b = u.hi;
> > break;
> > + }
>
> I think the code would be more clear without the "mask" helper
> variable. But you're the maintainer ... (But please let me know
> whether you want to do a v2 or rather have this committed as
> is.)
>
> Other than these minor items
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
Thanks. Could you fix up that typo and commit.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present
2017-03-20 11:36 ` Jan Beulich
@ 2017-03-20 11:50 ` Paul Durrant
2017-03-20 13:42 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 11:50 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 11:36
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page
> is present
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > @@ -288,6 +304,14 @@ static void initialize_vp_assist(struct vcpu *v)
> > * enlightenment.
> > */
> >
> > + if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
> > + {
> > + if ( v->arch.hvm_vcpu.viridian.vp_assist.gmfn == gmfn )
> > + return;
>
> Is this shortcut valid? I.e. is it not valid for the guest to expect the
> VP assist state to be fully reset if it calls this more than once on a
> vCPU, yet possibly with the same GFN? (It also looks like this isn't
> really part of the corrections you want to make here, according to
> the description.)
Hmm. The spec is not clear. The problem is that doing a save-context followed by restore-context is going through this path. Maybe it's best to leave the teardown in the MSR right and special-case a restore when vp_assist_va is set.
>
> > --- a/xen/include/asm-x86/hvm/viridian.h
> > +++ b/xen/include/asm-x86/hvm/viridian.h
> > @@ -23,6 +23,7 @@ struct viridian_vcpu
> > {
> > struct {
> > union viridian_vp_assist msr;
> > + unsigned long gmfn;
>
> gfn_t ?
>
Yes, you're right. I should probably precede this with a patch fixing up the gmfn stack variables in viridian.c to use gfn_t for consistency though.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the specification
2017-03-20 11:43 ` Paul Durrant
@ 2017-03-20 11:54 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 11:54 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel, Ian Jackson
>>> On 20.03.17 at 12:43, <Paul.Durrant@citrix.com> wrote:
>> -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 March 2017 11:27
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
>> <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; xen-
>> devel@lists.xenproject.org
>> Subject: Re: [PATCH 1/7] x86/viridian: update to version 5.0a of the
>> specification
>>
>> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> > @@ -48,20 +48,60 @@
>> > /* Viridian Hypercall Flags. */
>> > #define HV_FLUSH_ALL_PROCESSORS 1
>> >
>> > -/* Viridian CPUID 4000003, Viridian MSR availability. */
>> > -#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_REFERENCE_TSC (1 << 9)
>> > -#define CPUID3A_MSR_FREQ (1 << 11)
>> > -
>> > -/* Viridian CPUID 4000004, Implementation Recommendations. */
>> > +/*
>> > + * Viridian Partition Privilege Flags.
>> > + *
>> > + * This is taken from section 4.2.2 of the specification, and fixed for
>> > + * style and correctness.
>> > + */
>> > +typedef struct {
>> > + /* Access to virtual MSRs */
>> > + uint64_t AccessVpRunTimeReg:1;
>> > + uint64_t AccessPartitionReferenceCounter:1;
>> > + uint64_t AccessSynicRegs:1;
>> > + uint64_t AccessSyntheticTimerRegs:1;
>> > + uint64_t AccessIntrCtrlRegs:1;
>> > + uint64_t AccessHypercallMsrs:1;
>> > + uint64_t AccessVpIndex:1;
>> > + uint64_t AccessResetReg:1;
>> > + uint64_t AccessStatsReg:1;
>> > + uint64_t AccessPartitionReferenceTsc:1;
>> > + uint64_t AccessGuestIdleReg:1;
>> > + uint64_t AccessFrequencyRegs:1;
>> > + uint64_t AccessDebugRegs:1;
>> > + uint64_t Reserved1:19;
>> > +
>> > + /* Access to hypercalls */
>> > + uint64_t CreatePartitions:1;
>> > + uint64_t AccessPartitionId:1;
>> > + uint64_t AccessMemoryPool:1;
>> > + uint64_t AdjustMessageBuffers:1;
>> > + uint64_t PostMessages:1;
>> > + uint64_t SignalEvents:1;
>> > + uint64_t CreatePort:1;
>> > + uint64_t ConnectPort:1;
>> > + uint64_t AccessStats:1;
>> > + uint64_t Reserved2:2;
>> > + uint64_t Debugging:1;
>> > + uint64_t CpuManagement:1;
>> > + uint64_t Reserved3:1;
>> > + uint64_t Reserved4:1;
>> > + uint64_t Reserved5:1;
>> > + uint64_t AccessVSM:1;
>> > + uint64_t AccessVpRegisters:1;
>> > + uint64_t Reserved6:1;
>> > + uint64_t Reserved7:1;
>> > + uint64_t EnableExtendedHypercalls:1;
>> > + uint64_t StartVirtualProcessor:1;
>> > + uint64_t Reserved8:10;
>> > +} HV_PARTITION_PRIVILEGE_MASK;
>>
>> I can see the use of uint64_t here matching the spec's UINT64, but
>> I don't see why we need a 64-bit (or even fixed width) type here.
>> Personally I'd also prefer if reserved entries in bit fields were simply
>> left unnamed.
>
> I'm trying not deviate from the spec too much. I resisted typdef-ing UINT64
> but the spec does use that type and it does (albeit incorrectly in some cases
> ) name its reserved fields, so I'd rather leave this as-is.
>
>>
>> > @@ -101,17 +141,35 @@ void cpuid_viridian_leaves(const struct vcpu *v,
>> uint32_t leaf,
>> > break;
>> >
>> > case 3:
>> > - /* Which hypervisor MSRs are available to the guest */
>> > - res->a = (CPUID3A_MSR_APIC_ACCESS |
>> > - CPUID3A_MSR_HYPERCALL |
>> > - CPUID3A_MSR_VP_INDEX);
>> > + {
>> > + /*
>> > + * Section 2.4.4 details this leaf and states that EAX and EBX
>> > + * are defined to the the low and high parts of the partition
>>
>> ... to be the ...
>
> Oops, yes.
>
>>
>> > + * privilege mask respectively.
>> > + */
>> > + HV_PARTITION_PRIVILEGE_MASK mask = {
>> > + .AccessIntrCtrlRegs = 1,
>> > + .AccessHypercallMsrs = 1,
>> > + .AccessVpIndex = 1,
>> > + };
>> > + union {
>> > + HV_PARTITION_PRIVILEGE_MASK mask;
>> > + uint32_t lo, hi;
>> > + } u;
>> > +
>> > if ( !(viridian_feature_mask(d) & HVMPV_no_freq) )
>> > - res->a |= CPUID3A_MSR_FREQ;
>> > + mask.AccessFrequencyRegs = 1;
>> > if ( viridian_feature_mask(d) & HVMPV_time_ref_count )
>> > - res->a |= CPUID3A_MSR_TIME_REF_COUNT;
>> > + mask.AccessPartitionReferenceCounter = 1;
>> > if ( viridian_feature_mask(d) & HVMPV_reference_tsc )
>> > - res->a |= CPUID3A_MSR_REFERENCE_TSC;
>> > + mask.AccessPartitionReferenceTsc = 1;
>> > +
>> > + u.mask = mask;
>> > +
>> > + res->a = u.lo;
>> > + res->b = u.hi;
>> > break;
>> > + }
>>
>> I think the code would be more clear without the "mask" helper
>> variable. But you're the maintainer ... (But please let me know
>> whether you want to do a v2 or rather have this committed as
>> is.)
>>
>> Other than these minor items
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>
>
> Thanks. Could you fix up that typo and commit.
Sure.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-20 11:41 ` Jan Beulich
@ 2017-03-20 11:57 ` Paul Durrant
2017-03-20 12:03 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 11:57 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 11:42
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 3/7] x86/viridian: don't put Xen version information in
> CPUID leaf 2
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > The Hypervisor Top Level Functional Specification v5.0a states in section
> > 2.5:
> >
> > "The hypervisor version information is encoded in leaf 0x40000002. Two
> > version numbers are provided: the main version and the service version.
> > The main version includes a major and minor version number and a build
> > number. These correspond to Microsoft Windows release numbers."
> >
> > It also goes on to advise clients (i.e. guest versions of Windows) to use
> > the following algorithm to determine compatibility with the hypervisor
> > enlightenments:
> >
> > if <your-main-version> greater than <hypervisor-main-version>
> > {
> > your version is compatible
> > }
> > else if <your-main-version> equal to <hypervisor-main-version> and
> > <your-service-version> greater than or equal to <hypervisor-service-
> version>
> > {
> > your version is compatible
> > }
> > else
> > {
> > your version is NOT compatible
> > }
> >
> > So, clearly putting Xen hypervisor version information in that leaf is
> > spurious, but we probably get away with it because Xen's major version
> > is lower than the major version of Windows in which Hyper-V first
> > appeared (Server 2008).
> >
> > This patch changes the leaf to hard-code the kernel major and minor
> > versions from Windows Server 2008.
>
> It's not really clear to me what the version numbers are supposed
> to be used for: Is there any functionality availability of which can
> be determined solely by looking at version?
There's not *supposed* to be, but I'm going entirely off the algorithm quoted above, which implies choosing the oldest possible hypervisor-main-version gives that best chance of compatibility. Interestingly KVM chooses a version of 6.1 and build number of 0x1bbc.
> If so, wouldn't
> hard coding a specific Windows Server version limit ourselves? If
> not, does it really matter what we put there?
>
Well I want to avoid a scenario where Xen's version gets bumped and suddenly some viridian enlightenment stops working. As you say, everything is supposed to be based on feature flags anyway, so hardcoding a version seems safest.
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> uint32_t leaf,
> > own version number. */
> > if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
> > break;
> > - res->a = 1; /* Build number */
> > - res->b = (xen_major_version() << 16) | xen_minor_version();
> > + res->a = 0; /* Build number */
>
> Is a build number of 0 valid at all?
The algorithm appears to ignore it and I've not observed any problems with Server 2008, Server 200R2, Windows 10 Anniversary, Server 2016 or Redstone2, so I think it's safe. I don't mind picking the real one from a fully updated Server 2008 if you'd prefer.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-20 11:57 ` Paul Durrant
@ 2017-03-20 12:03 ` Jan Beulich
2017-03-20 13:08 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 12:03 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 20.03.17 at 12:57, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 March 2017 11:42
>> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> > So, clearly putting Xen hypervisor version information in that leaf is
>> > spurious, but we probably get away with it because Xen's major version
>> > is lower than the major version of Windows in which Hyper-V first
>> > appeared (Server 2008).
>> >
>> > This patch changes the leaf to hard-code the kernel major and minor
>> > versions from Windows Server 2008.
>>
>> It's not really clear to me what the version numbers are supposed
>> to be used for: Is there any functionality availability of which can
>> be determined solely by looking at version?
>
> There's not *supposed* to be, but I'm going entirely off the algorithm
> quoted above, which implies choosing the oldest possible
> hypervisor-main-version gives that best chance of compatibility.
> Interestingly KVM chooses a version of 6.1 and build number of 0x1bbc.
Might that be because Windows itself considers the older version
buggy in some respect?
>> If so, wouldn't
>> hard coding a specific Windows Server version limit ourselves? If
>> not, does it really matter what we put there?
>>
>
> Well I want to avoid a scenario where Xen's version gets bumped and suddenly
> some viridian enlightenment stops working. As you say, everything is supposed
> to be based on feature flags anyway, so hardcoding a version seems safest.
Sure, using Xen's version here was plain wrong. What I'm rather
wondering is whether the returned number needs to be a little
more dynamic (e.g. depending on other, newer features we
support, partly - iirc - optionally).
>> > --- a/xen/arch/x86/hvm/viridian.c
>> > +++ b/xen/arch/x86/hvm/viridian.c
>> > @@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v,
>> uint32_t leaf,
>> > own version number. */
>> > if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
>> > break;
>> > - res->a = 1; /* Build number */
>> > - res->b = (xen_major_version() << 16) | xen_minor_version();
>> > + res->a = 0; /* Build number */
>>
>> Is a build number of 0 valid at all?
>
> The algorithm appears to ignore it and I've not observed any problems with
> Server 2008, Server 200R2, Windows 10 Anniversary, Server 2016 or Redstone2,
> so I think it's safe. I don't mind picking the real one from a fully updated
> Server 2008 if you'd prefer.
Or the one KVM is using, provided they have somewhere spelled
out why it was this one they've picked?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2
2017-03-17 9:57 ` [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2 Paul Durrant
@ 2017-03-20 12:15 ` Jan Beulich
2017-03-20 12:56 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 12:15 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -119,14 +119,16 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
> switch ( leaf )
> {
> case 0:
> + /* See section 2.4.1 of the specification */
> res->a = 0x40000006; /* Maximum leaf */
> - res->b = 0x7263694d; /* Magic numbers */
> - res->c = 0x666F736F;
> - res->d = 0x76482074;
> + res->b = *(uint32_t *)"Micr";
> + res->c = *(uint32_t *)"osof";
> + res->d = *(uint32_t *)"t Hv";
> break;
>
> case 1:
> - res->a = 0x31237648; /* Version number */
> + /* See section 2.4.2 of the specification */
> + res->a = *(uint32_t *)"Hv#1";
> break;
You're the maintainer of the code, so I can't really reject this, but
(ugly) casts don't seem any better to me, the more that they cast
away constness. Would
memcpy(&res->a, "Hv#1", 4);
etc be acceptable to you?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs
2017-03-17 9:57 ` [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs Paul Durrant
@ 2017-03-20 12:21 ` Jan Beulich
2017-03-20 12:54 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 12:21 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> The warnings are rate limited so a malicious guest cannot use them to
> as a DoS.
In fact they're debug-build-only ones. Did you perhaps mean gprintk()?
> @@ -534,6 +592,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> break;
>
> default:
> + if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
Coding style (also further down).
> + gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> + idx);
Can you please make distinguishing of write from ...
> @@ -657,6 +719,10 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> }
>
> default:
> + if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> + gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> + idx);
... read possible without having to make use of the logged line
number?
> @@ -809,7 +875,15 @@ int viridian_hypercall(struct cpu_user_regs *regs)
> break;
> }
>
> + case HvGetPartitionId:
> + case HvExtCallQueryCapabilities:
> + status = HV_STATUS_INVALID_HYPERCALL_CODE;
> + break;
I think a brief comment on why these don't get a message logged
would be helpful to future readers. I also think that it would be
more natural to place the two case labels ...
> default:
> + gdprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
> + input.call_code);
> +
... here.
> status = HV_STATUS_INVALID_HYPERCALL_CODE;
> break;
> }
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-17 9:57 ` [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable Paul Durrant
@ 2017-03-20 12:26 ` Jan Beulich
2017-03-20 12:51 ` Paul Durrant
2017-03-20 17:03 ` Andrew Cooper
1 sibling, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 12:26 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -22,6 +22,12 @@
> #include <public/sched.h>
> #include <public/hvm/hvm_op.h>
>
> +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047
> +
> +static int __read_mostly viridian_spinlock_retry_count;
Why don't you simply initialized the variable to 2047? None of ...
> @@ -991,6 +1003,16 @@ 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);
>
> +static int __init viridian_init(void)
> +{
> + if ( !viridian_spinlock_retry_count )
> + viridian_spinlock_retry_count =
> + VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT;
> +
> + return 0;
> +}
> +__initcall(viridian_init);
... this would be needed then (and zero, while a useless value,
isn't being named invalid by the spec afaics).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] x86/viridian: implement the crash MSRs
2017-03-17 9:57 ` [PATCH 7/7] x86/viridian: implement the crash MSRs Paul Durrant
@ 2017-03-20 12:38 ` Jan Beulich
2017-03-20 12:48 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 12:38 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
>>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> @@ -234,6 +247,10 @@ void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
>
> res->a = u.lo;
> res->b = u.hi;
> +
> + if ( viridian_feature_mask(d) & HVMPV_crash_ctl )
> + res->d = CPUID3D_CRASH_MSRS;
|= (for consistency with other code as well as to avoid the need
to touch this line again going forward)
> @@ -603,6 +620,37 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> update_reference_tsc(d, 1);
> break;
>
> + case HV_X64_MSR_CRASH_P0:
> + case HV_X64_MSR_CRASH_P1:
> + case HV_X64_MSR_CRASH_P2:
> + case HV_X64_MSR_CRASH_P3:
> + case HV_X64_MSR_CRASH_P4:
> + BUILD_BUG_ON(HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0 >
> + ARRAY_SIZE(d->arch.hvm_domain.viridian.crash_param));
>= as it looks.
> + idx -= HV_X64_MSR_CRASH_P0;
> + d->arch.hvm_domain.viridian.crash_param[idx] = val;
> + break;
> +
> + case HV_X64_MSR_CRASH_CTL:
> + {
> + HV_CRASH_CTL_REG_CONTENTS ctl;
> +
> + ctl.AsUINT64 = val;
> +
> + if ( !ctl.CrashNotify )
> + break;
> +
> + printk(XENLOG_G_INFO "d%d: VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> + d->domain_id,
> + d->arch.hvm_domain.viridian.crash_param[0],
> + d->arch.hvm_domain.viridian.crash_param[1],
> + d->arch.hvm_domain.viridian.crash_param[2],
> + d->arch.hvm_domain.viridian.crash_param[3],
> + d->arch.hvm_domain.viridian.crash_param[4]);
> + break;
> + }
> +
> default:
> if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> @@ -730,6 +778,25 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> break;
> }
>
> + case HV_X64_MSR_CRASH_P0:
> + case HV_X64_MSR_CRASH_P1:
> + case HV_X64_MSR_CRASH_P2:
> + case HV_X64_MSR_CRASH_P3:
> + case HV_X64_MSR_CRASH_P4:
> + idx -= HV_X64_MSR_CRASH_P0;
> + *val = d->arch.hvm_domain.viridian.crash_param[idx];
> + break;
I think it would be better to reproduce the BUILD_BUG_ON() here.
> + case HV_X64_MSR_CRASH_CTL:
> + {
> + HV_CRASH_CTL_REG_CONTENTS ctl;
> +
> + ctl.CrashNotify = 1;
You leak 63 bits of hypervisor stack here.
> --- a/xen/include/asm-x86/hvm/viridian.h
> +++ b/xen/include/asm-x86/hvm/viridian.h
> @@ -96,6 +96,7 @@ struct viridian_domain
> union viridian_hypercall_gpa hypercall_gpa;
> struct viridian_time_ref_count time_ref_count;
> union viridian_reference_tsc reference_tsc;
> + uint64_t crash_param[5];
Are these really per-domain values (normally MSRs are per-vCPU)?
And don't they need migrating?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] x86/viridian: implement the crash MSRs
2017-03-20 12:38 ` Jan Beulich
@ 2017-03-20 12:48 ` Paul Durrant
2017-03-20 13:29 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 12:48 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, Wei Liu, xen-devel, Ian Jackson
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:38
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Wei Liu
> <wei.liu2@citrix.com>; Ian Jackson <Ian.Jackson@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 7/7] x86/viridian: implement the crash MSRs
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > @@ -234,6 +247,10 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> uint32_t leaf,
> >
> > res->a = u.lo;
> > res->b = u.hi;
> > +
> > + if ( viridian_feature_mask(d) & HVMPV_crash_ctl )
> > + res->d = CPUID3D_CRASH_MSRS;
>
> |= (for consistency with other code as well as to avoid the need
> to touch this line again going forward)
Ok.
>
> > @@ -603,6 +620,37 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> > update_reference_tsc(d, 1);
> > break;
> >
> > + case HV_X64_MSR_CRASH_P0:
> > + case HV_X64_MSR_CRASH_P1:
> > + case HV_X64_MSR_CRASH_P2:
> > + case HV_X64_MSR_CRASH_P3:
> > + case HV_X64_MSR_CRASH_P4:
> > + BUILD_BUG_ON(HV_X64_MSR_CRASH_P4 -
> HV_X64_MSR_CRASH_P0 >
> > + ARRAY_SIZE(d->arch.hvm_domain.viridian.crash_param));
>
> >= as it looks.
Yes.
>
> > + idx -= HV_X64_MSR_CRASH_P0;
> > + d->arch.hvm_domain.viridian.crash_param[idx] = val;
> > + break;
> > +
> > + case HV_X64_MSR_CRASH_CTL:
> > + {
> > + HV_CRASH_CTL_REG_CONTENTS ctl;
> > +
> > + ctl.AsUINT64 = val;
> > +
> > + if ( !ctl.CrashNotify )
> > + break;
> > +
> > + printk(XENLOG_G_INFO "d%d: VIRIDIAN CRASH: %lx %lx %lx %lx
> %lx\n",
> > + d->domain_id,
> > + d->arch.hvm_domain.viridian.crash_param[0],
> > + d->arch.hvm_domain.viridian.crash_param[1],
> > + d->arch.hvm_domain.viridian.crash_param[2],
> > + d->arch.hvm_domain.viridian.crash_param[3],
> > + d->arch.hvm_domain.viridian.crash_param[4]);
> > + break;
> > + }
> > +
> > default:
> > if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> > gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> > @@ -730,6 +778,25 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> > break;
> > }
> >
> > + case HV_X64_MSR_CRASH_P0:
> > + case HV_X64_MSR_CRASH_P1:
> > + case HV_X64_MSR_CRASH_P2:
> > + case HV_X64_MSR_CRASH_P3:
> > + case HV_X64_MSR_CRASH_P4:
> > + idx -= HV_X64_MSR_CRASH_P0;
> > + *val = d->arch.hvm_domain.viridian.crash_param[idx];
> > + break;
>
> I think it would be better to reproduce the BUILD_BUG_ON() here.
>
Ok.
> > + case HV_X64_MSR_CRASH_CTL:
> > + {
> > + HV_CRASH_CTL_REG_CONTENTS ctl;
> > +
> > + ctl.CrashNotify = 1;
>
> You leak 63 bits of hypervisor stack here.
>
Yes. Should use a struct initializer instead.
> > --- a/xen/include/asm-x86/hvm/viridian.h
> > +++ b/xen/include/asm-x86/hvm/viridian.h
> > @@ -96,6 +96,7 @@ struct viridian_domain
> > union viridian_hypercall_gpa hypercall_gpa;
> > struct viridian_time_ref_count time_ref_count;
> > union viridian_reference_tsc reference_tsc;
> > + uint64_t crash_param[5];
>
> Are these really per-domain values (normally MSRs are per-vCPU)?
I don't think the usage described in section 4.3 really warrants per-vCPU. I've not looked at the exact sequence of events but I'm fairly sure the MSRs are written by the Windows crash kernel, which is single threaded.
> And don't they need migrating?
>
I don't think so, given that they are only written just prior to the domain dying anyway.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-20 12:26 ` Jan Beulich
@ 2017-03-20 12:51 ` Paul Durrant
2017-03-20 13:22 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 12:51 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:26
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 6/7] x86/viridian: make the threshold for
> HvNotifyLongSpinWait tunable
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -22,6 +22,12 @@
> > #include <public/sched.h>
> > #include <public/hvm/hvm_op.h>
> >
> > +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047
> > +
> > +static int __read_mostly viridian_spinlock_retry_count;
>
> Why don't you simply initialized the variable to 2047? None of ...
>
I wasn't sure whether that was ok in Xen. I was following other code (mainly from grant table) as a template.
I'll change to using an initializer and drop the __initcall.
Paul
> > @@ -991,6 +1003,16 @@ 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);
> >
> > +static int __init viridian_init(void)
> > +{
> > + if ( !viridian_spinlock_retry_count )
> > + viridian_spinlock_retry_count =
> > + VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT;
> > +
> > + return 0;
> > +}
> > +__initcall(viridian_init);
>
> ... this would be needed then (and zero, while a useless value,
> isn't being named invalid by the spec afaics).
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs
2017-03-20 12:21 ` Jan Beulich
@ 2017-03-20 12:54 ` Paul Durrant
0 siblings, 0 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 12:54 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:22
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 5/7] x86/viridian: add warnings for unimplemented
> hypercalls and MSRs
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > The warnings are rate limited so a malicious guest cannot use them to
> > as a DoS.
>
> In fact they're debug-build-only ones. Did you perhaps mean gprintk()?
>
Yes, I did.
> > @@ -534,6 +592,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> > break;
> >
> > default:
> > + if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
>
> Coding style (also further down).
>
> > + gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> > + idx);
>
> Can you please make distinguishing of write from ...
>
> > @@ -657,6 +719,10 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
> > }
> >
> > default:
> > + if (idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX)
> > + gdprintk(XENLOG_WARNING, "unimplemented MSR %08x\n",
> > + idx);
>
> ... read possible without having to make use of the logged line
> number?
>
I will.
> > @@ -809,7 +875,15 @@ int viridian_hypercall(struct cpu_user_regs *regs)
> > break;
> > }
> >
> > + case HvGetPartitionId:
> > + case HvExtCallQueryCapabilities:
> > + status = HV_STATUS_INVALID_HYPERCALL_CODE;
> > + break;
>
> I think a brief comment on why these don't get a message logged
> would be helpful to future readers.
Sure.
> I also think that it would be
> more natural to place the two case labels ...
>
> > default:
> > + gdprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
> > + input.call_code);
> > +
>
> ... here.
>
Ok. My general preference is for default to be last but given that doing it this way round allows for a fall-through it is indeed neater.
Paul
> > status = HV_STATUS_INVALID_HYPERCALL_CODE;
> > break;
> > }
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2
2017-03-20 12:15 ` Jan Beulich
@ 2017-03-20 12:56 ` Paul Durrant
0 siblings, 0 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 12:56 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:15
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID
> leaves 1 and 2
>
> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -119,14 +119,16 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> uint32_t leaf,
> > switch ( leaf )
> > {
> > case 0:
> > + /* See section 2.4.1 of the specification */
> > res->a = 0x40000006; /* Maximum leaf */
> > - res->b = 0x7263694d; /* Magic numbers */
> > - res->c = 0x666F736F;
> > - res->d = 0x76482074;
> > + res->b = *(uint32_t *)"Micr";
> > + res->c = *(uint32_t *)"osof";
> > + res->d = *(uint32_t *)"t Hv";
> > break;
> >
> > case 1:
> > - res->a = 0x31237648; /* Version number */
> > + /* See section 2.4.2 of the specification */
> > + res->a = *(uint32_t *)"Hv#1";
> > break;
>
> You're the maintainer of the code, so I can't really reject this, but
> (ugly) casts don't seem any better to me, the more that they cast
> away constness. Would
>
> memcpy(&res->a, "Hv#1", 4);
>
> etc be acceptable to you?
I'm ok with that.
Paul
>
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-20 12:03 ` Jan Beulich
@ 2017-03-20 13:08 ` Paul Durrant
2017-03-20 13:20 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 13:08 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 12:03
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: RE: [PATCH 3/7] x86/viridian: don't put Xen version information in
> CPUID leaf 2
>
> >>> On 20.03.17 at 12:57, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 20 March 2017 11:42
> >> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> >> > So, clearly putting Xen hypervisor version information in that leaf is
> >> > spurious, but we probably get away with it because Xen's major version
> >> > is lower than the major version of Windows in which Hyper-V first
> >> > appeared (Server 2008).
> >> >
> >> > This patch changes the leaf to hard-code the kernel major and minor
> >> > versions from Windows Server 2008.
> >>
> >> It's not really clear to me what the version numbers are supposed
> >> to be used for: Is there any functionality availability of which can
> >> be determined solely by looking at version?
> >
> > There's not *supposed* to be, but I'm going entirely off the algorithm
> > quoted above, which implies choosing the oldest possible
> > hypervisor-main-version gives that best chance of compatibility.
> > Interestingly KVM chooses a version of 6.1 and build number of 0x1bbc.
>
> Might that be because Windows itself considers the older version
> buggy in some respect?
No clue. Maybe it's safer to match KVM.
>
> >> If so, wouldn't
> >> hard coding a specific Windows Server version limit ourselves? If
> >> not, does it really matter what we put there?
> >>
> >
> > Well I want to avoid a scenario where Xen's version gets bumped and
> suddenly
> > some viridian enlightenment stops working. As you say, everything is
> supposed
> > to be based on feature flags anyway, so hardcoding a version seems safest.
>
> Sure, using Xen's version here was plain wrong. What I'm rather
> wondering is whether the returned number needs to be a little
> more dynamic (e.g. depending on other, newer features we
> support, partly - iirc - optionally).
>
The algorithm would suggest that bumping up the hypervisor version would only be done with make it *incompatible* with older versions of Windows. E.g. if the ABI were being deliberately broken. I guess there is a chance we'd need to do that but I'd hope not.
> >> > --- a/xen/arch/x86/hvm/viridian.c
> >> > +++ b/xen/arch/x86/hvm/viridian.c
> >> > @@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v,
> >> uint32_t leaf,
> >> > own version number. */
> >> > if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
> >> > break;
> >> > - res->a = 1; /* Build number */
> >> > - res->b = (xen_major_version() << 16) | xen_minor_version();
> >> > + res->a = 0; /* Build number */
> >>
> >> Is a build number of 0 valid at all?
> >
> > The algorithm appears to ignore it and I've not observed any problems with
> > Server 2008, Server 200R2, Windows 10 Anniversary, Server 2016 or
> Redstone2,
> > so I think it's safe. I don't mind picking the real one from a fully updated
> > Server 2008 if you'd prefer.
>
> Or the one KVM is using, provided they have somewhere spelled
> out why it was this one they've picked?
>
I'll see if I mine out the commit that the chose the number and look for some justification. If not then maybe going with default values from Server 2008 with command line overrides 'just in case' would be best?
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2
2017-03-20 13:08 ` Paul Durrant
@ 2017-03-20 13:20 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 13:20 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 20.03.17 at 14:08, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 March 2017 12:03
>> >>> On 20.03.17 at 12:57, <Paul.Durrant@citrix.com> wrote:
>> >> From: Jan Beulich [mailto:JBeulich@suse.com]
>> >> Sent: 20 March 2017 11:42
>> >> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> >> > --- a/xen/arch/x86/hvm/viridian.c
>> >> > +++ b/xen/arch/x86/hvm/viridian.c
>> >> > @@ -134,8 +134,8 @@ void cpuid_viridian_leaves(const struct vcpu *v,
>> >> uint32_t leaf,
>> >> > own version number. */
>> >> > if ( d->arch.hvm_domain.viridian.guest_os_id.raw == 0 )
>> >> > break;
>> >> > - res->a = 1; /* Build number */
>> >> > - res->b = (xen_major_version() << 16) | xen_minor_version();
>> >> > + res->a = 0; /* Build number */
>> >>
>> >> Is a build number of 0 valid at all?
>> >
>> > The algorithm appears to ignore it and I've not observed any problems with
>> > Server 2008, Server 200R2, Windows 10 Anniversary, Server 2016 or
>> Redstone2,
>> > so I think it's safe. I don't mind picking the real one from a fully updated
>> > Server 2008 if you'd prefer.
>>
>> Or the one KVM is using, provided they have somewhere spelled
>> out why it was this one they've picked?
>>
>
> I'll see if I mine out the commit that the chose the number and look for
> some justification. If not then maybe going with default values from Server
> 2008 with command line overrides 'just in case' would be best?
I guess so, yes.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-20 12:51 ` Paul Durrant
@ 2017-03-20 13:22 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 13:22 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 20.03.17 at 13:51, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 March 2017 12:26
>> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/viridian.c
>> > +++ b/xen/arch/x86/hvm/viridian.c
>> > @@ -22,6 +22,12 @@
>> > #include <public/sched.h>
>> > #include <public/hvm/hvm_op.h>
>> >
>> > +#define VIRIDIAN_SPINLOCK_RETRY_COUNT_DEFAULT 2047
>> > +
>> > +static int __read_mostly viridian_spinlock_retry_count;
>>
>> Why don't you simply initialized the variable to 2047? None of ...
>>
>
> I wasn't sure whether that was ok in Xen. I was following other code (mainly
> from grant table) as a template.
That other approach in grant table code has, iirc, some other
reason.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] x86/viridian: implement the crash MSRs
2017-03-20 12:48 ` Paul Durrant
@ 2017-03-20 13:29 ` Jan Beulich
2017-03-20 13:33 ` Paul Durrant
0 siblings, 1 reply; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 13:29 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, Wei Liu, xen-devel, Ian Jackson
>>> On 20.03.17 at 13:48, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 20 March 2017 12:38
>> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> > --- a/xen/include/asm-x86/hvm/viridian.h
>> > +++ b/xen/include/asm-x86/hvm/viridian.h
>> > @@ -96,6 +96,7 @@ struct viridian_domain
>> > union viridian_hypercall_gpa hypercall_gpa;
>> > struct viridian_time_ref_count time_ref_count;
>> > union viridian_reference_tsc reference_tsc;
>> > + uint64_t crash_param[5];
>>
>> Are these really per-domain values (normally MSRs are per-vCPU)?
>
> I don't think the usage described in section 4.3 really warrants per-vCPU.
> I've not looked at the exact sequence of events but I'm fairly sure the MSRs
> are written by the Windows crash kernel, which is single threaded.
Well, what I'm wondering about here is whether it wouldn't be
possible (and reasonable) to have two vCPU-s crashing at almost
the same time write independent values. They might then even
sync with one another to settle on who of them should do the
final MSR write. But if they're indeed only (meant to be) written
by the (single threaded) crash kernel (which I find strange), then
of course these are fine per-domain.
>> And don't they need migrating?
>
> I don't think so, given that they are only written just prior to the domain
> dying anyway.
Well, that leaves the risk of the host admin to decide moving the
domain the very moment it is doing its final crash processing.
Which may of course be an acceptable risk.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 7/7] x86/viridian: implement the crash MSRs
2017-03-20 13:29 ` Jan Beulich
@ 2017-03-20 13:33 ` Paul Durrant
0 siblings, 0 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 13:33 UTC (permalink / raw)
To: 'Jan Beulich'; +Cc: Andrew Cooper, Wei Liu, xen-devel, Ian Jackson
> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 20 March 2017 13:29
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Wei Liu <wei.liu2@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: RE: [PATCH 7/7] x86/viridian: implement the crash MSRs
>
> >>> On 20.03.17 at 13:48, <Paul.Durrant@citrix.com> wrote:
> >> From: Jan Beulich [mailto:JBeulich@suse.com]
> >> Sent: 20 March 2017 12:38
> >> >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> >> > --- a/xen/include/asm-x86/hvm/viridian.h
> >> > +++ b/xen/include/asm-x86/hvm/viridian.h
> >> > @@ -96,6 +96,7 @@ struct viridian_domain
> >> > union viridian_hypercall_gpa hypercall_gpa;
> >> > struct viridian_time_ref_count time_ref_count;
> >> > union viridian_reference_tsc reference_tsc;
> >> > + uint64_t crash_param[5];
> >>
> >> Are these really per-domain values (normally MSRs are per-vCPU)?
> >
> > I don't think the usage described in section 4.3 really warrants per-vCPU.
> > I've not looked at the exact sequence of events but I'm fairly sure the
> MSRs
> > are written by the Windows crash kernel, which is single threaded.
>
> Well, what I'm wondering about here is whether it wouldn't be
> possible (and reasonable) to have two vCPU-s crashing at almost
> the same time write independent values. They might then even
> sync with one another to settle on who of them should do the
> final MSR write. But if they're indeed only (meant to be) written
> by the (single threaded) crash kernel (which I find strange), then
> of course these are fine per-domain.
>
Ok. Given it's not very much data, I'll make them per-vcpu just in case.
> >> And don't they need migrating?
> >
> > I don't think so, given that they are only written just prior to the domain
> > dying anyway.
>
> Well, that leaves the risk of the host admin to decide moving the
> domain the very moment it is doing its final crash processing.
> Which may of course be an acceptable risk.
>
I think that's probably an acceptable risk. I won't add them to the migration stream for the moment.
Paul
> Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present
2017-03-20 11:50 ` Paul Durrant
@ 2017-03-20 13:42 ` Paul Durrant
2017-03-20 13:58 ` Jan Beulich
0 siblings, 1 reply; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 13:42 UTC (permalink / raw)
To: Paul Durrant, 'Jan Beulich'; +Cc: Andrew Cooper, xen-devel
> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of
> Paul Durrant
> Sent: 20 March 2017 11:50
> To: 'Jan Beulich' <JBeulich@suse.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH 2/7] x86/viridian: fix xen-hvmcrash when
> vp_assist page is present
>
> > -----Original Message-----
> > From: Jan Beulich [mailto:JBeulich@suse.com]
> > Sent: 20 March 2017 11:36
> > To: Paul Durrant <Paul.Durrant@citrix.com>
> > Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> > devel@lists.xenproject.org
> > Subject: Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist
> page
> > is present
> >
> > >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
> > > @@ -288,6 +304,14 @@ static void initialize_vp_assist(struct vcpu *v)
> > > * enlightenment.
> > > */
> > >
> > > + if ( v->arch.hvm_vcpu.viridian.vp_assist.va )
> > > + {
> > > + if ( v->arch.hvm_vcpu.viridian.vp_assist.gmfn == gmfn )
> > > + return;
> >
> > Is this shortcut valid? I.e. is it not valid for the guest to expect the
> > VP assist state to be fully reset if it calls this more than once on a
> > vCPU, yet possibly with the same GFN? (It also looks like this isn't
> > really part of the corrections you want to make here, according to
> > the description.)
>
> Hmm. The spec is not clear. The problem is that doing a save-context
> followed by restore-context is going through this path. Maybe it's best to
> leave the teardown in the MSR right and special-case a restore when
> vp_assist_va is set.
>
> >
> > > --- a/xen/include/asm-x86/hvm/viridian.h
> > > +++ b/xen/include/asm-x86/hvm/viridian.h
> > > @@ -23,6 +23,7 @@ struct viridian_vcpu
> > > {
> > > struct {
> > > union viridian_vp_assist msr;
> > > + unsigned long gmfn;
> >
> > gfn_t ?
> >
>
> Yes, you're right. I should probably precede this with a patch fixing up the
> gmfn stack variables in viridian.c to use gfn_t for consistency though.
Actually, looking at this again, I'm not sure there's any point in making this a gfn_t. It's only stored for the purposes of an identity match and the thing it matches with is an unsigned long extracted from an MSR bit-field.
Paul
>
> Paul
>
> > Jan
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present
2017-03-20 13:42 ` Paul Durrant
@ 2017-03-20 13:58 ` Jan Beulich
0 siblings, 0 replies; 33+ messages in thread
From: Jan Beulich @ 2017-03-20 13:58 UTC (permalink / raw)
To: Paul Durrant; +Cc: Andrew Cooper, xen-devel
>>> On 20.03.17 at 14:42, <Paul.Durrant@citrix.com> wrote:
>> From: Xen-devel [mailto:xen-devel-bounces@lists.xen.org] On Behalf Of Paul Durrant
>> Sent: 20 March 2017 11:50
>> > From: Jan Beulich [mailto:JBeulich@suse.com]
>> > Sent: 20 March 2017 11:36
>> > >>> On 17.03.17 at 10:57, <paul.durrant@citrix.com> wrote:
>> > > --- a/xen/include/asm-x86/hvm/viridian.h
>> > > +++ b/xen/include/asm-x86/hvm/viridian.h
>> > > @@ -23,6 +23,7 @@ struct viridian_vcpu
>> > > {
>> > > struct {
>> > > union viridian_vp_assist msr;
>> > > + unsigned long gmfn;
>> >
>> > gfn_t ?
>> >
>>
>> Yes, you're right. I should probably precede this with a patch fixing up the
>> gmfn stack variables in viridian.c to use gfn_t for consistency though.
>
> Actually, looking at this again, I'm not sure there's any point in making
> this a gfn_t. It's only stored for the purposes of an identity match and the
> thing it matches with is an unsigned long extracted from an MSR bit-field.
Well, hence the question mark in my original reply: I did realize
this, yet our mid term goal is to have all GFN variables properly
typed, and I'm not sure we should make exceptions in cases
like this one. (But then the question is still open whether what
you use the field for is valid to do in the first place.)
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-17 9:57 ` [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable Paul Durrant
2017-03-20 12:26 ` Jan Beulich
@ 2017-03-20 17:03 ` Andrew Cooper
2017-03-20 17:07 ` Paul Durrant
1 sibling, 1 reply; 33+ messages in thread
From: Andrew Cooper @ 2017-03-20 17:03 UTC (permalink / raw)
To: Paul Durrant, xen-devel; +Cc: Jan Beulich
On 17/03/17 09:57, Paul Durrant wrote:
> The current threshold before the guest issues the hypercall is, and always
> has been, hard-coded to 2047. It is not clear where this number came
> from so, to at least allow for ease of experimentation, this patch makes
> the threshold tunable via the Xen command line.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
This is one example of a configuration option which I'd prefer to see
done with the (planned) new CPUID interface.
The value here doesn't have any impact on the hypervisor, so a
configuration option is free to be chosen entirely by the toolstack, and
would also become inherently per-domain configuration rather than global.
Can I talk you into deferring this change until the CPUID interface is
sorted, to avoid introducing yet another command line option?
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
* Re: [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable
2017-03-20 17:03 ` Andrew Cooper
@ 2017-03-20 17:07 ` Paul Durrant
0 siblings, 0 replies; 33+ messages in thread
From: Paul Durrant @ 2017-03-20 17:07 UTC (permalink / raw)
To: Andrew Cooper, xen-devel; +Cc: Jan Beulich
> -----Original Message-----
> From: Andrew Cooper
> Sent: 20 March 2017 17:03
> To: Paul Durrant <Paul.Durrant@citrix.com>; xen-devel@lists.xenproject.org
> Cc: Jan Beulich <jbeulich@suse.com>
> Subject: Re: [PATCH 6/7] x86/viridian: make the threshold for
> HvNotifyLongSpinWait tunable
>
> On 17/03/17 09:57, Paul Durrant wrote:
> > The current threshold before the guest issues the hypercall is, and always
> > has been, hard-coded to 2047. It is not clear where this number came
> > from so, to at least allow for ease of experimentation, this patch makes
> > the threshold tunable via the Xen command line.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
>
> This is one example of a configuration option which I'd prefer to see
> done with the (planned) new CPUID interface.
>
> The value here doesn't have any impact on the hypervisor, so a
> configuration option is free to be chosen entirely by the toolstack, and
> would also become inherently per-domain configuration rather than global.
>
> Can I talk you into deferring this change until the CPUID interface is
> sorted, to avoid introducing yet another command line option?
>
I don't really want to wait for something with no definite timescale. I don't see any harm in adding this and then stating it can be overridden by per-domain CPU policy later on. Presumably there's a bunch of other stuff where this would be the case.
Paul
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 33+ messages in thread
end of thread, other threads:[~2017-03-20 17:07 UTC | newest]
Thread overview: 33+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-17 9:57 [PATCH 0/7] x86/viridian updates Paul Durrant
2017-03-17 9:57 ` [PATCH 1/7] x86/viridian: update to version 5.0a of the specification Paul Durrant
2017-03-20 11:27 ` Jan Beulich
2017-03-20 11:43 ` Paul Durrant
2017-03-20 11:54 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 2/7] x86/viridian: fix xen-hvmcrash when vp_assist page is present Paul Durrant
2017-03-20 11:36 ` Jan Beulich
2017-03-20 11:50 ` Paul Durrant
2017-03-20 13:42 ` Paul Durrant
2017-03-20 13:58 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 3/7] x86/viridian: don't put Xen version information in CPUID leaf 2 Paul Durrant
2017-03-20 11:41 ` Jan Beulich
2017-03-20 11:57 ` Paul Durrant
2017-03-20 12:03 ` Jan Beulich
2017-03-20 13:08 ` Paul Durrant
2017-03-20 13:20 ` Jan Beulich
2017-03-17 9:57 ` [PATCH 4/7] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2 Paul Durrant
2017-03-20 12:15 ` Jan Beulich
2017-03-20 12:56 ` Paul Durrant
2017-03-17 9:57 ` [PATCH 5/7] x86/viridian: add warnings for unimplemented hypercalls and MSRs Paul Durrant
2017-03-20 12:21 ` Jan Beulich
2017-03-20 12:54 ` Paul Durrant
2017-03-17 9:57 ` [PATCH 6/7] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable Paul Durrant
2017-03-20 12:26 ` Jan Beulich
2017-03-20 12:51 ` Paul Durrant
2017-03-20 13:22 ` Jan Beulich
2017-03-20 17:03 ` Andrew Cooper
2017-03-20 17:07 ` Paul Durrant
2017-03-17 9:57 ` [PATCH 7/7] x86/viridian: implement the crash MSRs Paul Durrant
2017-03-20 12:38 ` Jan Beulich
2017-03-20 12:48 ` Paul Durrant
2017-03-20 13:29 ` Jan Beulich
2017-03-20 13:33 ` 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.