[PATCH v2 0/6] viridian updates

[PATCH v2 0/6] viridian updates

[Paul Durrant]

Only 6 patches now...

Paul Durrant (6):
  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 |  29 +++++
 tools/libxl/libxl.h                 |   6 +
 tools/libxl/libxl_dom.c             |   4 +
 tools/libxl/libxl_types.idl         |   1 +
 xen/arch/x86/hvm/viridian.c         | 222 ++++++++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/viridian.h  |   1 +
 xen/include/public/hvm/params.h     |   7 +-
 8 files changed, 255 insertions(+), 25 deletions(-)

-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 1/6] x86/viridian: fix xen-hvmcrash when vp_assist page is present

[Paul Durrant]

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 addes a check into viridian_load_vcpu_ctxt() to avoid re-
initialization and turned the domain_crash() in initialize_vp_assist()
into an ASSERT() since neither codepath into that function should allow
it to be hit.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2:
 - Patch significantly simplified
---
 xen/arch/x86/hvm/viridian.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index f2c9613..3a2611b 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -283,6 +283,8 @@ static void initialize_vp_assist(struct vcpu *v)
     struct page_info *page = get_page_from_gfn(d, gmfn, NULL, P2M_ALLOC);
     void *va;
 
+    ASSERT(!v->arch.hvm_vcpu.viridian.vp_assist.va);
+
     /*
      * See section 7.8.7 of the specification for details of this
      * enlightenment.
@@ -904,7 +906,8 @@ static int viridian_load_vcpu_ctxt(struct domain *d, hvm_domain_context_t *h)
         return -EINVAL;
 
     v->arch.hvm_vcpu.viridian.vp_assist.msr.raw = ctxt.vp_assist_msr;
-    if ( v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.enabled )
+    if ( v->arch.hvm_vcpu.viridian.vp_assist.msr.fields.enabled &&
+         !v->arch.hvm_vcpu.viridian.vp_assist.va )
         initialize_vp_assist(v);
 
     v->arch.hvm_vcpu.viridian.vp_assist.vector = ctxt.vp_assist_vector;
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 2/6] x86/viridian: don't put Xen version information in CPUID leaf 2

[Paul Durrant]

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 use the kernel major and minor
versions, and build number from Windows Server 2008 (64-bit) by default.
These default values can be overriden from the Xen command line using new
'viridian_major_version', 'viridian_minor_version' and
'viridian_build_number' parameters.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v2:
 - Use full version information (including build number) from Windows
   Server 2008
 - Add command line parameters to allow version information to be
   overridden
---
 docs/misc/xen-command-line.markdown | 21 +++++++++++++++++++++
 xen/arch/x86/hvm/viridian.c         | 20 ++++++++++++++++++--
 2 files changed, 39 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bad20db..44da4bd 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1616,6 +1616,27 @@ 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\_major\_version
+> `= <integer>`
+
+> Default: `6`
+
+The hypervisor major version number encoded in CPUID 0x40000002:EBX.
+
+### viridian\_minor\_version
+> `= <integer>`
+
+> Default: `0`
+
+The hypervisor minor version number encoded in CPUID 0x40000002:EBX.
+
+### viridian\_build\_number
+> `= <integer>`
+
+> Default: `0x1772`
+
+The hypervisor build number encoded in CPUID 0x40000002:EAX.
+
 ### vpid (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 3a2611b..0d208c3 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -106,6 +106,21 @@ typedef struct {
 #define CPUID6A_MSR_BITMAPS     (1 << 1)
 #define CPUID6A_NESTED_PAGING   (1 << 3)
 
+/*
+ * Version and build number reported by CPUID leaf 2
+ *
+ * These numbers are chosen to match the version numbers reported by
+ * Windows Server 2008.
+ */
+static uint16_t __read_mostly viridian_major_version = 6;
+integer_param("viridian_major_version", viridian_major_version);
+
+static uint16_t __read_mostly viridian_minor_version = 0;
+integer_param("viridian_minor_version", viridian_minor_version);
+
+static uint32_t __read_mostly viridian_build_number = 0x1772;
+integer_param("viridian_build_number", viridian_build_number);
+
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
                            uint32_t subleaf, struct cpuid_leaf *res)
 {
@@ -134,8 +149,9 @@ 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 = viridian_build_number;
+        res->b = ((uint32_t)viridian_major_version << 16) |
+                 viridian_minor_version;
         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

[PATCH v2 3/6] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2

[Paul Durrant]

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>

v2:
 - Use memcpy() rather than cast-and-assign
---
 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 0d208c3..27ad5e8 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -134,14 +134,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;
+        memcpy(&res->b, "Micr", 4);
+        memcpy(&res->c, "osof", 4);
+        memcpy(&res->d, "t Hv", 4);
         break;
 
     case 1:
-        res->a = 0x31237648; /* Version number */
+        /* See section 2.4.2 of the specification */
+        memcpy(&res->a, "Hv#1", 4);
         break;
 
     case 2:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 4/6] x86/viridian: add warnings for unimplemented hypercalls and MSRs

[Paul Durrant]

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>

v2:
 - Use gprintk() rather than gdprintk()
 - Further changes requested by Jan
---
 xen/arch/x86/hvm/viridian.c | 106 ++++++++++++++++++++++++++++++++++++++------
 1 file changed, 92 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index 27ad5e8..d241de2 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
@@ -552,6 +610,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
         break;
 
     default:
+        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
+            gprintk(XENLOG_WARNING, "write to unimplemented MSR %08x\n",
+                    idx);
+
         return 0;
     }
 
@@ -675,6 +737,10 @@ int rdmsr_viridian_regs(uint32_t idx, uint64_t *val)
     }
 
     default:
+        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
+            gprintk(XENLOG_WARNING, "read from unimplemented MSR %08x\n",
+                    idx);
+
         return 0;
     }
 
@@ -828,6 +894,18 @@ int viridian_hypercall(struct cpu_user_regs *regs)
     }
 
     default:
+        gprintk(XENLOG_WARNING, "unimplemented hypercall %04x\n",
+                input.call_code);
+
+    case HvGetPartitionId:
+    case HvExtCallQueryCapabilities:
+        /*
+         * These hypercalls seem to be erroneously issued by Windows
+         * despite neither AccessPartitionId nor EnableExtendedHypercalls
+         * being set in CPUID leaf 2.
+         * Given that return a status of 'invalid code' has not so far
+         * caused any problems it's not worth logging.
+         */
         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

[PATCH v2 5/6] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable

[Paul Durrant]

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>

v2:
 - Significantly simplify patch by directly initializing the parameter
---
 docs/misc/xen-command-line.markdown |  8 ++++++++
 xen/arch/x86/hvm/viridian.c         | 12 +++++++++++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index 44da4bd..962e4d1 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1637,6 +1637,14 @@ The hypervisor minor version number encoded in CPUID 0x40000002:EBX.
 
 The hypervisor build number encoded in CPUID 0x40000002:EAX.
 
+### 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 d241de2..5ca1167 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -179,6 +179,10 @@ integer_param("viridian_minor_version", viridian_minor_version);
 static uint32_t __read_mostly viridian_build_number = 0x1772;
 integer_param("viridian_build_number", viridian_build_number);
 
+static uint32_t __read_mostly viridian_spinlock_retry_count = 2047;
+integer_param("viridian_spinlock_retry_count",
+              viridian_spinlock_retry_count);
+
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
                            uint32_t subleaf, struct cpuid_leaf *res)
 {
@@ -257,7 +261,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:
-- 
2.1.4


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

[PATCH v2 6/6] x86/viridian: implement the crash MSRs

[Paul Durrant]

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>

v2:
 - Make MSRs per-vcpu rather than per-domain
 - Fix hypervisor stack leak
 - Replicate BUILD_BOG_ON()
 - Use gprintk() rather than printk()
---
 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        | 69 ++++++++++++++++++++++++++++++++++++++
 xen/include/asm-x86/hvm/viridian.h |  1 +
 xen/include/public/hvm/params.h    |  7 +++-
 7 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/docs/man/xl.cfg.pod.5.in b/docs/man/xl.cfg.pod.5.in
index 52802d5..991960b 100644
--- a/docs/man/xl.cfg.pod.5.in
+++ b/docs/man/xl.cfg.pod.5.in
@@ -1601,11 +1601,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 72ec39d..af45582 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 e133962..c25cf48 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 2475a4d..69e789a 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 5ca1167..c914ee8 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -154,6 +154,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)
@@ -248,6 +261,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;
     }
 
@@ -619,6 +636,36 @@ 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(v->arch.hvm_vcpu.viridian.crash_param));
+
+        idx -= HV_X64_MSR_CRASH_P0;
+        v->arch.hvm_vcpu.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;
+
+        gprintk(XENLOG_INFO, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
+                v->arch.hvm_vcpu.viridian.crash_param[0],
+                v->arch.hvm_vcpu.viridian.crash_param[1],
+                v->arch.hvm_vcpu.viridian.crash_param[2],
+                v->arch.hvm_vcpu.viridian.crash_param[3],
+                v->arch.hvm_vcpu.viridian.crash_param[4]);
+        break;
+    }
+
     default:
         if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
             gprintk(XENLOG_WARNING, "write to unimplemented MSR %08x\n",
@@ -746,6 +793,28 @@ 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:
+        BUILD_BUG_ON(HV_X64_MSR_CRASH_P4 - HV_X64_MSR_CRASH_P0 >=
+                     ARRAY_SIZE(v->arch.hvm_vcpu.viridian.crash_param));
+
+        idx -= HV_X64_MSR_CRASH_P0;
+        *val = v->arch.hvm_vcpu.viridian.crash_param[idx];
+        break;
+
+    case HV_X64_MSR_CRASH_CTL:
+    {
+        HV_CRASH_CTL_REG_CONTENTS ctl = {
+            .CrashNotify = 1,
+        };
+
+        *val = ctl.AsUINT64;
+        break;
+    }
+
     default:
         if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
             gprintk(XENLOG_WARNING, "read from unimplemented MSR %08x\n",
diff --git a/xen/include/asm-x86/hvm/viridian.h b/xen/include/asm-x86/hvm/viridian.h
index 271c36d..30259e9 100644
--- a/xen/include/asm-x86/hvm/viridian.h
+++ b/xen/include/asm-x86/hvm/viridian.h
@@ -26,6 +26,7 @@ struct viridian_vcpu
         void *va;
         int vector;
     } vp_assist;
+    uint64_t crash_param[5];
 };
 
 union viridian_guest_os_id
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

Re: [PATCH v2 1/6] x86/viridian: fix xen-hvmcrash when vp_assist page is present

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> 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 addes a check into viridian_load_vcpu_ctxt() to avoid re-
> initialization and turned the domain_crash() in initialize_vp_assist()
> into an ASSERT() since neither codepath into that function should allow
> it to be hit.

The patch looks fine to me, but not in line with the description:
There's no domain_crash() being eliminated anymore.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 1/6] x86/viridian: fix xen-hvmcrash when vp_assist page is present

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 March 2017 15:27
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 1/6] x86/viridian: fix xen-hvmcrash when vp_assist
> page is present
> 
> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> > 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 addes a check into viridian_load_vcpu_ctxt() to avoid re-
> > initialization and turned the domain_crash() in initialize_vp_assist()
> > into an ASSERT() since neither codepath into that function should allow
> > it to be hit.
> 
> The patch looks fine to me, but not in line with the description:
> There's no domain_crash() being eliminated anymore.

Oh. That hunk must have been dropped for some reason. It should be there. I'll send v3.

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/viridian: don't put Xen version information in CPUID leaf 2

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -106,6 +106,21 @@ typedef struct {
>  #define CPUID6A_MSR_BITMAPS     (1 << 1)
>  #define CPUID6A_NESTED_PAGING   (1 << 3)
>  
> +/*
> + * Version and build number reported by CPUID leaf 2
> + *
> + * These numbers are chosen to match the version numbers reported by
> + * Windows Server 2008.
> + */
> +static uint16_t __read_mostly viridian_major_version = 6;
> +integer_param("viridian_major_version", viridian_major_version);
> +
> +static uint16_t __read_mostly viridian_minor_version = 0;
> +integer_param("viridian_minor_version", viridian_minor_version);
> +
> +static uint32_t __read_mostly viridian_build_number = 0x1772;
> +integer_param("viridian_build_number", viridian_build_number);

While I realize it's easier for you this way, I dislike having three
options here. How about

viridian-version=[<major>],[<minor>],[<build>]

?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 3/6] x86/viridian: get rid of the magic numbers in CPUID leaves 1 and 2

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> The numbers correspond to ASCII characters so just use appropriate
> character strings directly.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/6] x86/viridian: add warnings for unimplemented hypercalls and MSRs

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

with one remark:

> @@ -552,6 +610,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
>          break;
>  
>      default:
> +        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
> +            gprintk(XENLOG_WARNING, "write to unimplemented MSR %08x\n",
> +                    idx);

Mind if I shorten the string literal by one using %#x instead of
%08x (also on the read side) while committing?

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/6] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> 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>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

albeit ...

> --- a/docs/misc/xen-command-line.markdown
> +++ b/docs/misc/xen-command-line.markdown
> @@ -1637,6 +1637,14 @@ The hypervisor minor version number encoded in CPUID 0x40000002:EBX.
>  
>  The hypervisor build number encoded in CPUID 0x40000002:EAX.
>  
> +### viridian\_spinlock\_retry\_count

... I'd prefer if dashes instead of underscores were used:
Command line options don't need to be identifiers, and dashes
are easier to type (not requiring the shift key).

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 6/6] x86/viridian: implement the crash MSRs

[Jan Beulich]

>>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> @@ -619,6 +636,36 @@ 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(v->arch.hvm_vcpu.viridian.crash_param));
> +
> +        idx -= HV_X64_MSR_CRASH_P0;
> +        v->arch.hvm_vcpu.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;
> +
> +        gprintk(XENLOG_INFO, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> +                v->arch.hvm_vcpu.viridian.crash_param[0],
> +                v->arch.hvm_vcpu.viridian.crash_param[1],
> +                v->arch.hvm_vcpu.viridian.crash_param[2],
> +                v->arch.hvm_vcpu.viridian.crash_param[3],
> +                v->arch.hvm_vcpu.viridian.crash_param[4]);

With default log level settings this message will go nowhere. If that's
intended, I don't mind, but I think XENLOG_WARNING (or simply no
log level) would be better here.

Reviewed-by: Jan Beulich <jbeulich@suse.com>

Let me know whether you want me to make the adjustment while
committing.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/viridian: don't put Xen version information in CPUID leaf 2

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 March 2017 15:29
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 2/6] x86/viridian: don't put Xen version information in
> CPUID leaf 2
> 
> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -106,6 +106,21 @@ typedef struct {
> >  #define CPUID6A_MSR_BITMAPS     (1 << 1)
> >  #define CPUID6A_NESTED_PAGING   (1 << 3)
> >
> > +/*
> > + * Version and build number reported by CPUID leaf 2
> > + *
> > + * These numbers are chosen to match the version numbers reported by
> > + * Windows Server 2008.
> > + */
> > +static uint16_t __read_mostly viridian_major_version = 6;
> > +integer_param("viridian_major_version", viridian_major_version);
> > +
> > +static uint16_t __read_mostly viridian_minor_version = 0;
> > +integer_param("viridian_minor_version", viridian_minor_version);
> > +
> > +static uint32_t __read_mostly viridian_build_number = 0x1772;
> > +integer_param("viridian_build_number", viridian_build_number);
> 
> While I realize it's easier for you this way, I dislike having three
> options here. How about
> 
> viridian-version=[<major>],[<minor>],[<build>]
> 

Ok. I guess I'll have to put back the initcall to parse this though.

  Paul

> ?
> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 5/6] x86/viridian: make the threshold for HvNotifyLongSpinWait tunable

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 March 2017 15:36
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 5/6] x86/viridian: make the threshold for
> HvNotifyLongSpinWait tunable
> 
> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> 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>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> albeit ...
> 
> > --- a/docs/misc/xen-command-line.markdown
> > +++ b/docs/misc/xen-command-line.markdown
> > @@ -1637,6 +1637,14 @@ The hypervisor minor version number encoded
> in CPUID 0x40000002:EBX.
> >
> >  The hypervisor build number encoded in CPUID 0x40000002:EAX.
> >
> > +### viridian\_spinlock\_retry\_count
> 
> ... I'd prefer if dashes instead of underscores were used:
> Command line options don't need to be identifiers, and dashes
> are easier to type (not requiring the shift key).

Ok. Since I'm modifying the other param(s) then I'll change this while I'm in the neighbourhood.

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 4/6] x86/viridian: add warnings for unimplemented hypercalls and MSRs

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 March 2017 15:33
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v2 4/6] x86/viridian: add warnings for unimplemented
> hypercalls and MSRs
> 
> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> > 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>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> with one remark:
> 
> > @@ -552,6 +610,10 @@ int wrmsr_viridian_regs(uint32_t idx, uint64_t val)
> >          break;
> >
> >      default:
> > +        if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
> > +            gprintk(XENLOG_WARNING, "write to unimplemented MSR
> %08x\n",
> > +                    idx);
> 
> Mind if I shorten the string literal by one using %#x instead of
> %08x (also on the read side) while committing?

No, I don't mind.

  Paul

> 
> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 6/6] x86/viridian: implement the crash MSRs

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 21 March 2017 15:42
> 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 v2 6/6] x86/viridian: implement the crash MSRs
> 
> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
> > @@ -619,6 +636,36 @@ 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(v->arch.hvm_vcpu.viridian.crash_param));
> > +
> > +        idx -= HV_X64_MSR_CRASH_P0;
> > +        v->arch.hvm_vcpu.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;
> > +
> > +        gprintk(XENLOG_INFO, "VIRIDIAN CRASH: %lx %lx %lx %lx %lx\n",
> > +                v->arch.hvm_vcpu.viridian.crash_param[0],
> > +                v->arch.hvm_vcpu.viridian.crash_param[1],
> > +                v->arch.hvm_vcpu.viridian.crash_param[2],
> > +                v->arch.hvm_vcpu.viridian.crash_param[3],
> > +                v->arch.hvm_vcpu.viridian.crash_param[4]);
> 
> With default log level settings this message will go nowhere. If that's
> intended, I don't mind, but I think XENLOG_WARNING (or simply no
> log level) would be better here.

Oh, I thought INFO was on by default. Please change it to WARNING... it seems appropriate.

> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> Let me know whether you want me to make the adjustment while
> committing.
> 

Thanks but since I'm going to send a v3 anyway, I'll fix it.

  Paul

> Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 2/6] x86/viridian: don't put Xen version information in CPUID leaf 2

[Jan Beulich]

>>> On 21.03.17 at 16:45, <Paul.Durrant@citrix.com> wrote:
>>  -----Original Message-----
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 21 March 2017 15:29
>> To: Paul Durrant <Paul.Durrant@citrix.com>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
>> devel@lists.xenproject.org 
>> Subject: Re: [PATCH v2 2/6] x86/viridian: don't put Xen version information 
> in
>> CPUID leaf 2
>> 
>> >>> On 20.03.17 at 18:08, <paul.durrant@citrix.com> wrote:
>> > --- a/xen/arch/x86/hvm/viridian.c
>> > +++ b/xen/arch/x86/hvm/viridian.c
>> > @@ -106,6 +106,21 @@ typedef struct {
>> >  #define CPUID6A_MSR_BITMAPS     (1 << 1)
>> >  #define CPUID6A_NESTED_PAGING   (1 << 3)
>> >
>> > +/*
>> > + * Version and build number reported by CPUID leaf 2
>> > + *
>> > + * These numbers are chosen to match the version numbers reported by
>> > + * Windows Server 2008.
>> > + */
>> > +static uint16_t __read_mostly viridian_major_version = 6;
>> > +integer_param("viridian_major_version", viridian_major_version);
>> > +
>> > +static uint16_t __read_mostly viridian_minor_version = 0;
>> > +integer_param("viridian_minor_version", viridian_minor_version);
>> > +
>> > +static uint32_t __read_mostly viridian_build_number = 0x1772;
>> > +integer_param("viridian_build_number", viridian_build_number);
>> 
>> While I realize it's easier for you this way, I dislike having three
>> options here. How about
>> 
>> viridian-version=[<major>],[<minor>],[<build>]
>> 
> 
> Ok. I guess I'll have to put back the initcall to parse this though.

Right, that's why I did say "it's easier for you this way".

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: [PATCH v2 6/6] x86/viridian: implement the crash MSRs

[Wei Liu]

On Mon, Mar 20, 2017 at 05:08:21PM +0000, Paul Durrant wrote:
> 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>
> 
> v2:
>  - Make MSRs per-vcpu rather than per-domain
>  - Fix hypervisor stack leak
>  - Replicate BUILD_BOG_ON()
>  - Use gprintk() rather than printk()
> ---
>  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 +

Acked-by: Wei Liu <wei.liu2@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel