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

[Paul Durrant <Paul.Durrant@xxxxxxxxxx>]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@xxxxxxxx]
> Sent: 22 March 2017 13:55
> To: Paul Durrant <Paul.Durrant@xxxxxxxxxx>
> Cc: Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>; xen-
> devel@xxxxxxxxxxxxxxxxxxxx
> 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@xxxxxxxxxx> 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


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