On 06/11/18 13:44, Jan Beulich wrote:
>>>> On 05.11.18 at 12:21, <andrew.coop...@citrix.com> 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.
>> --- 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(). The other current user is the
x86 instruction emulator, where this functionality isn't the interesting
part. 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.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
