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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Nov 05, 2018 at 11:21:03AM +0000, Andrew Cooper wrote:
> This will shortly be wanted by the userspace emulator harnesses as well.
> 
> Consolidate the cpuid{,_count}_leaf() helpers beside the structure definition,
> rather than having them scattered throughout Xen.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

I'm slightly worried about the _native prefix in the function name, if
this is run on Dom0 userspace the _native part of this is dubious,
since the policy returned is going to be provided by Xen, and thus
might not match the host one. I don't have a better name
recommendation, so I'm fine with this.

> diff --git a/xen/lib/x86/cpuid.c b/xen/lib/x86/cpuid.c
> index a63e42b..ddd3821 100644
> --- 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");

It would be good to provide some logging abstraction that can be used
by both the tools and Xen, ie:

printf(LIBX86_WARN, "...");

And then add a couple of defines for __XEN__ and __XEN_TOOLS__ as
required. Can be done in a followup patch IMO.

Thanks, Roger.

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