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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

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


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