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

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----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