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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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


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