| Message ID | 1490184924-20156-2-git-send-email-paul.durrant@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> 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
> -----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
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
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(-)