Re: [Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
>>> On 09.11.18 at 17:24, wrote:
> On 06/11/18 13:44, Jan Beulich wrote:
> On 05.11.18 at 12:21, 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
>>>
>>> +void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>>> +{
>>> +unsigned int i;
>>> +
>>> +cpuid_leaf(0, >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, >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, );
>>> +
>>> +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
Jan
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
On 06/11/18 16:31, Roger Pau Monné wrote:
> On Mon, Nov 05, 2018 at 11:21:03AM +, 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
> Reviewed-by: Roger Pau Monné
>
> 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.
Well - it is a native instruction, whether or not there is a outer
hypervisor or masking/faulting configuration playing with the values seen.
I'll see about mentioning this in the function documentation.
~Andrew
___
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
On 06/11/18 13:44, Jan Beulich wrote:
On 05.11.18 at 12:21, 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
>>
>> +void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
>> +{
>> +unsigned int i;
>> +
>> +cpuid_leaf(0, >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, >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, );
>> +
>> +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
Re: [Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
On Mon, Nov 05, 2018 at 11:21:03AM +, 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
Reviewed-by: Roger Pau Monné
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
>
> +void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
> +{
> +unsigned int i;
> +
> +cpuid_leaf(0, >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, >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, );
> +
> +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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
>>> On 05.11.18 at 12:21, 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
>
> +void x86_cpuid_policy_fill_native(struct cpuid_policy *p)
> +{
> +unsigned int i;
> +
> +cpuid_leaf(0, >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, >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, );
> +
> +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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
[Xen-devel] [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
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
---
CC: Jan Beulich
CC: Wei Liu
---
xen/arch/x86/cpuid.c| 105 +-
xen/include/asm-x86/processor.h | 6 ---
xen/include/xen/lib/x86/cpuid.h | 18 +++
xen/lib/x86/cpuid.c | 108
4 files changed, 127 insertions(+), 110 deletions(-)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index d21e745..0591a7d 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -75,11 +75,6 @@ struct cpuid_policy __read_mostly raw_cpuid_policy,
__read_mostly pv_max_cpuid_policy,
__read_mostly hvm_max_cpuid_policy;
-static void cpuid_leaf(uint32_t leaf, struct cpuid_leaf *data)
-{
-cpuid(leaf, >a, >b, >c, >d);
-}
-
static void sanitise_featureset(uint32_t *fs)
{
/* for_each_set_bit() uses unsigned longs. Extend with zeroes. */
@@ -273,106 +268,8 @@ static void recalculate_misc(struct cpuid_policy *p)
static void __init calculate_raw_policy(void)
{
struct cpuid_policy *p = _cpuid_policy;
-unsigned int i;
-
-cpuid_leaf(0, >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, >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, );
-
-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.
- */
-if ( i == ARRAY_SIZE(p->cache.raw) )
-printk(XENLOG_WARNING
- "CPUID: Insufficient Leaf 4 space for this hardware\n");
-}
-
-if ( p->basic.max_leaf >= 7 )
-{
-cpuid_count_leaf(7, 0, >feat.raw[0]);
-
-for ( i = 1; i < min(ARRAY_SIZE(p->feat.raw),
- p->feat.max_subleaf + 1ul); ++i )
-cpuid_count_leaf(7, i, >feat.raw[i]);
-}
-
-if ( p->basic.max_leaf >= 0xb )
-{
-union {
-struct cpuid_leaf l;
-struct cpuid_topo_leaf t;
-} u;
-
-for ( i = 0; i < ARRAY_SIZE(p->topo.raw); ++i )
-{
-cpuid_count_leaf(0xb, i, );
-
-if ( u.t.type == 0 )
-break;
-
-p->topo.subleaf[i] = u.t;
-}
-
-/*
- * The choice of CPUID_GUEST_NR_TOPO is per the manual. It may need
- * to grow for future hardware.
- */
-if ( i == ARRAY_SIZE(p->topo.raw) &&
- (cpuid_count_leaf(0xb, i, ), u.t.type != 0) )
-printk(XENLOG_WARNING
- "CPUID: Insufficient Leaf 0xb space for this hardware\n");
-}
-
-if ( p->basic.max_leaf >= XSTATE_CPUID )
-{
-uint64_t xstates;
-
-cpuid_count_leaf(XSTATE_CPUID, 0, >xstate.raw[0]);
-cpuid_count_leaf(XSTATE_CPUID, 1, >xstate.raw[1]);
-
-xstates = ((uint64_t)(p->xstate.xcr0_high | p->xstate.xss_high) << 32)
|
-(p->xstate.xcr0_low | p->xstate.xss_low);
-
-for ( i = 2; i < min(63ul, ARRAY_SIZE(p->xstate.raw)); ++i )
-{
-if ( xstates & (1ul << i) )
-cpuid_count_leaf(XSTATE_CPUID, i, >xstate.raw[i]);
-}
-}
-/* Extended leaves. */
-cpuid_leaf(0x8000, >extd.raw[0]);
-for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw),
- p->extd.max_leaf + 1 - 0x8000ul); ++i )
-cpuid_leaf(0x8000 + i, >extd.raw[i]);
+x86_cpuid_policy_fill_native(p);
p->x86_vendor = boot_cpu_data.x86_vendor;
}
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 03555e1..df01ae3 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -263,12 +263,6 @@ static always_inline unsigned int cpuid_count_ebx(
return ebx;
}
-static always_inline void cpuid_count_leaf(uint32_t leaf, uint32_t subleaf,
- struct cpuid_leaf *data)
-{
-cpuid_count(leaf, subleaf, >a, >b, >c, >d);
-}
-
static inline unsigned long read_cr0(void)
{
unsigned long cr0;
diff --git a/xen/include/xen/lib/x86/cpuid.h b/xen/include/xen/lib/x86/cpuid.h
index 93ada23..5e4ec09 100644
--- a/xen/include/xen/lib/x86/cpuid.h
