* [PATCH 1/6] tools: Move the typesafe min/max helpers into xen-tools/libs.h
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-05 11:23 ` Wei Liu
2018-11-05 11:21 ` [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy() Andrew Cooper
` (4 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Ian Jackson
... rather than implementing them separately for libxc and libxl. They will
shortly be wanted in libx86 as well.
Fix up the style/consistency in the declaration, but no functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/include/xen-tools/libs.h | 38 ++++++++++++++++++++++++++++++++++++++
tools/libxc/xc_private.h | 16 ----------------
tools/libxl/libxl_internal.h | 16 ----------------
3 files changed, 38 insertions(+), 32 deletions(-)
diff --git a/tools/include/xen-tools/libs.h b/tools/include/xen-tools/libs.h
index 927e057..cc7dfc8 100644
--- a/tools/include/xen-tools/libs.h
+++ b/tools/include/xen-tools/libs.h
@@ -21,4 +21,42 @@
#define MIN(x, y) ((x) < (y) ? (x) : (y))
#endif
+#ifndef min
+#define min(x, y) \
+ ({ \
+ const typeof(x) _x = (x); \
+ const typeof(y) _y = (y); \
+ (void) (&_x == &_y); \
+ (_x < _y) ? _x : _y; \
+ })
+#endif
+
+#ifndef max
+#define max(x, y) \
+ ({ \
+ const typeof(x) _x = (x); \
+ const typeof(y) _y = (y); \
+ (void)(&_x == &_y); \
+ (_x > _y) ? _x : _y; \
+ })
+#endif
+
+#ifndef min_t
+#define min_t(type, x, y) \
+ ({ \
+ const type _x = (x); \
+ const type _y = (y); \
+ (_x < _y) ? _x: _y; \
+ })
+#endif
+
+#ifndef max_t
+#define max_t(type, x, y) \
+ ({ \
+ const type _x = (x); \
+ const type _y = (y); \
+ (_x > _y) ? _x: _y; \
+ })
+#endif
+
#endif /* __XEN_TOOLS_LIBS__ */
diff --git a/tools/libxc/xc_private.h b/tools/libxc/xc_private.h
index 705eaa9..adc3b6a 100644
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -405,22 +405,6 @@ int xc_ffs16(uint16_t x);
int xc_ffs32(uint32_t x);
int xc_ffs64(uint64_t x);
-#define min(X, Y) ({ \
- const typeof (X) _x = (X); \
- const typeof (Y) _y = (Y); \
- (void) (&_x == &_y); \
- (_x < _y) ? _x : _y; })
-#define max(X, Y) ({ \
- const typeof (X) _x = (X); \
- const typeof (Y) _y = (Y); \
- (void) (&_x == &_y); \
- (_x > _y) ? _x : _y; })
-
-#define min_t(type,x,y) \
- ({ type __x = (x); type __y = (y); __x < __y ? __x: __y; })
-#define max_t(type,x,y) \
- ({ type __x = (x); type __y = (y); __x > __y ? __x: __y; })
-
#define DOMPRINTF(fmt, args...) xc_dom_printf(dom->xch, fmt, ## args)
#define DOMPRINTF_CALLED(xch) xc_dom_printf((xch), "%s: called", __FUNCTION__)
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 153566a..ff88938 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -138,22 +138,6 @@
#define MASK_EXTR(v, m) (((v) & (m)) / ((m) & -(m)))
#define MASK_INSR(v, m) (((v) * ((m) & -(m))) & (m))
-#define min(X, Y) ({ \
- const typeof (X) _x = (X); \
- const typeof (Y) _y = (Y); \
- (void) (&_x == &_y); \
- (_x < _y) ? _x : _y; })
-#define max(X, Y) ({ \
- const typeof (X) _x = (X); \
- const typeof (Y) _y = (Y); \
- (void) (&_x == &_y); \
- (_x > _y) ? _x : _y; })
-
-#define min_t(type, x, y) \
- ({ const type _x = (x); const type _y = (y); _x < _y ? _x: _y; })
-#define max_t(type, x, y) \
- ({ const type _x = (x); const type _y = (y); _x > _y ? _x: _y; })
-
#define LIBXL__LOGGING_ENABLED
#ifdef LIBXL__LOGGING_ENABLED
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
2018-11-05 11:21 ` [PATCH 1/6] tools: Move the typesafe min/max helpers into xen-tools/libs.h Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-06 13:44 ` Jan Beulich
2018-11-06 16:31 ` Roger Pau Monné
2018-11-05 11:21 ` [PATCH 3/6] tools/x86emul: Use struct cpuid_policy in the userspace test harnesses Andrew Cooper
` (3 subsequent siblings)
5 siblings, 2 replies; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
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@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
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, &data->a, &data->b, &data->c, &data->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 = &raw_cpuid_policy;
- 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.
- */
- 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, &p->feat.raw[0]);
-
- for ( i = 1; i < min(ARRAY_SIZE(p->feat.raw),
- p->feat.max_subleaf + 1ul); ++i )
- cpuid_count_leaf(7, i, &p->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, &u.l);
-
- 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.l), 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, &p->xstate.raw[0]);
- cpuid_count_leaf(XSTATE_CPUID, 1, &p->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, &p->xstate.raw[i]);
- }
- }
- /* Extended leaves. */
- cpuid_leaf(0x80000000, &p->extd.raw[0]);
- for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw),
- p->extd.max_leaf + 1 - 0x80000000ul); ++i )
- cpuid_leaf(0x80000000 + i, &p->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, &data->a, &data->b, &data->c, &data->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
+++ 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) );
+}
+
#define CPUID_GUEST_NR_BASIC (0xdu + 1)
#define CPUID_GUEST_NR_FEAT (0u + 1)
#define CPUID_GUEST_NR_CACHE (5u + 1)
@@ -228,6 +243,9 @@ static inline void cpuid_featureset_to_policy(
p->feat._7d0 = fs[FEATURESET_7d0];
}
+/* Fill a CPUID policy using the native CPUID instruction. */
+void x86_cpuid_policy_fill_native(struct cpuid_policy *p);
+
const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature);
#endif /* !XEN_LIB_X86_CPUID_H */
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");
+#endif
+ }
+
+ if ( p->basic.max_leaf >= 7 )
+ {
+ cpuid_count_leaf(7, 0, &p->feat.raw[0]);
+
+ for ( i = 1; i < min(ARRAY_SIZE(p->feat.raw),
+ p->feat.max_subleaf + 1ul); ++i )
+ cpuid_count_leaf(7, i, &p->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, &u.l);
+
+ 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.
+ */
+#ifdef __XEN__
+ if ( i == ARRAY_SIZE(p->topo.raw) &&
+ (cpuid_count_leaf(0xb, i, &u.l), u.t.type != 0) )
+ printk(XENLOG_WARNING
+ "CPUID: Insufficient Leaf 0xb space for this hardware\n");
+#endif
+ }
+
+ if ( p->basic.max_leaf >= 0xd )
+ {
+ uint64_t xstates;
+
+ cpuid_count_leaf(0xd, 0, &p->xstate.raw[0]);
+ cpuid_count_leaf(0xd, 1, &p->xstate.raw[1]);
+
+ xstates = ((uint64_t)(p->xstate.xcr0_high | p->xstate.xss_high) << 32);
+ xstates |= (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(0xd, i, &p->xstate.raw[i]);
+ }
+ }
+
+ /* Extended leaves. */
+ cpuid_leaf(0x80000000, &p->extd.raw[0]);
+ for ( i = 1; i < min(ARRAY_SIZE(p->extd.raw),
+ p->extd.max_leaf + 1 - 0x80000000ul); ++i )
+ cpuid_leaf(0x80000000 + i, &p->extd.raw[i]);
+}
+
const uint32_t *x86_cpuid_lookup_deep_deps(uint32_t feature)
{
static const uint32_t deep_features[] = INIT_DEEP_FEATURES;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-05 11:21 ` [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy() Andrew Cooper
@ 2018-11-06 13:44 ` Jan Beulich
2018-11-09 16:24 ` Andrew Cooper
2018-11-06 16:31 ` Roger Pau Monné
1 sibling, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2018-11-06 13:44 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 05.11.18 at 12:21, <andrew.cooper3@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?
> --- 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-06 13:44 ` Jan Beulich
@ 2018-11-09 16:24 ` Andrew Cooper
2018-11-12 8:13 ` Jan Beulich
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-09 16:24 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
[-- Attachment #1.1: Type: text/plain, Size: 3574 bytes --]
On 06/11/18 13:44, Jan Beulich wrote:
>>>> On 05.11.18 at 12:21, <andrew.cooper3@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
[-- Attachment #1.2: Type: text/html, Size: 4450 bytes --]
[-- Attachment #2: Type: text/plain, Size: 157 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-09 16:24 ` Andrew Cooper
@ 2018-11-12 8:13 ` Jan Beulich
0 siblings, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2018-11-12 8:13 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 09.11.18 at 17:24, <andrew.cooper3@citrix.com> wrote:
> On 06/11/18 13:44, Jan Beulich wrote:
>>>>> On 05.11.18 at 12:21, <andrew.cooper3@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.
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@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-05 11:21 ` [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy() Andrew Cooper
2018-11-06 13:44 ` Jan Beulich
@ 2018-11-06 16:31 ` Roger Pau Monné
2018-11-09 16:26 ` Andrew Cooper
1 sibling, 1 reply; 25+ messages in thread
From: Roger Pau Monné @ 2018-11-06 16:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Jan Beulich, Xen-devel
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@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy()
2018-11-06 16:31 ` Roger Pau Monné
@ 2018-11-09 16:26 ` Andrew Cooper
0 siblings, 0 replies; 25+ messages in thread
From: Andrew Cooper @ 2018-11-09 16:26 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: Wei Liu, Jan Beulich, Xen-devel
On 06/11/18 16:31, Roger Pau Monné wrote:
> 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@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
> 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
^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/6] tools/x86emul: Use struct cpuid_policy in the userspace test harnesses
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
2018-11-05 11:21 ` [PATCH 1/6] tools: Move the typesafe min/max helpers into xen-tools/libs.h Andrew Cooper
2018-11-05 11:21 ` [PATCH 2/6] libx86: Split x86_cpuid_policy_fill_native() out of calculate_raw_policy() Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-06 15:25 ` Jan Beulich
2018-11-05 11:21 ` [PATCH 4/6] x86/emul: Pass a full cpuid_policy into x86_emulate() Andrew Cooper
` (2 subsequent siblings)
5 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
This will shortly be required to pass into the emulator itself.
Simplify the shared cpu_has_* helpers by reading the correct bit out of the
CPUID policy itself.
No (intended) change in behaviour.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/fuzz/x86_instruction_emulator/Makefile | 9 +-
tools/tests/x86_emulator/Makefile | 7 +-
tools/tests/x86_emulator/x86-emulate.c | 12 ++
tools/tests/x86_emulator/x86-emulate.h | 217 +++++----------------------
4 files changed, 58 insertions(+), 187 deletions(-)
diff --git a/tools/fuzz/x86_instruction_emulator/Makefile b/tools/fuzz/x86_instruction_emulator/Makefile
index eb88f94..a55bc78 100644
--- a/tools/fuzz/x86_instruction_emulator/Makefile
+++ b/tools/fuzz/x86_instruction_emulator/Makefile
@@ -8,6 +8,9 @@ else
x86-insn-fuzz-all:
endif
+# Add libx86 to the build
+vpath %.c $(XEN_ROOT)/xen/lib/x86
+
x86_emulate:
[ -L $@ ] || ln -sf $(XEN_ROOT)/xen/arch/x86/$@
@@ -31,13 +34,13 @@ x86-emulate.o x86-emulate-cov.o: x86_emulate/x86_emulate.c $(x86_emulate.h)
fuzz-emul.o fuzz-emulate-cov.o wrappers.o: $(x86_emulate.h)
-x86-insn-fuzzer.a: fuzz-emul.o x86-emulate.o
+x86-insn-fuzzer.a: fuzz-emul.o x86-emulate.o cpuid.o
$(AR) rc $@ $^
-afl-harness: afl-harness.o fuzz-emul.o x86-emulate.o wrappers.o
+afl-harness: afl-harness.o fuzz-emul.o x86-emulate.o cpuid.o wrappers.o
$(CC) $(CFLAGS) $^ -o $@
-afl-harness-cov: afl-harness-cov.o fuzz-emul-cov.o x86-emulate-cov.o wrappers.o
+afl-harness-cov: afl-harness-cov.o fuzz-emul-cov.o x86-emulate-cov.o cpuid.o wrappers.o
$(CC) $(CFLAGS) $(GCOV_FLAGS) $^ -o $@
# Common targets
diff --git a/tools/tests/x86_emulator/Makefile b/tools/tests/x86_emulator/Makefile
index a97c43b..b2f8e44 100644
--- a/tools/tests/x86_emulator/Makefile
+++ b/tools/tests/x86_emulator/Makefile
@@ -11,6 +11,11 @@ all: $(TARGET)
run: $(TARGET)
./$(TARGET)
+# Add libx86 to the build
+vpath %.c $(XEN_ROOT)/xen/lib/x86
+
+CFLAGS += $(CFLAGS_xeninclude)
+
SIMD := 3dnow sse sse2 sse4 avx avx2 xop
FMA := fma4 fma
SG := avx2-sg
@@ -139,7 +144,7 @@ $(addsuffix .h,$(SIMD) $(FMA) $(SG)): simd.h
xop.h: simd-fma.c
-$(TARGET): x86-emulate.o test_x86_emulator.o wrappers.o
+$(TARGET): x86-emulate.o cpuid.o test_x86_emulator.o wrappers.o
$(HOSTCC) $(HOSTCFLAGS) -o $@ $^
.PHONY: clean
diff --git a/tools/tests/x86_emulator/x86-emulate.c b/tools/tests/x86_emulator/x86-emulate.c
index aba5768..a109e93 100644
--- a/tools/tests/x86_emulator/x86-emulate.c
+++ b/tools/tests/x86_emulator/x86-emulate.c
@@ -25,6 +25,7 @@
#define put_stub(stb) ((stb).addr = 0)
uint32_t mxcsr_mask = 0x0000ffbf;
+struct cpuid_policy cp;
static char fpu_save_area[4096] __attribute__((__aligned__((64))));
static bool use_xsave;
@@ -64,6 +65,17 @@ bool emul_test_init(void)
unsigned long sp;
+ x86_cpuid_policy_fill_native(&cp);
+
+ /*
+ * The emulator doesn't use these instructions, so can always emulate
+ * them.
+ */
+ cp.basic.movbe = true;
+ cp.feat.adx = true;
+ cp.feat.rdpid = true;
+ cp.extd.clzero = true;
+
if ( cpu_has_xsave )
{
unsigned int tmp, ebx;
diff --git a/tools/tests/x86_emulator/x86-emulate.h b/tools/tests/x86_emulator/x86-emulate.h
index ef58466..8bb02fd 100644
--- a/tools/tests/x86_emulator/x86-emulate.h
+++ b/tools/tests/x86_emulator/x86-emulate.h
@@ -62,6 +62,7 @@
#define is_canonical_address(x) (((int64_t)(x) >> 47) == ((int64_t)(x) >> 63))
extern uint32_t mxcsr_mask;
+extern struct cpuid_policy cp;
#define MMAP_SZ 16384
bool emul_test_init(void);
@@ -104,191 +105,41 @@ static inline uint64_t xgetbv(uint32_t xcr)
return ((uint64_t)hi << 32) | lo;
}
-#define cache_line_size() ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- res.d & (1U << 19) ? (res.b >> 5) & 0x7f8 : 0; \
-})
-
-#define cpu_has_mmx ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.d & (1U << 23)) != 0; \
-})
-
-#define cpu_has_fxsr ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.d & (1U << 24)) != 0; \
-})
-
-#define cpu_has_sse ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.d & (1U << 25)) != 0; \
-})
-
-#define cpu_has_sse2 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.d & (1U << 26)) != 0; \
-})
-
-#define cpu_has_sse3 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.c & (1U << 0)) != 0; \
-})
-
-#define cpu_has_fma ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.c = 0; \
- (res.c & (1U << 12)) != 0; \
-})
-
-#define cpu_has_sse4_1 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.c & (1U << 19)) != 0; \
-})
-
-#define cpu_has_sse4_2 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.c & (1U << 20)) != 0; \
-})
-
-#define cpu_has_popcnt ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- (res.c & (1U << 23)) != 0; \
-})
-
-#define cpu_has_xsave ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- /* Intentionally checking OSXSAVE here. */ \
- (res.c & (1U << 27)) != 0; \
-})
-
-#define cpu_has_avx ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.c = 0; \
- (res.c & (1U << 28)) != 0; \
-})
-
-#define cpu_has_f16c ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.c = 0; \
- (res.c & (1U << 29)) != 0; \
-})
+/* Intentionally checking OSXSAVE here. */
+#define cpu_has_xsave (cp.basic.raw[1].c & (1u << 27))
-#define cpu_has_avx2 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.b = 0; \
- else { \
- emul_test_cpuid(7, 0, &res, NULL); \
- } \
- (res.b & (1U << 5)) != 0; \
-})
-
-#define cpu_has_xgetbv1 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) ) \
- res.a = 0; \
- else \
- emul_test_cpuid(0xd, 1, &res, NULL); \
- (res.a & (1U << 2)) != 0; \
-})
-
-#define cpu_has_bmi1 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(7, 0, &res, NULL); \
- (res.b & (1U << 3)) != 0; \
-})
-
-#define cpu_has_bmi2 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(7, 0, &res, NULL); \
- (res.b & (1U << 8)) != 0; \
-})
-
-#define cpu_has_3dnow_ext ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(0x80000001, 0, &res, NULL); \
- (res.d & (1U << 30)) != 0; \
-})
-
-#define cpu_has_sse4a ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(0x80000001, 0, &res, NULL); \
- (res.c & (1U << 6)) != 0; \
-})
-
-#define cpu_has_xop ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.c = 0; \
- else \
- emul_test_cpuid(0x80000001, 0, &res, NULL); \
- (res.c & (1U << 11)) != 0; \
-})
-
-#define cpu_has_fma4 ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 6) != 6) ) \
- res.c = 0; \
- else \
- emul_test_cpuid(0x80000001, 0, &res, NULL); \
- (res.c & (1U << 16)) != 0; \
-})
-
-#define cpu_has_tbm ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(0x80000001, 0, &res, NULL); \
- (res.c & (1U << 21)) != 0; \
-})
-
-#define cpu_has_avx512f ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 0xe6) != 0xe6) ) \
- res.b = 0; \
- else \
- emul_test_cpuid(7, 0, &res, NULL); \
- (res.b & (1U << 16)) != 0; \
-})
-
-#define cpu_has_avx512dq ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 0xe6) != 0xe6) ) \
- res.b = 0; \
- else \
- emul_test_cpuid(7, 0, &res, NULL); \
- (res.b & (1U << 17)) != 0; \
-})
+static inline bool xcr0_mask(uint64_t mask)
+{
+ return cpu_has_xsave && ((xgetbv(0) & mask) == mask);
+}
-#define cpu_has_avx512bw ({ \
- struct cpuid_leaf res; \
- emul_test_cpuid(1, 0, &res, NULL); \
- if ( !(res.c & (1U << 27)) || ((xgetbv(0) & 0xe6) != 0xe6) ) \
- res.b = 0; \
- else \
- emul_test_cpuid(7, 0, &res, NULL); \
- (res.b & (1U << 30)) != 0; \
-})
+#define cache_line_size() (cp.basic.clflush_size * 8)
+#define cpu_has_mmx (cp.basic.mmx)
+#define cpu_has_fxsr (cp.basic.fxsr)
+#define cpu_has_sse (cp.basic.sse)
+#define cpu_has_sse2 (cp.basic.sse2)
+#define cpu_has_sse3 (cp.basic.sse3)
+#define cpu_has_fma (xcr0_mask(6) && cp.basic.fma)
+#define cpu_has_sse4_1 (cp.basic.sse4_1)
+#define cpu_has_sse4_2 (cp.basic.sse4_2)
+#define cpu_has_popcnt (cp.basic.popcnt)
+#define cpu_has_avx (xcr0_mask(6) && cp.basic.avx)
+#define cpu_has_f16c (xcr0_mask(6) && cp.basic.f16c)
+
+#define cpu_has_avx2 (xcr0_mask(6) && cp.feat.avx2)
+#define cpu_has_bmi1 (cp.feat.bmi1)
+#define cpu_has_bmi2 (cp.feat.bmi2)
+#define cpu_has_avx512f (xcr0_mask(0xe6) && cp.feat.avx512f)
+#define cpu_has_avx512dq (xcr0_mask(0xe6) && cp.feat.avx512dq)
+#define cpu_has_avx512bw (xcr0_mask(0xe6) && cp.feat.avx512bw)
+
+#define cpu_has_xgetbv1 (cpu_has_xsave && cp.xstate.xgetbv1)
+
+#define cpu_has_3dnow_ext (cp.extd._3dnowext)
+#define cpu_has_sse4a (cp.extd.sse4a)
+#define cpu_has_xop (xcr0_mask(6) && cp.extd.xop)
+#define cpu_has_fma4 (xcr0_mask(6) && cp.extd.fma4)
+#define cpu_has_tbm (cp.extd.tbm)
int emul_test_cpuid(
uint32_t leaf,
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 4/6] x86/emul: Pass a full cpuid_policy into x86_emulate()
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
` (2 preceding siblings ...)
2018-11-05 11:21 ` [PATCH 3/6] tools/x86emul: Use struct cpuid_policy in the userspace test harnesses Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-06 15:28 ` Jan Beulich
2018-11-05 11:21 ` [PATCH 5/6] x86/emul: Don't use the ->cpuid() hook for feature checks Andrew Cooper
2018-11-05 11:21 ` [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid() Andrew Cooper
5 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
This will be used to simplify feature checking.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
tools/fuzz/x86_instruction_emulator/fuzz-emul.c | 1 +
tools/tests/x86_emulator/test_x86_emulator.c | 2 +-
xen/arch/x86/hvm/emulate.c | 2 +-
xen/arch/x86/mm/shadow/common.c | 2 +-
xen/arch/x86/pv/emul-priv-op.c | 2 +-
xen/arch/x86/pv/ro-page-fault.c | 2 +-
xen/arch/x86/x86_emulate/x86_emulate.c | 7 ++++---
xen/arch/x86/x86_emulate/x86_emulate.h | 4 ++--
8 files changed, 12 insertions(+), 10 deletions(-)
diff --git a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
index 0ffd0fb..8d0ea02 100644
--- a/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
+++ b/tools/fuzz/x86_instruction_emulator/fuzz-emul.c
@@ -827,6 +827,7 @@ int LLVMFuzzerTestOneInput(const uint8_t *data_p, size_t size)
struct x86_emulate_ctxt ctxt = {
.data = &state,
.regs = &input.regs,
+ .cpuid = &cp,
.addr_size = 8 * sizeof(void *),
.sp_size = 8 * sizeof(void *),
};
diff --git a/tools/tests/x86_emulator/test_x86_emulator.c b/tools/tests/x86_emulator/test_x86_emulator.c
index ed5a3d8..286abb7 100644
--- a/tools/tests/x86_emulator/test_x86_emulator.c
+++ b/tools/tests/x86_emulator/test_x86_emulator.c
@@ -434,7 +434,7 @@ int main(int argc, char **argv)
ctxt.regs = ®s;
ctxt.force_writeback = 0;
- ctxt.vendor = X86_VENDOR_UNKNOWN;
+ ctxt.cpuid = &cp;
ctxt.lma = sizeof(void *) == 8;
ctxt.addr_size = 8 * sizeof(void *);
ctxt.sp_size = 8 * sizeof(void *);
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 9e7deaa..91fa9db 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2543,7 +2543,7 @@ void hvm_emulate_init_once(
hvmemul_ctxt->validate = validate;
hvmemul_ctxt->ctxt.regs = regs;
- hvmemul_ctxt->ctxt.vendor = curr->domain->arch.cpuid->x86_vendor;
+ hvmemul_ctxt->ctxt.cpuid = curr->domain->arch.cpuid;
hvmemul_ctxt->ctxt.force_writeback = true;
}
diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index d54a848..4526055 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -140,7 +140,7 @@ const struct x86_emulate_ops *shadow_init_emulation(
memset(sh_ctxt, 0, sizeof(*sh_ctxt));
sh_ctxt->ctxt.regs = regs;
- sh_ctxt->ctxt.vendor = v->domain->arch.cpuid->x86_vendor;
+ sh_ctxt->ctxt.cpuid = v->domain->arch.cpuid;
sh_ctxt->ctxt.lma = hvm_long_mode_active(v);
/* Segment cache initialisation. Primed with CS. */
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index f73ea4a..5968f99 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1299,7 +1299,7 @@ int pv_emulate_privileged_op(struct cpu_user_regs *regs)
struct domain *currd = curr->domain;
struct priv_op_ctxt ctxt = {
.ctxt.regs = regs,
- .ctxt.vendor = currd->arch.cpuid->x86_vendor,
+ .ctxt.cpuid = currd->arch.cpuid,
.ctxt.lma = !is_pv_32bit_domain(currd),
};
int rc;
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index e7a7179..9d4913d 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -351,7 +351,7 @@ int pv_ro_page_fault(unsigned long addr, struct cpu_user_regs *regs)
unsigned int addr_size = is_pv_32bit_domain(currd) ? 32 : BITS_PER_LONG;
struct x86_emulate_ctxt ctxt = {
.regs = regs,
- .vendor = currd->arch.cpuid->x86_vendor,
+ .cpuid = currd->arch.cpuid,
.addr_size = addr_size,
.sp_size = addr_size,
.lma = addr_size > 32,
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index e69dfdd..8bff02a 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1838,6 +1838,7 @@ protmode_load_seg(
struct x86_emulate_ctxt *ctxt,
const struct x86_emulate_ops *ops)
{
+ const struct cpuid_policy *cp = ctxt->cpuid;
enum x86_segment sel_seg = (sel & 4) ? x86_seg_ldtr : x86_seg_gdtr;
struct { uint32_t a, b; } desc, desc_hi = {};
uint8_t dpl, rpl;
@@ -1862,7 +1863,7 @@ protmode_load_seg(
case x86_seg_tr:
goto raise_exn;
}
- if ( ctxt->vendor != X86_VENDOR_AMD || !ops->read_segment ||
+ if ( cp->x86_vendor != X86_VENDOR_AMD || !ops->read_segment ||
ops->read_segment(seg, sreg, ctxt) != X86EMUL_OKAY )
memset(sreg, 0, sizeof(*sreg));
else
@@ -1989,7 +1990,7 @@ protmode_load_seg(
*/
bool wide = desc.b & 0x1000
? false : (desc.b & 0xf00) != 0xc00 &&
- ctxt->vendor != X86_VENDOR_AMD
+ cp->x86_vendor != X86_VENDOR_AMD
? mode_64bit() : ctxt->lma;
if ( wide )
@@ -2007,7 +2008,7 @@ protmode_load_seg(
default:
return rc;
}
- if ( !mode_64bit() && ctxt->vendor == X86_VENDOR_AMD &&
+ if ( !mode_64bit() && cp->x86_vendor == X86_VENDOR_AMD &&
(desc.b & 0xf00) != 0xc00 )
desc_hi.b = desc_hi.a = 0;
if ( (desc_hi.b & 0x00001f00) ||
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 3750f0c..0397c1d 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -501,8 +501,8 @@ struct x86_emulate_ctxt
* Input-only state:
*/
- /* CPU vendor (X86_VENDOR_UNKNOWN for "don't care") */
- unsigned char vendor;
+ /* CPUID Policy for the domain. */
+ const struct cpuid_policy *cpuid;
/* Set this if writes may have side effects. */
bool force_writeback;
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 5/6] x86/emul: Don't use the ->cpuid() hook for feature checks
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
` (3 preceding siblings ...)
2018-11-05 11:21 ` [PATCH 4/6] x86/emul: Pass a full cpuid_policy into x86_emulate() Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-06 15:32 ` Jan Beulich
2018-11-05 11:21 ` [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid() Andrew Cooper
5 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
For a release build of xen, this removes almost 4k of code volume, and removes
a function pointer call from every instantiation.
add/remove: 0/1 grow/shrink: 0/3 up/down: 0/-3877 (-3877)
Function old new delta
adjust_bnd 227 205 -22
x86_decode 8136 8096 -40
vcpu_has.isra 133 - -133
x86_emulate 118687 115005 -3682
Total: Before=3309200, After=3305323, chg -0.12%
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/x86_emulate/x86_emulate.c | 140 ++++++++++++---------------------
1 file changed, 52 insertions(+), 88 deletions(-)
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index 8bff02a..3960c8e 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -1694,91 +1694,58 @@ in_protmode(
return !(in_realmode(ctxt, ops) || (ctxt->regs->eflags & X86_EFLAGS_VM));
}
-#define EAX 0
-#define ECX 1
-#define EDX 2
-#define EBX 3
-
-static bool vcpu_has(
- unsigned int eax,
- unsigned int reg,
- unsigned int bit,
- struct x86_emulate_ctxt *ctxt,
- const struct x86_emulate_ops *ops)
-{
- struct cpuid_leaf res;
- int rc = X86EMUL_OKAY;
-
- fail_if(!ops->cpuid);
- rc = ops->cpuid(eax, 0, &res, ctxt);
- if ( rc == X86EMUL_OKAY )
- {
- switch ( reg )
- {
- case EAX: reg = res.a; break;
- case EBX: reg = res.b; break;
- case ECX: reg = res.c; break;
- case EDX: reg = res.d; break;
- default: BUG();
- }
- if ( !(reg & (1U << bit)) )
- rc = ~X86EMUL_OKAY;
- }
-
- done:
- return rc == X86EMUL_OKAY;
-}
-
-#define vcpu_has_fpu() vcpu_has( 1, EDX, 0, ctxt, ops)
-#define vcpu_has_sep() vcpu_has( 1, EDX, 11, ctxt, ops)
-#define vcpu_has_cx8() vcpu_has( 1, EDX, 8, ctxt, ops)
-#define vcpu_has_cmov() vcpu_has( 1, EDX, 15, ctxt, ops)
-#define vcpu_has_clflush() vcpu_has( 1, EDX, 19, ctxt, ops)
-#define vcpu_has_mmx() vcpu_has( 1, EDX, 23, ctxt, ops)
-#define vcpu_has_sse() vcpu_has( 1, EDX, 25, ctxt, ops)
-#define vcpu_has_sse2() vcpu_has( 1, EDX, 26, ctxt, ops)
-#define vcpu_has_sse3() vcpu_has( 1, ECX, 0, ctxt, ops)
-#define vcpu_has_pclmulqdq() vcpu_has( 1, ECX, 1, ctxt, ops)
-#define vcpu_has_ssse3() vcpu_has( 1, ECX, 9, ctxt, ops)
-#define vcpu_has_fma() vcpu_has( 1, ECX, 12, ctxt, ops)
-#define vcpu_has_cx16() vcpu_has( 1, ECX, 13, ctxt, ops)
-#define vcpu_has_sse4_1() vcpu_has( 1, ECX, 19, ctxt, ops)
-#define vcpu_has_sse4_2() vcpu_has( 1, ECX, 20, ctxt, ops)
-#define vcpu_has_movbe() vcpu_has( 1, ECX, 22, ctxt, ops)
-#define vcpu_has_popcnt() vcpu_has( 1, ECX, 23, ctxt, ops)
-#define vcpu_has_aesni() vcpu_has( 1, ECX, 25, ctxt, ops)
-#define vcpu_has_avx() vcpu_has( 1, ECX, 28, ctxt, ops)
-#define vcpu_has_f16c() vcpu_has( 1, ECX, 29, ctxt, ops)
-#define vcpu_has_rdrand() vcpu_has( 1, ECX, 30, ctxt, ops)
-#define vcpu_has_mmxext() (vcpu_has(0x80000001, EDX, 22, ctxt, ops) || \
+#define vcpu_has_fpu() (ctxt->cpuid->basic.fpu)
+#define vcpu_has_sep() (ctxt->cpuid->basic.sep)
+#define vcpu_has_cx8() (ctxt->cpuid->basic.cx8)
+#define vcpu_has_cmov() (ctxt->cpuid->basic.cmov)
+#define vcpu_has_clflush() (ctxt->cpuid->basic.clflush)
+#define vcpu_has_mmx() (ctxt->cpuid->basic.mmx)
+#define vcpu_has_sse() (ctxt->cpuid->basic.sse)
+#define vcpu_has_sse2() (ctxt->cpuid->basic.sse2)
+#define vcpu_has_sse3() (ctxt->cpuid->basic.sse3)
+#define vcpu_has_pclmulqdq() (ctxt->cpuid->basic.pclmulqdq)
+#define vcpu_has_ssse3() (ctxt->cpuid->basic.ssse3)
+#define vcpu_has_fma() (ctxt->cpuid->basic.fma)
+#define vcpu_has_cx16() (ctxt->cpuid->basic.cx16)
+#define vcpu_has_sse4_1() (ctxt->cpuid->basic.sse4_1)
+#define vcpu_has_sse4_2() (ctxt->cpuid->basic.sse4_2)
+#define vcpu_has_movbe() (ctxt->cpuid->basic.movbe)
+#define vcpu_has_popcnt() (ctxt->cpuid->basic.popcnt)
+#define vcpu_has_aesni() (ctxt->cpuid->basic.aesni)
+#define vcpu_has_avx() (ctxt->cpuid->basic.avx)
+#define vcpu_has_f16c() (ctxt->cpuid->basic.f16c)
+#define vcpu_has_rdrand() (ctxt->cpuid->basic.rdrand)
+
+#define vcpu_has_mmxext() (ctxt->cpuid->extd.mmxext || \
vcpu_has_sse())
-#define vcpu_has_3dnow_ext() vcpu_has(0x80000001, EDX, 30, ctxt, ops)
-#define vcpu_has_3dnow() vcpu_has(0x80000001, EDX, 31, ctxt, ops)
-#define vcpu_has_lahf_lm() vcpu_has(0x80000001, ECX, 0, ctxt, ops)
-#define vcpu_has_cr8_legacy() vcpu_has(0x80000001, ECX, 4, ctxt, ops)
-#define vcpu_has_lzcnt() vcpu_has(0x80000001, ECX, 5, ctxt, ops)
-#define vcpu_has_sse4a() vcpu_has(0x80000001, ECX, 6, ctxt, ops)
-#define vcpu_has_misalignsse() vcpu_has(0x80000001, ECX, 7, ctxt, ops)
-#define vcpu_has_xop() vcpu_has(0x80000001, ECX, 12, ctxt, ops)
-#define vcpu_has_fma4() vcpu_has(0x80000001, ECX, 16, ctxt, ops)
-#define vcpu_has_tbm() vcpu_has(0x80000001, ECX, 21, ctxt, ops)
-#define vcpu_has_bmi1() vcpu_has( 7, EBX, 3, ctxt, ops)
-#define vcpu_has_hle() vcpu_has( 7, EBX, 4, ctxt, ops)
-#define vcpu_has_avx2() vcpu_has( 7, EBX, 5, ctxt, ops)
-#define vcpu_has_bmi2() vcpu_has( 7, EBX, 8, ctxt, ops)
-#define vcpu_has_rtm() vcpu_has( 7, EBX, 11, ctxt, ops)
-#define vcpu_has_mpx() vcpu_has( 7, EBX, 14, ctxt, ops)
-#define vcpu_has_avx512f() vcpu_has( 7, EBX, 16, ctxt, ops)
-#define vcpu_has_avx512dq() vcpu_has( 7, EBX, 17, ctxt, ops)
-#define vcpu_has_rdseed() vcpu_has( 7, EBX, 18, ctxt, ops)
-#define vcpu_has_adx() vcpu_has( 7, EBX, 19, ctxt, ops)
-#define vcpu_has_smap() vcpu_has( 7, EBX, 20, ctxt, ops)
-#define vcpu_has_clflushopt() vcpu_has( 7, EBX, 23, ctxt, ops)
-#define vcpu_has_clwb() vcpu_has( 7, EBX, 24, ctxt, ops)
-#define vcpu_has_sha() vcpu_has( 7, EBX, 29, ctxt, ops)
-#define vcpu_has_avx512bw() vcpu_has( 7, EBX, 30, ctxt, ops)
-#define vcpu_has_rdpid() vcpu_has( 7, ECX, 22, ctxt, ops)
-#define vcpu_has_clzero() vcpu_has(0x80000008, EBX, 0, ctxt, ops)
+#define vcpu_has_3dnow_ext() (ctxt->cpuid->extd._3dnowext)
+#define vcpu_has_3dnow() (ctxt->cpuid->extd._3dnow)
+#define vcpu_has_lahf_lm() (ctxt->cpuid->extd.lahf_lm)
+#define vcpu_has_cr8_legacy() (ctxt->cpuid->extd.cr8_legacy)
+#define vcpu_has_lzcnt() (ctxt->cpuid->extd.abm)
+#define vcpu_has_sse4a() (ctxt->cpuid->extd.sse4a)
+#define vcpu_has_misalignsse() (ctxt->cpuid->extd.misalignsse)
+#define vcpu_has_xop() (ctxt->cpuid->extd.xop)
+#define vcpu_has_fma4() (ctxt->cpuid->extd.fma4)
+#define vcpu_has_tbm() (ctxt->cpuid->extd.tbm)
+#define vcpu_has_clzero() (ctxt->cpuid->extd.clzero)
+
+#define vcpu_has_bmi1() (ctxt->cpuid->feat.bmi1)
+#define vcpu_has_hle() (ctxt->cpuid->feat.hle)
+#define vcpu_has_avx2() (ctxt->cpuid->feat.avx2)
+#define vcpu_has_bmi2() (ctxt->cpuid->feat.bmi2)
+#define vcpu_has_rtm() (ctxt->cpuid->feat.rtm)
+#define vcpu_has_mpx() (ctxt->cpuid->feat.mpx)
+#define vcpu_has_avx512f() (ctxt->cpuid->feat.avx512f)
+#define vcpu_has_avx512dq() (ctxt->cpuid->feat.avx512dq)
+#define vcpu_has_rdseed() (ctxt->cpuid->feat.rdseed)
+#define vcpu_has_adx() (ctxt->cpuid->feat.adx)
+#define vcpu_has_smap() (ctxt->cpuid->feat.smap)
+#define vcpu_has_clflushopt() (ctxt->cpuid->feat.clflushopt)
+#define vcpu_has_clwb() (ctxt->cpuid->feat.clwb)
+#define vcpu_has_sha() (ctxt->cpuid->feat.sha)
+#define vcpu_has_avx512bw() (ctxt->cpuid->feat.avx512bw)
+#define vcpu_has_rdpid() (ctxt->cpuid->feat.rdpid)
#define vcpu_must_have(feat) \
generate_exception_if(!vcpu_has_##feat(), EXC_UD)
@@ -5268,10 +5235,7 @@ x86_emulate(
base = ad_bytes == 8 ? _regs.r(ax) :
ad_bytes == 4 ? _regs.eax : _regs.ax;
- limit = 0;
- if ( vcpu_has_clflush() &&
- ops->cpuid(1, 0, &cpuid_leaf, ctxt) == X86EMUL_OKAY )
- limit = ((cpuid_leaf.b >> 8) & 0xff) * 8;
+ limit = ctxt->cpuid->basic.clflush_size * 8;
generate_exception_if(limit < sizeof(long) ||
(limit & (limit - 1)), EXC_UD);
base &= ~(limit - 1);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-05 11:21 [PATCH 0/6] x86/emul: Use struct cpuid_policy for feature checks Andrew Cooper
` (4 preceding siblings ...)
2018-11-05 11:21 ` [PATCH 5/6] x86/emul: Don't use the ->cpuid() hook for feature checks Andrew Cooper
@ 2018-11-05 11:21 ` Andrew Cooper
2018-11-06 15:38 ` Jan Beulich
5 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-05 11:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich
They are identical, so provide a single x86emul_cpuid() instead.
As x86_emulate() now only uses the ->cpuid() hook for real CPUID instructions,
the hook can be omitted from all special-purpose emulation ops.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
---
xen/arch/x86/hvm/emulate.c | 13 ++-----------
xen/arch/x86/mm/shadow/hvm.c | 1 -
xen/arch/x86/pv/emul-priv-op.c | 10 +---------
xen/arch/x86/pv/ro-page-fault.c | 3 ---
xen/arch/x86/x86_emulate.c | 8 ++++++++
xen/arch/x86/x86_emulate/x86_emulate.h | 2 ++
xen/include/asm-x86/hvm/emulate.h | 2 --
xen/include/asm-x86/mm.h | 2 --
8 files changed, 13 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/hvm/emulate.c b/xen/arch/x86/hvm/emulate.c
index 91fa9db..3c42381 100644
--- a/xen/arch/x86/hvm/emulate.c
+++ b/xen/arch/x86/hvm/emulate.c
@@ -2107,13 +2107,6 @@ static int hvmemul_wbinvd(
return X86EMUL_OKAY;
}
-int hvmemul_cpuid(uint32_t leaf, uint32_t subleaf,
- struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt)
-{
- guest_cpuid(current, leaf, subleaf, res);
- return X86EMUL_OKAY;
-}
-
static int hvmemul_get_fpu(
enum x86_emulate_fpu_type type,
struct x86_emulate_ctxt *ctxt)
@@ -2312,7 +2305,7 @@ static const struct x86_emulate_ops hvm_emulate_ops = {
.read_msr = hvmemul_read_msr,
.write_msr = hvmemul_write_msr,
.wbinvd = hvmemul_wbinvd,
- .cpuid = hvmemul_cpuid,
+ .cpuid = x86emul_cpuid,
.get_fpu = hvmemul_get_fpu,
.put_fpu = hvmemul_put_fpu,
.invlpg = hvmemul_invlpg,
@@ -2339,7 +2332,7 @@ static const struct x86_emulate_ops hvm_emulate_ops_no_write = {
.read_msr = hvmemul_read_msr,
.write_msr = hvmemul_write_msr_discard,
.wbinvd = hvmemul_wbinvd_discard,
- .cpuid = hvmemul_cpuid,
+ .cpuid = x86emul_cpuid,
.get_fpu = hvmemul_get_fpu,
.put_fpu = hvmemul_put_fpu,
.invlpg = hvmemul_invlpg,
@@ -2424,13 +2417,11 @@ int hvm_emulate_one_mmio(unsigned long mfn, unsigned long gla)
.read = x86emul_unhandleable_rw,
.insn_fetch = hvmemul_insn_fetch,
.write = mmcfg_intercept_write,
- .cpuid = hvmemul_cpuid,
};
static const struct x86_emulate_ops hvm_ro_emulate_ops_mmio = {
.read = x86emul_unhandleable_rw,
.insn_fetch = hvmemul_insn_fetch,
.write = mmio_ro_emulated_write,
- .cpuid = hvmemul_cpuid,
};
struct mmio_ro_emulate_ctxt mmio_ro_ctxt = { .cr2 = gla };
struct hvm_emulate_ctxt ctxt;
diff --git a/xen/arch/x86/mm/shadow/hvm.c b/xen/arch/x86/mm/shadow/hvm.c
index 4cc7591..5087cd9 100644
--- a/xen/arch/x86/mm/shadow/hvm.c
+++ b/xen/arch/x86/mm/shadow/hvm.c
@@ -297,7 +297,6 @@ const struct x86_emulate_ops hvm_shadow_emulator_ops = {
.insn_fetch = hvm_emulate_insn_fetch,
.write = hvm_emulate_write,
.cmpxchg = hvm_emulate_cmpxchg,
- .cpuid = hvmemul_cpuid,
};
/**************************************************************************/
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 5968f99..d5a4046 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -1178,14 +1178,6 @@ static int _wbinvd(struct x86_emulate_ctxt *ctxt)
return X86EMUL_OKAY;
}
-int pv_emul_cpuid(uint32_t leaf, uint32_t subleaf,
- struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt)
-{
- guest_cpuid(current, leaf, subleaf, res);
-
- return X86EMUL_OKAY;
-}
-
static int validate(const struct x86_emulate_state *state,
struct x86_emulate_ctxt *ctxt)
{
@@ -1289,7 +1281,7 @@ static const struct x86_emulate_ops priv_op_ops = {
.write_xcr = x86emul_write_xcr,
.read_msr = read_msr,
.write_msr = write_msr,
- .cpuid = pv_emul_cpuid,
+ .cpuid = x86emul_cpuid,
.wbinvd = _wbinvd,
};
diff --git a/xen/arch/x86/pv/ro-page-fault.c b/xen/arch/x86/pv/ro-page-fault.c
index 9d4913d..fa358a6 100644
--- a/xen/arch/x86/pv/ro-page-fault.c
+++ b/xen/arch/x86/pv/ro-page-fault.c
@@ -259,7 +259,6 @@ static const struct x86_emulate_ops ptwr_emulate_ops = {
.write = ptwr_emulated_write,
.cmpxchg = ptwr_emulated_cmpxchg,
.validate = pv_emul_is_mem_write,
- .cpuid = pv_emul_cpuid,
};
/* Write page fault handler: check if guest is trying to modify a PTE. */
@@ -308,7 +307,6 @@ static const struct x86_emulate_ops mmio_ro_emulate_ops = {
.insn_fetch = ptwr_emulated_read,
.write = mmio_ro_emulated_write,
.validate = pv_emul_is_mem_write,
- .cpuid = pv_emul_cpuid,
};
static const struct x86_emulate_ops mmcfg_intercept_ops = {
@@ -316,7 +314,6 @@ static const struct x86_emulate_ops mmcfg_intercept_ops = {
.insn_fetch = ptwr_emulated_read,
.write = mmcfg_intercept_write,
.validate = pv_emul_is_mem_write,
- .cpuid = pv_emul_cpuid,
};
/* Check if guest is trying to modify a r/o MMIO page. */
diff --git a/xen/arch/x86/x86_emulate.c b/xen/arch/x86/x86_emulate.c
index 886bd87..60f73d0 100644
--- a/xen/arch/x86/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate.c
@@ -156,6 +156,14 @@ int x86emul_write_dr(unsigned int reg, unsigned long val,
}
}
+int x86emul_cpuid(uint32_t leaf, uint32_t subleaf,
+ struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt)
+{
+ guest_cpuid(current, leaf, subleaf, res);
+
+ return X86EMUL_OKAY;
+}
+
/*
* Local variables:
* mode: C
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.h b/xen/arch/x86/x86_emulate/x86_emulate.h
index 0397c1d..73201b9 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.h
+++ b/xen/arch/x86/x86_emulate/x86_emulate.h
@@ -724,6 +724,8 @@ int x86emul_read_dr(unsigned int reg, unsigned long *val,
struct x86_emulate_ctxt *ctxt);
int x86emul_write_dr(unsigned int reg, unsigned long val,
struct x86_emulate_ctxt *ctxt);
+int x86emul_cpuid(uint32_t leaf, uint32_t subleaf,
+ struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
#endif
diff --git a/xen/include/asm-x86/hvm/emulate.h b/xen/include/asm-x86/hvm/emulate.h
index 26a01e8..b39a1a0 100644
--- a/xen/include/asm-x86/hvm/emulate.h
+++ b/xen/include/asm-x86/hvm/emulate.h
@@ -76,8 +76,6 @@ void hvm_emulate_init_per_insn(
unsigned int insn_bytes);
void hvm_emulate_writeback(
struct hvm_emulate_ctxt *hvmemul_ctxt);
-int hvmemul_cpuid(uint32_t leaf, uint32_t subleaf,
- struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
struct segment_register *hvmemul_get_seg_reg(
enum x86_segment seg,
struct hvm_emulate_ctxt *hvmemul_ctxt);
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 6e45651..cda73e2 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -528,8 +528,6 @@ extern int mmcfg_intercept_write(enum x86_segment seg,
void *p_data,
unsigned int bytes,
struct x86_emulate_ctxt *ctxt);
-int pv_emul_cpuid(uint32_t leaf, uint32_t subleaf,
- struct cpuid_leaf *res, struct x86_emulate_ctxt *ctxt);
int audit_adjust_pgtables(struct domain *d, int dir, int noisy);
--
2.1.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-05 11:21 ` [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid() Andrew Cooper
@ 2018-11-06 15:38 ` Jan Beulich
2018-11-06 15:52 ` Andrew Cooper
0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2018-11-06 15:38 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
> They are identical, so provide a single x86emul_cpuid() instead.
>
> As x86_emulate() now only uses the ->cpuid() hook for real CPUID instructions,
> the hook can be omitted from all special-purpose emulation ops.
So I was expecting the hook to go away altogether, but I
now realize that it can't because of some of the customization
that's needed. That, in turn, means that the removal of the
hook specification as per above will get us into problems the
moment we need to check a feature that can't be taken
straight from the policy object. I'm therefore unconvinced we
actually want to go this far. It'll require enough care already
to not blindly clone a new vcpu_has_...() wrongly from the
many pre-existing examples in such a case. Thoughts?
Folding the two identical implementations, otoh, is fine with me.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-06 15:38 ` Jan Beulich
@ 2018-11-06 15:52 ` Andrew Cooper
2018-11-06 16:16 ` Jan Beulich
0 siblings, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-06 15:52 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 06/11/18 15:38, Jan Beulich wrote:
>>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
>> They are identical, so provide a single x86emul_cpuid() instead.
>>
>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID instructions,
>> the hook can be omitted from all special-purpose emulation ops.
> So I was expecting the hook to go away altogether, but I
> now realize that it can't because of some of the customization
> that's needed. That, in turn, means that the removal of the
> hook specification as per above will get us into problems the
> moment we need to check a feature that can't be taken
> straight from the policy object. I'm therefore unconvinced we
> actually want to go this far. It'll require enough care already
> to not blindly clone a new vcpu_has_...() wrongly from the
> many pre-existing examples in such a case. Thoughts?
All dynamic bits in CPUID are derived from other control state. e.g. we
check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC,
which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4.
In the emulator itself, I think it would be a bug if we ever had
vcpu_has_osxsave etc, because that isn't how pipelines actually behave.
The feature checks here are semantically equivalent to "do the
instruction decode and execution units have silicon to cope with these
instructions".
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-06 15:52 ` Andrew Cooper
@ 2018-11-06 16:16 ` Jan Beulich
2018-11-09 17:16 ` Andrew Cooper
0 siblings, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2018-11-06 16:16 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 06.11.18 at 16:52, <andrew.cooper3@citrix.com> wrote:
> On 06/11/18 15:38, Jan Beulich wrote:
>>>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
>>> They are identical, so provide a single x86emul_cpuid() instead.
>>>
>>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID
> instructions,
>>> the hook can be omitted from all special-purpose emulation ops.
>> So I was expecting the hook to go away altogether, but I
>> now realize that it can't because of some of the customization
>> that's needed. That, in turn, means that the removal of the
>> hook specification as per above will get us into problems the
>> moment we need to check a feature that can't be taken
>> straight from the policy object. I'm therefore unconvinced we
>> actually want to go this far. It'll require enough care already
>> to not blindly clone a new vcpu_has_...() wrongly from the
>> many pre-existing examples in such a case. Thoughts?
>
> All dynamic bits in CPUID are derived from other control state. e.g. we
> check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC,
> which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4.
>
> In the emulator itself, I think it would be a bug if we ever had
> vcpu_has_osxsave etc, because that isn't how pipelines actually behave.
> The feature checks here are semantically equivalent to "do the
> instruction decode and execution units have silicon to cope with these
> instructions".
I agree that vcpu_has_os* makes no sense, but the APIC bit for
example isn't really pipeline / decoder related. However, one
issue already might be that in order for bits in a (sub)leaf above
(guest) limits to come out all clear, it is guest_cpuid() which cuts
things off. Neither cpuid_featureset_to_policy() nor its inverse
nor sanitise_featureset() look to zap fields above limits, in case
they've been previously set to non-zero values. Am I overlooking
something?
Furthermore I wouldn't exclude that we may need to look at a
hypervisor or Viridian leaf at some point. The dynamic vPMU
adjustments also look potentially problematic, if we were to
emulate RDPMC (albeit they're DS-related only right now). And
then there's the dynamic exposing of MONITOR for PV; granted
I don't expect us to ever emulate this for PV, but it shows the
general issue. Plus there's SYSCALL, the insn emulation of which
currently doesn't check the (dynamically adjusted) CPUID bit.
And finally LWP, which again we can only hope we'll never have
to emulate.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-06 16:16 ` Jan Beulich
@ 2018-11-09 17:16 ` Andrew Cooper
2018-11-12 8:17 ` Jan Beulich
2018-11-15 14:23 ` Andrew Cooper
0 siblings, 2 replies; 25+ messages in thread
From: Andrew Cooper @ 2018-11-09 17:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 06/11/18 16:16, Jan Beulich wrote:
>>>> On 06.11.18 at 16:52, <andrew.cooper3@citrix.com> wrote:
>> On 06/11/18 15:38, Jan Beulich wrote:
>>>>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
>>>> They are identical, so provide a single x86emul_cpuid() instead.
>>>>
>>>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID
>> instructions,
>>>> the hook can be omitted from all special-purpose emulation ops.
>>> So I was expecting the hook to go away altogether, but I
>>> now realize that it can't because of some of the customization
>>> that's needed. That, in turn, means that the removal of the
>>> hook specification as per above will get us into problems the
>>> moment we need to check a feature that can't be taken
>>> straight from the policy object. I'm therefore unconvinced we
>>> actually want to go this far. It'll require enough care already
>>> to not blindly clone a new vcpu_has_...() wrongly from the
>>> many pre-existing examples in such a case. Thoughts?
>> All dynamic bits in CPUID are derived from other control state. e.g. we
>> check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC,
>> which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4.
>>
>> In the emulator itself, I think it would be a bug if we ever had
>> vcpu_has_osxsave etc, because that isn't how pipelines actually behave.
>> The feature checks here are semantically equivalent to "do the
>> instruction decode and execution units have silicon to cope with these
>> instructions".
> I agree that vcpu_has_os* makes no sense, but the APIC bit for
> example isn't really pipeline / decoder related.
Correct, so why do you think APIC matters? All interaction with the
APIC is via its MMIO/MSR interface, rather than via dedicated instructions.
> However, one
> issue already might be that in order for bits in a (sub)leaf above
> (guest) limits to come out all clear, it is guest_cpuid() which cuts
> things off. Neither cpuid_featureset_to_policy() nor its inverse
> nor sanitise_featureset() look to zap fields above limits, in case
> they've been previously set to non-zero values. Am I overlooking
> something?
No - that is an aspect I'd overlooked, because the
DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet.
I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf
settings, because the intention was always that a flat cpuid_policy was
safe to use in this manner. I think there is an existing corner case
when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x8000007.
> Furthermore I wouldn't exclude that we may need to look at a
> hypervisor or Viridian leaf at some point. The dynamic vPMU
> adjustments also look potentially problematic, if we were to
> emulate RDPMC (albeit they're DS-related only right now).
The only reason vPMU is dynamic is because we don't (yet) have a split
between default and max policies. Fixing this is on the todo list,
albeit behind a fairly long chain of dependencies.
> And then there's the dynamic exposing of MONITOR for PV; granted
> I don't expect us to ever emulate this for PV, but it shows the
> general issue.
That is to work around a deficiency in how Linux behaves. It isn't for
allowing PV guests to use MONITOR.
> Plus there's SYSCALL, the insn emulation of which
> currently doesn't check the (dynamically adjusted) CPUID bit.
No, nor should it. Intel's objection to the SYSCALL/SYSRET instructions
is mode based. (As a demonstration which proves the point of this
patch, if you hide the SYSCALL bit using masking, the instruction still
operates fine).
It seems I never got around to submitting my XSA-204 followup patch
which fixes many emulation bugs with SYS{CALL,RET,ENTER,EXIT}.
> And finally LWP, which again we can only hope we'll never have
> to emulate.
LWP doesn't exist any more, even on the hardware it used to exist on.
It was never implemented on Fam17h, and was removed from Fam15/16h in a
microcode update to make room to implement IBPB for Spectre v2 mitigations.
I recommend we purge the support completely.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-09 17:16 ` Andrew Cooper
@ 2018-11-12 8:17 ` Jan Beulich
2018-11-14 0:20 ` Woods, Brian
2018-11-15 14:23 ` Andrew Cooper
1 sibling, 1 reply; 25+ messages in thread
From: Jan Beulich @ 2018-11-12 8:17 UTC (permalink / raw)
To: Brian Woods, Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 09.11.18 at 18:16, <andrew.cooper3@citrix.com> wrote:
> On 06/11/18 16:16, Jan Beulich wrote:
>>>>> On 06.11.18 at 16:52, <andrew.cooper3@citrix.com> wrote:
>>> On 06/11/18 15:38, Jan Beulich wrote:
>>>>>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
>>>>> They are identical, so provide a single x86emul_cpuid() instead.
>>>>>
>>>>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID
>>> instructions,
>>>>> the hook can be omitted from all special-purpose emulation ops.
>>>> So I was expecting the hook to go away altogether, but I
>>>> now realize that it can't because of some of the customization
>>>> that's needed. That, in turn, means that the removal of the
>>>> hook specification as per above will get us into problems the
>>>> moment we need to check a feature that can't be taken
>>>> straight from the policy object. I'm therefore unconvinced we
>>>> actually want to go this far. It'll require enough care already
>>>> to not blindly clone a new vcpu_has_...() wrongly from the
>>>> many pre-existing examples in such a case. Thoughts?
>>> All dynamic bits in CPUID are derived from other control state. e.g. we
>>> check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC,
>>> which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4.
>>>
>>> In the emulator itself, I think it would be a bug if we ever had
>>> vcpu_has_osxsave etc, because that isn't how pipelines actually behave.
>>> The feature checks here are semantically equivalent to "do the
>>> instruction decode and execution units have silicon to cope with these
>>> instructions".
>> I agree that vcpu_has_os* makes no sense, but the APIC bit for
>> example isn't really pipeline / decoder related.
>
> Correct, so why do you think APIC matters? All interaction with the
> APIC is via its MMIO/MSR interface, rather than via dedicated instructions.
It was a general example, not something that specifically matters here.
>> And finally LWP, which again we can only hope we'll never have
>> to emulate.
>
> LWP doesn't exist any more, even on the hardware it used to exist on.
> It was never implemented on Fam17h, and was removed from Fam15/16h in a
> microcode update to make room to implement IBPB for Spectre v2 mitigations.
>
> I recommend we purge the support completely.
I certainly don't mind; I'd prefer though if such a withdrawal of
functionality came actually from AMD. Brian?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-12 8:17 ` Jan Beulich
@ 2018-11-14 0:20 ` Woods, Brian
2018-11-14 7:47 ` Jan Beulich
0 siblings, 1 reply; 25+ messages in thread
From: Woods, Brian @ 2018-11-14 0:20 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Woods, Brian, Wei Liu, Xen-devel
On Mon, Nov 12, 2018 at 01:17:59AM -0700, Jan Beulich wrote:
> >>> On 09.11.18 at 18:16, <andrew.cooper3@citrix.com> wrote:
> > On 06/11/18 16:16, Jan Beulich wrote:
> >>>>> On 06.11.18 at 16:52, <andrew.cooper3@citrix.com> wrote:
> >>> On 06/11/18 15:38, Jan Beulich wrote:
> >>>>>>> On 05.11.18 at 12:21, <andrew.cooper3@citrix.com> wrote:
> >>>>> They are identical, so provide a single x86emul_cpuid() instead.
> >>>>>
> >>>>> As x86_emulate() now only uses the ->cpuid() hook for real CPUID
> >>> instructions,
> >>>>> the hook can be omitted from all special-purpose emulation ops.
> >>>> So I was expecting the hook to go away altogether, but I
> >>>> now realize that it can't because of some of the customization
> >>>> that's needed. That, in turn, means that the removal of the
> >>>> hook specification as per above will get us into problems the
> >>>> moment we need to check a feature that can't be taken
> >>>> straight from the policy object. I'm therefore unconvinced we
> >>>> actually want to go this far. It'll require enough care already
> >>>> to not blindly clone a new vcpu_has_...() wrongly from the
> >>>> many pre-existing examples in such a case. Thoughts?
> >>> All dynamic bits in CPUID are derived from other control state. e.g. we
> >>> check CR4.OSXSAVE, not CPUID.OSXSAVE. The other dynamic bits are APIC,
> >>> which comes from MSR_APIC_BASE, and OSPKE which also comes from CR4.
> >>>
> >>> In the emulator itself, I think it would be a bug if we ever had
> >>> vcpu_has_osxsave etc, because that isn't how pipelines actually behave.
> >>> The feature checks here are semantically equivalent to "do the
> >>> instruction decode and execution units have silicon to cope with these
> >>> instructions".
> >> I agree that vcpu_has_os* makes no sense, but the APIC bit for
> >> example isn't really pipeline / decoder related.
> >
> > Correct, so why do you think APIC matters? All interaction with the
> > APIC is via its MMIO/MSR interface, rather than via dedicated instructions.
>
> It was a general example, not something that specifically matters here.
>
> >> And finally LWP, which again we can only hope we'll never have
> >> to emulate.
> >
> > LWP doesn't exist any more, even on the hardware it used to exist on.
> > It was never implemented on Fam17h, and was removed from Fam15/16h in a
> > microcode update to make room to implement IBPB for Spectre v2 mitigations.
> >
> > I recommend we purge the support completely.
>
> I certainly don't mind; I'd prefer though if such a withdrawal of
> functionality came actually from AMD. Brian?
>
> Jan
LWP support isn't enabled on F15h system with the latest Ucode and it
isn't available on F17h. I don't see any reason to keep it if it's
hampering cleanup and reformatting.
--
Brian Woods
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-14 0:20 ` Woods, Brian
@ 2018-11-14 7:47 ` Jan Beulich
0 siblings, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2018-11-14 7:47 UTC (permalink / raw)
To: Brian Woods; +Cc: Andrew Cooper, Wei Liu, Xen-devel
>>> On 14.11.18 at 01:20, <Brian.Woods@amd.com> wrote:
> On Mon, Nov 12, 2018 at 01:17:59AM -0700, Jan Beulich wrote:
>> >>> On 09.11.18 at 18:16, <andrew.cooper3@citrix.com> wrote:
>> > LWP doesn't exist any more, even on the hardware it used to exist on.
>> > It was never implemented on Fam17h, and was removed from Fam15/16h in a
>> > microcode update to make room to implement IBPB for Spectre v2 mitigations.
>> >
>> > I recommend we purge the support completely.
>>
>> I certainly don't mind; I'd prefer though if such a withdrawal of
>> functionality came actually from AMD. Brian?
>
> LWP support isn't enabled on F15h system with the latest Ucode and it
> isn't available on F17h. I don't see any reason to keep it if it's
> hampering cleanup and reformatting.
Okay, this confirms your agreement with the removal plans, but is
there any chance the patch removing it could actually come from
an AMD person, as suggested above?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-09 17:16 ` Andrew Cooper
2018-11-12 8:17 ` Jan Beulich
@ 2018-11-15 14:23 ` Andrew Cooper
2018-11-15 15:09 ` Jan Beulich
1 sibling, 1 reply; 25+ messages in thread
From: Andrew Cooper @ 2018-11-15 14:23 UTC (permalink / raw)
To: Jan Beulich; +Cc: Wei Liu, Xen-devel
On 09/11/2018 17:16, Andrew Cooper wrote:
>
>> However, one
>> issue already might be that in order for bits in a (sub)leaf above
>> (guest) limits to come out all clear, it is guest_cpuid() which cuts
>> things off. Neither cpuid_featureset_to_policy() nor its inverse
>> nor sanitise_featureset() look to zap fields above limits, in case
>> they've been previously set to non-zero values. Am I overlooking
>> something?
> No - that is an aspect I'd overlooked, because the
> DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet.
>
> I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf
> settings, because the intention was always that a flat cpuid_policy was
> safe to use in this manner. I think there is an existing corner case
> when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x8000007.
Actually, I remember now that I tried fixing this once before. It's not
possible without DOMCTL_set_cpu_policy because the current API with
DOMCTL_set_cpuid provides leaves piecewise and there is no point at
which Xen can safely zero the out-of-range leaves without loosing
information later if max_leaf is increased.
The content of cpuid_policy will be safe for the emulator to use,
because it will be bounded by real hardware support. As levelling like
this is an extreme corner case which doesn't work at the moment for
other reasons, I'd like to take this series now and fix up the behaviour
later, to reduce the dependencies in the remaining CPUID/MSR work - its
already complicated enough.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 6/6] x86/emul: dedup hvmemul_cpuid() and pv_emul_cpuid()
2018-11-15 14:23 ` Andrew Cooper
@ 2018-11-15 15:09 ` Jan Beulich
0 siblings, 0 replies; 25+ messages in thread
From: Jan Beulich @ 2018-11-15 15:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Wei Liu, Xen-devel
>>> On 15.11.18 at 15:23, <andrew.cooper3@citrix.com> wrote:
> On 09/11/2018 17:16, Andrew Cooper wrote:
>>
>>> However, one
>>> issue already might be that in order for bits in a (sub)leaf above
>>> (guest) limits to come out all clear, it is guest_cpuid() which cuts
>>> things off. Neither cpuid_featureset_to_policy() nor its inverse
>>> nor sanitise_featureset() look to zap fields above limits, in case
>>> they've been previously set to non-zero values. Am I overlooking
>>> something?
>> No - that is an aspect I'd overlooked, because the
>> DOMCTL_set_cpuid_policy work (which does this correctly) hasn't gone in yet.
>>
>> I think I'll tweak recalculate_misc() to zero out beyond the max_subleaf
>> settings, because the intention was always that a flat cpuid_policy was
>> safe to use in this manner. I think there is an existing corner case
>> when trying to level basic.max_leaf to < 7, or extd.max_leaf to < 0x8000007.
>
> Actually, I remember now that I tried fixing this once before. It's not
> possible without DOMCTL_set_cpu_policy because the current API with
> DOMCTL_set_cpuid provides leaves piecewise and there is no point at
> which Xen can safely zero the out-of-range leaves without loosing
> information later if max_leaf is increased.
There could be such a point if an arch call was added at the point
where d->creation_finished gets set the first time.
Looking at the domctl side flow I'm wondering what the domain
pausing is good for there, now that we don't allow changes once
d->creation_finished is set. Was not dropping this an oversight
of 3d0cab7b5d?
> The content of cpuid_policy will be safe for the emulator to use,
> because it will be bounded by real hardware support.
Except that it voids all respective uses of vcpu_has_*() - just
cpu_has_* would then be sufficient. And overall behavior is
inconsistent between bit-wise leveling and full leaf hiding.
> As levelling like
> this is an extreme corner case which doesn't work at the moment for
> other reasons, I'd like to take this series now and fix up the behaviour
> later, to reduce the dependencies in the remaining CPUID/MSR work - its
> already complicated enough.
Hmm, okay, as long as a respective remark gets added at least
to the commit message, I could live with that.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 25+ messages in thread