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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 22.03.17 at 11:20, <Paul.Durrant@xxxxxxxxxx> wrote:
>> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
>> Sent: 22 March 2017 10:10
>> >>> On 21.03.17 at 19:17, <paul.durrant@xxxxxxxxxx> 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@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel