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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 09.11.18 at 17:24, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 06/11/18 13:44, Jan Beulich wrote:
>>>>> 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?
> 
> Force of habit, I think.  I'll drop volatile here.
> 
> We should probably do the same for Xen, although there is one place in
> the Intel ucode handler which would need adjusting to cope.

And that construct would probably better get a name which
expresses the need for / purpose of the barrier.

>>> --- 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?
> 
> Well - it was this consideration which caused me to omit it.
> 
> Realistically, the first situation to hit this message will be someone
> booting Xen on a brand new piece of hardware, so I expect changes to the
> structure size to come from vendors.
> 
> One user is the AFL fuzzer, and that definitely doesn't want to be
> spitting out a warning on every fork().

It could still use a construct to spit out a warning exactly once.
Arguably this might be a little difficult to arrange for.

>  The other current user is the
> x86 instruction emulator, where this functionality isn't the interesting
> part.

But such a warning might be a helpful explanation of why some
test failed.

>  Furthermore, I don't expect the toolstack to be making use of
> this itself, so it won't be useful to attempt to plumb the message
> through there.

Sure. Overall I'm not going to insist on extending the logging as
suggested (albeit Roger iirc had asked for it too), so with the
volatile-s above dropped and with the expectation that you
wouldn't object to extending the logging down the road, should
it turn out useful to have outside of the hypervisor
Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan


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