[PATCH v4 0/3] viridian updates

[PATCH v4 0/3] viridian updates

[Paul Durrant]

Paul Durrant (3):
  x86/viridian: don't put Xen version information in CPUID leaf 2
  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 |  16 ++++
 tools/libxl/libxl.h                 |   6 ++
 tools/libxl/libxl_dom.c             |   4 +
 tools/libxl/libxl_types.idl         |   1 +
 xen/arch/x86/hvm/viridian.c         | 144 +++++++++++++++++++++++++++++++++++-
 xen/include/asm-x86/hvm/viridian.h  |   1 +
 xen/include/public/hvm/params.h     |   7 +-
 8 files changed, 183 insertions(+), 6 deletions(-)

-- 
2.1.4


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

[PATCH v4 1/3] 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-version' parameter.

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

v4:
 - Make elements of viridian-version parameter optional
 - Use custom_param()
 - Constify a pointer

v3:
 - Use a combined version parameter

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 |  8 +++++
 xen/arch/x86/hvm/viridian.c         | 59 +++++++++++++++++++++++++++++++++++--
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bad20db..c81d693 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1616,6 +1616,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-version
+> `= [<major>],[<minor>],[<build>]`
+
+> Default: `6,0,0x1772`
+
+<major>, <minor> and <build> must be integers. The values will be
+encoded in guest CPUID 0x40000002 if viridian enlightenments are enabled.
+
 ### vpid (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index b740224..3d58416 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -164,6 +164,16 @@ 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 viridian_major = 6;
+static uint16_t viridian_minor = 0;
+static uint32_t viridian_build = 0x1772;
+
 void cpuid_viridian_leaves(const struct vcpu *v, uint32_t leaf,
                            uint32_t subleaf, struct cpuid_leaf *res)
 {
@@ -194,8 +204,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 = viridian_build;
+        res->b = ((uint32_t)viridian_major << 16) | viridian_minor;
         res->c = 0; /* SP */
         res->d = 0; /* Service branch and number */
         break;
@@ -990,6 +1000,51 @@ 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 void __init parse_viridian_version(char *arg)
+{
+    const char *t;
+    long n[3];
+    unsigned int i = 0;
+
+    if ( !arg )
+        return;
+
+    while ( (t = strsep(&arg, ",")) != NULL )
+    {
+        const char *e;
+
+        if ( *t == '\0' )
+        {
+            n[i++] = -1;
+            continue;
+        }
+
+        n[i++] = simple_strtoul(t, &e, 0);
+        if ( *e != '\0' )
+            goto fail;
+    }
+    if ( i != 3 )
+        goto fail;
+
+    if ( n[0] > 0xffff || n[1] > 0xffff || n[2] > 0xffffffff )
+        goto fail;
+
+    if ( n[0] >= 0 )
+        viridian_major = n[0];
+    if ( n[1] >= 0 )
+        viridian_minor = n[1];
+    if ( n[2] >= 0 )
+        viridian_build = n[2];
+
+    printk("Overriding viridian-version to %#x,%#x,%#x\n",
+           viridian_major, viridian_minor, viridian_build);
+    return;
+
+fail:
+    printk(XENLOG_WARNING "Invalid viridian-version, using default\n");
+}
+custom_param("viridian-version", parse_viridian_version);
+
 /*
  * Local variables:
  * mode: C
-- 
2.1.4


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

[PATCH v4 2/3] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3:
 - Use '-' in parameter name rather than '_'

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

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index c81d693..bdbdb8a 100644
--- a/docs/misc/xen-command-line.markdown
+++ b/docs/misc/xen-command-line.markdown
@@ -1624,6 +1624,14 @@ relinquish control to dom0.
 <major>, <minor> and <build> must be integers. The values will be
 encoded in guest CPUID 0x40000002 if viridian enlightenments are enabled.
 
+### 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 3d58416..e18a453 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -174,6 +174,14 @@ static uint16_t viridian_major = 6;
 static uint16_t viridian_minor = 0;
 static uint32_t viridian_build = 0x1772;
 
+/*
+ * Maximum number of retries before the guest will notify of failure
+ * to acquire a spinlock.
+ */
+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)
 {
@@ -251,7 +259,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 v4 3/3] 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Wei Liu <wei.liu2@citrix.com>
---
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Paul Durrant <paul.durrant@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v4:
 - Name the union in HV_CRASH_CTL_REG_CONTENTS to keep older gcc happy

v3:
 - Use WARNING level message rather than INFO

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 c88b904..cf03ded 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 e18a453..c7a3b60 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;
+    } u;
+} 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)
@@ -246,6 +259,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;
     }
 
@@ -609,6 +626,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.u.CrashNotify )
+            break;
+
+        gprintk(XENLOG_WARNING, "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 %#x\n",
@@ -736,6 +783,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 = {
+            .u.CrashNotify = 1,
+        };
+
+        *val = ctl.AsUINT64;
+        break;
+    }
+
     default:
         if ( idx >= VIRIDIAN_MSR_MIN && idx <= VIRIDIAN_MSR_MAX )
             gprintk(XENLOG_WARNING, "read from unimplemented MSR %#x\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 v4 3/3] x86/viridian: implement the crash MSRs

[Paul Durrant]

> -----Original Message-----
> From: Paul Durrant [mailto:paul.durrant@citrix.com]
> Sent: 22 March 2017 12:15
> To: xen-devel@lists.xenproject.org
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; Andrew Cooper <Andrew.Cooper3@citrix.com>
> Subject: [PATCH v4 3/3] x86/viridian: implement the crash MSRs
> 
> 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>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Wei Liu <wei.liu2@citrix.com>
> ---
> Cc: Ian Jackson <ian.jackson@eu.citrix.com>
> Cc: Paul Durrant <paul.durrant@citrix.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> v4:
>  - Name the union in HV_CRASH_CTL_REG_CONTENTS to keep older gcc
> happy
> 
> v3:
>  - Use WARNING level message rather than INFO
> 
> 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

Sorry, I forgot to fix this up. Can this be done at commit assuming the rest of the patch is now ok?

  Paul

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

Re: [PATCH v4 3/3] x86/viridian: implement the crash MSRs

[Jan Beulich]

>>> On 22.03.17 at 13:17, <Paul.Durrant@citrix.com> wrote:
>> From: Paul Durrant [mailto:paul.durrant@citrix.com]
>> Sent: 22 March 2017 12:15
>> --- 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
> 
> Sorry, I forgot to fix this up. Can this be done at commit assuming the rest 
> of the patch is now ok?

Sure.

Jan


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

Re: [PATCH v4 1/3] x86/viridian: don't put Xen version information in CPUID leaf 2

[Jan Beulich]

>>> On 22.03.17 at 13:15, <paul.durrant@citrix.com> wrote:
> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -164,6 +164,16 @@ 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 viridian_major = 6;
> +static uint16_t viridian_minor = 0;
> +static uint32_t viridian_build = 0x1772;

Didn't an earlier version have them all __read_mostly?

> @@ -990,6 +1000,51 @@ 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 void __init parse_viridian_version(char *arg)
> +{
> +    const char *t;
> +    long n[3];

Why long?

> +    unsigned int i = 0;
> +
> +    if ( !arg )
> +        return;

Pointless check (the sole caller of these functions never passes
NULL). Did you perhaps mean !*arg?

> +    while ( (t = strsep(&arg, ",")) != NULL )
> +    {
> +        const char *e;
> +
> +        if ( *t == '\0' )
> +        {
> +            n[i++] = -1;

I'd like to suggest a different approach: Fill n[] with the original
values, simply skip the update here when no value was specified,
and write all three fields back unconditionally below (the upper
bounds check is fine of course, but it could be done without
incurring an implicit dependency between types and literal
constants by verifying that the implicit casts won't truncate, i.e.
(typeof(viridian_xyz))n[x] != n[x]).

> +            continue;
> +        }
> +
> +        n[i++] = simple_strtoul(t, &e, 0);
> +        if ( *e != '\0' )
> +            goto fail;
> +    }
> +    if ( i != 3 )
> +        goto fail;
> +
> +    if ( n[0] > 0xffff || n[1] > 0xffff || n[2] > 0xffffffff )
> +        goto fail;
> +
> +    if ( n[0] >= 0 )
> +        viridian_major = n[0];
> +    if ( n[1] >= 0 )
> +        viridian_minor = n[1];
> +    if ( n[2] >= 0 )
> +        viridian_build = n[2];
> +
> +    printk("Overriding viridian-version to %#x,%#x,%#x\n",
> +           viridian_major, viridian_minor, viridian_build);

Same question as on an earlier version: Why hex?

> +    return;
> +
> +fail:

Ahem - indentation.

Jan


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

Re: [PATCH v4 1/3] x86/viridian: don't put Xen version information in CPUID leaf 2

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 22 March 2017 13:55
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v4 1/3] x86/viridian: don't put Xen version information in
> CPUID leaf 2
> 
> >>> On 22.03.17 at 13:15, <paul.durrant@citrix.com> wrote:
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -164,6 +164,16 @@ 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 viridian_major = 6;
> > +static uint16_t viridian_minor = 0;
> > +static uint32_t viridian_build = 0x1772;
> 
> Didn't an earlier version have them all __read_mostly?

Yes, that got dropped moving things around... I'll put it back.

> 
> > @@ -990,6 +1000,51 @@ 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 void __init parse_viridian_version(char *arg)
> > +{
> > +    const char *t;
> > +    long n[3];
> 
> Why long?
> 

Because I need something that can store a positive value up to 32-bits and also -1.

> > +    unsigned int i = 0;
> > +
> > +    if ( !arg )
> > +        return;
> 
> Pointless check (the sole caller of these functions never passes
> NULL). Did you perhaps mean !*arg?
> 

No, I was following example code from the first file my cscope hit... xen/arch/arm/acpi/boot.c which has a check of that form at line 198. I'm happy to drop it.

> > +    while ( (t = strsep(&arg, ",")) != NULL )
> > +    {
> > +        const char *e;
> > +
> > +        if ( *t == '\0' )
> > +        {
> > +            n[i++] = -1;
> 
> I'd like to suggest a different approach: Fill n[] with the original
> values, simply skip the update here when no value was specified,
> and write all three fields back unconditionally below (the upper
> bounds check is fine of course, but it could be done without
> incurring an implicit dependency between types and literal
> constants by verifying that the implicit casts won't truncate, i.e.
> (typeof(viridian_xyz))n[x] != n[x]).
> 

Ok.

> > +            continue;
> > +        }
> > +
> > +        n[i++] = simple_strtoul(t, &e, 0);
> > +        if ( *e != '\0' )
> > +            goto fail;
> > +    }
> > +    if ( i != 3 )
> > +        goto fail;
> > +
> > +    if ( n[0] > 0xffff || n[1] > 0xffff || n[2] > 0xffffffff )
> > +        goto fail;
> > +
> > +    if ( n[0] >= 0 )
> > +        viridian_major = n[0];
> > +    if ( n[1] >= 0 )
> > +        viridian_minor = n[1];
> > +    if ( n[2] >= 0 )
> > +        viridian_build = n[2];
> > +
> > +    printk("Overriding viridian-version to %#x,%#x,%#x\n",
> > +           viridian_major, viridian_minor, viridian_build);
> 
> Same question as on an earlier version: Why hex?
> 

Because dump_guest_os_id() uses hex and those values are what these will be compared against.

> > +    return;
> > +
> > +fail:
> 
> Ahem - indentation.

Sorry I forgot. Neither linux nor qemu indents labels in this way so it's a pain to remember.

  Paul

> 
> Jan


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