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