[PATCH v3 0/6] viridian updates

[PATCH v3 0/6] viridian updates

[Paul Durrant]

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 |  16 +++
 tools/libxl/libxl.h                 |   6 +
 tools/libxl/libxl_dom.c             |   4 +
 tools/libxl/libxl_types.idl         |   1 +
 xen/arch/x86/hvm/viridian.c         | 264 ++++++++++++++++++++++++++++++++----
 xen/include/asm-x86/hvm/viridian.h  |   1 +
 xen/include/public/hvm/params.h     |   7 +-
 8 files changed, 276 insertions(+), 33 deletions(-)

-- 
2.1.4


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

[PATCH v3 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>

v3:
 - Slightly too much simplification in v2... missing hunk re-instated

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

diff --git a/xen/arch/x86/hvm/viridian.c b/xen/arch/x86/hvm/viridian.c
index f2c9613..a71f928 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.
@@ -306,14 +308,6 @@ 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;
     return;
 
@@ -904,7 +898,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

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

[Jan Beulich]

>>> On 21.03.17 at 19:17, <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.
> 
> 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

[PATCH v3 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-version' parameter.

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

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         | 50 +++++++++++++++++++++++++++++++++++--
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/docs/misc/xen-command-line.markdown b/docs/misc/xen-command-line.markdown
index bad20db..f029e66 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,1772`
+
+<major>, <minor> and <build> must be integers specified in hexadecimal. 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 a71f928..8a5c831 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -106,6 +106,19 @@ 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 char __initdata viridian_version[sizeof("0xVVVV,0xVVVV,0xVVVVVVVV")] = "";
+string_param("viridian-version", viridian_version);
+
+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)
 {
@@ -134,8 +147,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;
@@ -910,6 +923,39 @@ 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)
+{
+    char *t, *v = viridian_version;
+    unsigned int n[3], i = 0;
+
+    if ( *v == '\0' )
+        return 0;
+
+    while ( (t = strsep(&v, ",")) != NULL )
+    {
+        const char *e;
+
+        n[i++] = simple_strtoul(t, &e, 16);
+        if ( *e != '\0' )
+            goto fail;
+    }
+    if ( i != 3 )
+        goto fail;
+
+    if (n[0] > 0xffff || n[1] > 0xffff)
+        goto fail;
+
+    viridian_major = n[0];
+    viridian_minor = n[1];
+    viridian_build = n[2];
+    return 0;
+
+fail:
+    printk(XENLOG_WARNING "Invalid viridian-version, using default\n");
+    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

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

[Jan Beulich]

>>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> --- 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>`

In my proposal I had intentionally enclosed each number in square
brackets, indicating that each one is optional. I don't see the point
of having to specify all numbers all the time (and your earlier split
option model didn't make this a requirement either).

> +> Default: `6,0,1772`
> +
> +<major>, <minor> and <build> must be integers specified in hexadecimal. The values will be
> +encoded in guest CPUID 0x40000002 if viridian enlightenments are enabled.

Please wrap at column 80 the latest. And why do the numbers need
to be in hex?

> --- a/xen/arch/x86/hvm/viridian.c
> +++ b/xen/arch/x86/hvm/viridian.c
> @@ -106,6 +106,19 @@ 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 char __initdata viridian_version[sizeof("0xVVVV,0xVVVV,0xVVVVVVVV")] = "";

Pointless in initializer.

> +string_param("viridian-version", viridian_version);

Why not custom_param()?

> @@ -910,6 +923,39 @@ 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)
> +{
> +    char *t, *v = viridian_version;

With e being pointer to const, I think t should be, too.

> +    unsigned int n[3], i = 0;
> +
> +    if ( *v == '\0' )
> +        return 0;
> +
> +    while ( (t = strsep(&v, ",")) != NULL )
> +    {
> +        const char *e;
> +
> +        n[i++] = simple_strtoul(t, &e, 16);
> +        if ( *e != '\0' )
> +            goto fail;
> +    }
> +    if ( i != 3 )
> +        goto fail;
> +
> +    if (n[0] > 0xffff || n[1] > 0xffff)

Missing blanks.

> +        goto fail;
> +
> +    viridian_major = n[0];
> +    viridian_minor = n[1];
> +    viridian_build = n[2];
> +    return 0;
> +
> +fail:

Indentation.

Jan


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

Re: [PATCH v3 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: 22 March 2017 10:10
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; xen-
> devel@lists.xenproject.org
> Subject: Re: [PATCH v3 2/6] x86/viridian: don't put Xen version information in
> CPUID leaf 2
> 
> >>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> > --- 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>`
> 
> In my proposal I had intentionally enclosed each number in square
> brackets, indicating that each one is optional. I don't see the point
> of having to specify all numbers all the time (and your earlier split
> option model didn't make this a requirement either).
>

So you'd like something like:

viridian-version=,,0xabcd

so that just the build number can be overridden to 0xabcd?
 
> > +> Default: `6,0,1772`
> > +
> > +<major>, <minor> and <build> must be integers specified in hexadecimal.
> The values will be
> > +encoded in guest CPUID 0x40000002 if viridian enlightenments are
> enabled.
> 
> Please wrap at column 80 the latest. And why do the numbers need
> to be in hex?

I can relax that restriction if you'd prefer.

> 
> > --- a/xen/arch/x86/hvm/viridian.c
> > +++ b/xen/arch/x86/hvm/viridian.c
> > @@ -106,6 +106,19 @@ 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 char __initdata
> viridian_version[sizeof("0xVVVV,0xVVVV,0xVVVVVVVV")] = "";
> 
> Pointless in initializer.
> 

Seemed to be common practice elsewhere in the code.

> > +string_param("viridian-version", viridian_version);
> 
> Why not custom_param()?
> 

Not come across custom_param(). I'll look it up.

> > @@ -910,6 +923,39 @@ 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)
> > +{
> > +    char *t, *v = viridian_version;
> 
> With e being pointer to const, I think t should be, too.
> 

Ok.

> > +    unsigned int n[3], i = 0;
> > +
> > +    if ( *v == '\0' )
> > +        return 0;
> > +
> > +    while ( (t = strsep(&v, ",")) != NULL )
> > +    {
> > +        const char *e;
> > +
> > +        n[i++] = simple_strtoul(t, &e, 16);
> > +        if ( *e != '\0' )
> > +            goto fail;
> > +    }
> > +    if ( i != 3 )
> > +        goto fail;
> > +
> > +    if (n[0] > 0xffff || n[1] > 0xffff)
> 
> Missing blanks.
> 

Oops.

> > +        goto fail;
> > +
> > +    viridian_major = n[0];
> > +    viridian_minor = n[1];
> > +    viridian_build = n[2];
> > +    return 0;
> > +
> > +fail:
> 
> Indentation.
>

Yes, sorry I forgot about that xen style quirk.

  Paul
 
> Jan


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

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

[Jan Beulich]

>>> On 22.03.17 at 11:20, <Paul.Durrant@citrix.com> wrote:
>> From: Jan Beulich [mailto:JBeulich@suse.com]
>> Sent: 22 March 2017 10:10
>> >>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
>> > --- 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>`
>> 
>> In my proposal I had intentionally enclosed each number in square
>> brackets, indicating that each one is optional. I don't see the point
>> of having to specify all numbers all the time (and your earlier split
>> option model didn't make this a requirement either).
>>
> 
> So you'd like something like:
> 
> viridian-version=,,0xabcd
> 
> so that just the build number can be overridden to 0xabcd?

Yes.

>> > +> Default: `6,0,1772`
>> > +
>> > +<major>, <minor> and <build> must be integers specified in hexadecimal.
>> The values will be
>> > +encoded in guest CPUID 0x40000002 if viridian enlightenments are
>> enabled.
>> 
>> Please wrap at column 80 the latest. And why do the numbers need
>> to be in hex?
> 
> I can relax that restriction if you'd prefer.

Yes please. I don't think version numbers are commonly expressed
in hex.

>> > --- a/xen/arch/x86/hvm/viridian.c
>> > +++ b/xen/arch/x86/hvm/viridian.c
>> > @@ -106,6 +106,19 @@ 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 char __initdata
>> viridian_version[sizeof("0xVVVV,0xVVVV,0xVVVVVVVV")] = "";
>> 
>> Pointless in initializer.
> 
> Seemed to be common practice elsewhere in the code.

Odd.

>> > +        goto fail;
>> > +
>> > +    viridian_major = n[0];
>> > +    viridian_minor = n[1];
>> > +    viridian_build = n[2];
>> > +    return 0;
>> > +
>> > +fail:
>> 
>> Indentation.
> 
> Yes, sorry I forgot about that xen style quirk.

Is this a quirk? It's desirable to not misguide diff's -p handling (as
we want it to produce the function, not some random label, where
the same name can occur more than once in a source file).

Jan


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

[PATCH v3 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>
Reviewed-by: 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 8a5c831..41f550c 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -132,14 +132,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 v3 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>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>

v3:
 - Slight tweak to string literals

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 41f550c..d6381c0 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
@@ -541,6 +599,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 %#x\n",
+                    idx);
+
         return 0;
     }
 
@@ -664,6 +726,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 %#x\n",
+                    idx);
+
         return 0;
     }
 
@@ -817,6 +883,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 v3 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>
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 f029e66..d42298c 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 specified in hexadecimal. 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 d6381c0..9d1272a 100644
--- a/xen/arch/x86/hvm/viridian.c
+++ b/xen/arch/x86/hvm/viridian.c
@@ -177,6 +177,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)
 {
@@ -254,7 +262,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 v3 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>
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>

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 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 9d1272a..dff749b 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)
@@ -249,6 +262,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;
     }
 
@@ -612,6 +629,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_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",
@@ -739,6 +786,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 %#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 v3 6/6] x86/viridian: implement the crash MSRs

[Jan Beulich]

>>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> --- 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

I was about to commit this together with 1, 3, and 4, when this
define caught my eye: Isn't the name a little too generic? I.e.
LIBXL_HAVE_VIRIDIAN_CRASH_CTL perhaps?

Jan


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

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

[Wei Liu]

On Wed, Mar 22, 2017 at 05:07:30AM -0600, Jan Beulich wrote:
> >>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> > --- 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
> 
> I was about to commit this together with 1, 3, and 4, when this
> define caught my eye: Isn't the name a little too generic? I.e.
> LIBXL_HAVE_VIRIDIAN_CRASH_CTL perhaps?
> 

That's fine by me.

> Jan
> 

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

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

[Paul Durrant]

> -----Original Message-----
> From: Wei Liu [mailto:wei.liu2@citrix.com]
> Sent: 22 March 2017 11:10
> To: Jan Beulich <JBeulich@suse.com>
> Cc: Paul Durrant <Paul.Durrant@citrix.com>; Wei Liu <wei.liu2@citrix.com>;
> Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v3 6/6] x86/viridian: implement the crash
> MSRs
> 
> On Wed, Mar 22, 2017 at 05:07:30AM -0600, Jan Beulich wrote:
> > >>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> > > --- 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
> >
> > I was about to commit this together with 1, 3, and 4, when this
> > define caught my eye: Isn't the name a little too generic? I.e.
> > LIBXL_HAVE_VIRIDIAN_CRASH_CTL perhaps?
> >
> 
> That's fine by me.
>

Ok with me too.

  Paul
 
> > Jan
> >

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

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

[Jan Beulich]

>>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> +    case HV_X64_MSR_CRASH_CTL:
> +    {
> +        HV_CRASH_CTL_REG_CONTENTS ctl = {
> +            .CrashNotify = 1,
> +        };

This initializer failed my pre-push build test: Some gcc versions we
still support don't allow fields of unnamed union members to be
initialized this way. As there are at least two ways to deal with
this, and as this is code maintained by you, I didn't want to pick
either, as I don't know your preference.

Jan


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

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

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 22 March 2017 11:38
> To: Paul Durrant <Paul.Durrant@citrix.com>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Ian Jackson
> <Ian.Jackson@citrix.com>; xen-devel@lists.xenproject.org
> Subject: Re: [Xen-devel] [PATCH v3 6/6] x86/viridian: implement the crash
> MSRs
> 
> >>> On 21.03.17 at 19:17, <paul.durrant@citrix.com> wrote:
> > +    case HV_X64_MSR_CRASH_CTL:
> > +    {
> > +        HV_CRASH_CTL_REG_CONTENTS ctl = {
> > +            .CrashNotify = 1,
> > +        };
> 
> This initializer failed my pre-push build test: Some gcc versions we
> still support don't allow fields of unnamed union members to be
> initialized this way. As there are at least two ways to deal with
> this, and as this is code maintained by you, I didn't want to pick
> either, as I don't know your preference.
> 

Ok, I guess I'll go for naming the union member. Do you want me to resubmit?

  Paul 

> Jan


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