Re: [PATCH v2 04/10] x86/cpuid: Handle leaf 0x4 in guest_cpuid()

["Jan Beulich" <JBeulich@suse.com>]

>>> On 10.03.17 at 17:27, <andrew.cooper3@citrix.com> wrote:
> Leaf 0x4 is reserved by AMD.  For Intel, it is a multi-invocation leaf with
> ecx enumerating different cache details.
> 
> Add a new union for it in struct cpuid_policy, collect it from hardware in
> calculate_raw_policy(), audit it in recalculate_cpuid_policy() and update
> guest_cpuid() and update_domain_cpuid_info() to properly insert/extract 
> data.
> 
> A lot of the data here will need further auditing/refinement when better
> topology support is introduced, but for now, this matches the existing
> toolstack behaviour.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

but with a couple of remarks:

> @@ -242,6 +243,25 @@ static void __init calculate_raw_policy(void)
>          cpuid_leaf(i, &p->basic.raw[i]);
>      }
>  
> +    if ( p->basic.max_leaf >= 4 )
> +    {
> +        for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i )
> +        {
> +            cpuid_count_leaf(4, i, &p->cache.raw[i]);
> +
> +            if ( p->cache.subleaf[i].type == 0 )
> +                break;
> +        }
> +
> +        /*
> +         * The choice of CPUID_GUEST_NR_CACHE is arbitrary.  It is expected
> +         * that it will eventually need increasing for future hardware.
> +         */
> +        if ( i == ARRAY_SIZE(p->cache.raw) )
> +            printk(XENLOG_WARNING
> +                   "CPUID: Insufficient Leaf 4 space for this hardware\n");
> +    }

As expressed before (perhaps in the context of another patch),
the warning may be logged prematurely, which I'd prefer to be
avoided.

> --- a/xen/arch/x86/domctl.c
> +++ b/xen/arch/x86/domctl.c
> @@ -101,6 +101,10 @@ static int update_domain_cpuid_info(struct domain *d,
>      switch ( ctl->input[0] )
>      {
>      case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1:
> +        if ( ctl->input[0] == 4 &&
> +             ctl->input[1] >= ARRAY_SIZE(p->cache.raw) )
> +            return 0;
> +
>          if ( ctl->input[0] == 7 &&
>               ctl->input[1] >= ARRAY_SIZE(p->feat.raw) )
>              return 0;
> @@ -129,7 +133,9 @@ static int update_domain_cpuid_info(struct domain *d,
>      switch ( ctl->input[0] )
>      {
>      case 0x00000000 ... ARRAY_SIZE(p->basic.raw) - 1:
> -        if ( ctl->input[0] == 7 )
> +        if ( ctl->input[0] == 4 )
> +            p->cache.raw[ctl->input[1]] = leaf;
> +        else if ( ctl->input[0] == 7 )
>              p->feat.raw[ctl->input[1]] = leaf;
>          else if ( ctl->input[0] == XSTATE_CPUID )
>              p->xstate.raw[ctl->input[1]] = leaf;

The contexts of these two hunks make it pretty likely that inner
switch() statements would help readability.

Jan


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