Re: [Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 05.11.18 at 12:21, <andrew.cooper3@xxxxxxxxxx> wrote:
> --- a/xen/include/xen/lib/x86/cpuid.h
> +++ b/xen/include/xen/lib/x86/cpuid.h
> @@ -20,6 +20,21 @@ struct cpuid_leaf
>      uint32_t a, b, c, d;
>  };
>  
> +static inline void cpuid_leaf(uint32_t leaf, struct cpuid_leaf *l)
> +{
> +    asm volatile ( "cpuid"
> +                   : "=a" (l->a), "=b" (l->b), "=c" (l->c), "=d" (l->d)
> +                   : "a" (leaf) );
> +}
> +
> +static inline void cpuid_count_leaf(
> +    uint32_t leaf, uint32_t subleaf, struct cpuid_leaf *l)
> +{
> +    asm volatile ( "cpuid"
> +                   : "=a" (l->a), "=b" (l->b), "=c" (l->c), "=d" (l->d)
> +                   : "a" (leaf), "c" (subleaf) );
> +}

Especially with this now being library code (i.e. side effects like
serialization not being supposed to be of interest): Why
volatile?

> --- a/xen/lib/x86/cpuid.c
> +++ b/xen/lib/x86/cpuid.c
> @@ -2,6 +2,114 @@
>  
>  #include <xen/lib/x86/cpuid.h>
>  
> +void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
> +{
> +    unsigned int i;
> +
> +    cpuid_leaf(0, &p->basic.raw[0]);
> +    for ( i = 1; i < min(ARRAY_SIZE(p->basic.raw),
> +                         p->basic.max_leaf + 1ul); ++i )
> +    {
> +        switch ( i )
> +        {
> +        case 0x4: case 0x7: case 0xb: case 0xd:
> +            /* Multi-invocation leaves.  Deferred. */
> +            continue;
> +        }
> +
> +        cpuid_leaf(i, &p->basic.raw[i]);
> +    }
> +
> +    if ( p->basic.max_leaf >= 4 )
> +    {
> +        for ( i = 0; i < ARRAY_SIZE(p->cache.raw); ++i )
> +        {
> +            union {
> +                struct cpuid_leaf l;
> +                struct cpuid_cache_leaf c;
> +            } u;
> +
> +            cpuid_count_leaf(4, i, &u.l);
> +
> +            if ( u.c.type == 0 )
> +                break;
> +
> +            p->cache.subleaf[i] = u.c;
> +        }
> +
> +        /*
> +         * The choice of CPUID_GUEST_NR_CACHE is arbitrary.  It is expected
> +         * that it will eventually need increasing for future hardware.
> +         */
> +#ifdef __XEN__
> +        if ( i == ARRAY_SIZE(p->cache.raw) )
> +            printk(XENLOG_WARNING
> +                   "CPUID: Insufficient Leaf 4 space for this hardware\n");
> +#endif

There being another similar instance further down, and possibly
new ones to appear later, plus such a warning potentially also
being of interest in the harness - would you mind abstracting
(could be as simple as making printk() and XENLOG_* available
where needed, provided there's no consumer which would
rather not want such logging) this so it can go without #ifdef-ary?

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel