* [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set()
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:51 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted Andrew Cooper
` (9 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
In order to combine the functionality of xc_cpuid_set() with
xc_cpuid_apply_policy(), arrange to pass the data in a single contained
struct, rather than two arrays.
libxl__cpuid_policy is the ideal structure to use, but that would introduce a
reverse dependency between libxc and libxl. Introduce xc_xend_cpuid (with a
transparent union to provide more useful names for the inputs), and use this
structure in libxl.
The public API has libxl_cpuid_policy as an opaque type referencing
libxl__cpuid_policy. Drop the inappropriate comment about its internals, and
use xc_xend_cpuid as a differently named opaque backing object. Users of both
libxl and libxc are not permitted to look at the internals.
No change in behaviour.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
---
tools/libxc/include/xenctrl.h | 30 ++++++++++++++++++++++++++++--
tools/libxc/xc_cpuid_x86.c | 39 +++++++++++----------------------------
tools/libxl/libxl.h | 8 ++++----
tools/libxl/libxl_cpuid.c | 7 +++----
tools/libxl/libxl_internal.h | 10 ----------
5 files changed, 46 insertions(+), 48 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 113ddd935d..178144e8e2 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1792,10 +1792,36 @@ int xc_domain_debug_control(xc_interface *xch,
uint32_t vcpu);
#if defined(__i386__) || defined(__x86_64__)
+
+/*
+ * CPUID policy data, expressed in the legacy XEND format.
+ *
+ * Policy is an array of strings, 32 chars long:
+ * policy[0] = eax
+ * policy[1] = ebx
+ * policy[2] = ecx
+ * policy[3] = edx
+ *
+ * The format of the string is the following:
+ * '1' -> force to 1
+ * '0' -> force to 0
+ * 'x' -> we don't care (use default)
+ * 'k' -> pass through host value
+ * 's' -> legacy alias for 'k'
+ */
+struct xc_xend_cpuid {
+ union {
+ struct {
+ uint32_t leaf, subleaf;
+ };
+ uint32_t input[2];
+ };
+ char *policy[4];
+};
+
int xc_cpuid_set(xc_interface *xch,
uint32_t domid,
- const unsigned int *input,
- const char **config);
+ const struct xc_xend_cpuid *xend);
/*
* Make adjustments to the CPUID settings for a domain.
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index b42edd6457..edc2ad9b47 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -259,27 +259,8 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
return ret;
}
-/*
- * Configure a single input with the informatiom from config.
- *
- * Config is an array of strings:
- * config[0] = eax
- * config[1] = ebx
- * config[2] = ecx
- * config[3] = edx
- *
- * The format of the string is the following:
- * '1' -> force to 1
- * '0' -> force to 0
- * 'x' -> we don't care (use default)
- * 'k' -> pass through host value
- * 's' -> legacy alias for 'k'
- *
- * In all cases, the returned string consists of just '0' and '1'.
- */
int xc_cpuid_set(
- xc_interface *xch, uint32_t domid, const unsigned int *input,
- const char **config)
+ xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
{
int rc;
unsigned int i, j, regs[4] = {}, polregs[4] = {};
@@ -324,7 +305,8 @@ int xc_cpuid_set(
goto fail;
}
for ( i = 0; i < policy_leaves; ++i )
- if ( leaves[i].leaf == input[0] && leaves[i].subleaf == input[1] )
+ if ( leaves[i].leaf == xend->leaf &&
+ leaves[i].subleaf == xend->subleaf )
{
polregs[0] = leaves[i].a;
polregs[1] = leaves[i].b;
@@ -345,7 +327,8 @@ int xc_cpuid_set(
goto fail;
}
for ( i = 0; i < policy_leaves; ++i )
- if ( leaves[i].leaf == input[0] && leaves[i].subleaf == input[1] )
+ if ( leaves[i].leaf == xend->leaf &&
+ leaves[i].subleaf == xend->subleaf )
{
regs[0] = leaves[i].a;
regs[1] = leaves[i].b;
@@ -356,7 +339,7 @@ int xc_cpuid_set(
for ( i = 0; i < 4; i++ )
{
- if ( config[i] == NULL )
+ if ( xend->policy[i] == NULL )
{
regs[i] = polregs[i];
continue;
@@ -375,14 +358,14 @@ int xc_cpuid_set(
unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
rc = -EINVAL;
- if ( !strchr("10xks", config[i][j]) )
+ if ( !strchr("10xks", xend->policy[i][j]) )
goto fail;
- if ( config[i][j] == '1' )
+ if ( xend->policy[i][j] == '1' )
val = 1;
- else if ( config[i][j] == '0' )
+ else if ( xend->policy[i][j] == '0' )
val = 0;
- else if ( config[i][j] == 'x' )
+ else if ( xend->policy[i][j] == 'x' )
val = polval;
if ( val )
@@ -393,7 +376,7 @@ int xc_cpuid_set(
}
/* Feed the transformed leaf back up to Xen. */
- leaves[0] = (xen_cpuid_leaf_t){ input[0], input[1],
+ leaves[0] = (xen_cpuid_leaf_t){ xend->leaf, xend->subleaf,
regs[0], regs[1], regs[2], regs[3] };
rc = xc_set_domain_cpu_policy(xch, domid, 1, leaves, 0, NULL,
&err_leaf, &err_subleaf, &err_msr);
diff --git a/tools/libxl/libxl.h b/tools/libxl/libxl.h
index 71709dc585..1cd6c38e83 100644
--- a/tools/libxl/libxl.h
+++ b/tools/libxl/libxl.h
@@ -1310,11 +1310,11 @@ typedef struct {
void libxl_bitmap_init(libxl_bitmap *map);
void libxl_bitmap_dispose(libxl_bitmap *map);
-/* libxl_cpuid_policy_list is a dynamic array storing CPUID policies
- * for multiple leafs. It is terminated with an entry holding
- * XEN_CPUID_INPUT_UNUSED in input[0]
+/*
+ * libxl_cpuid_policy is opaque in the libxl ABI. Users of both libxl and
+ * libxc may not make assumptions about xc_xend_cpuid.
*/
-typedef struct libxl__cpuid_policy libxl_cpuid_policy;
+typedef struct xc_xend_cpuid libxl_cpuid_policy;
typedef libxl_cpuid_policy * libxl_cpuid_policy_list;
void libxl_cpuid_dispose(libxl_cpuid_policy_list *cpuid_list);
int libxl_cpuid_policy_list_length(const libxl_cpuid_policy_list *l);
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 796ec4f2d9..e001cbcd4f 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -288,7 +288,7 @@ int libxl_cpuid_parse_config(libxl_cpuid_policy_list *cpuid, const char* str)
char *sep, *val, *endptr;
int i;
const struct cpuid_flags *flag;
- struct libxl__cpuid_policy *entry;
+ struct xc_xend_cpuid *entry;
unsigned long num;
char flags[33], *resstr;
@@ -366,7 +366,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
char *endptr;
unsigned long value;
uint32_t leaf, subleaf = XEN_CPUID_INPUT_UNUSED;
- struct libxl__cpuid_policy *entry;
+ struct xc_xend_cpuid *entry;
/* parse the leaf number */
value = strtoul(str, &endptr, 0);
@@ -442,8 +442,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
return;
for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++)
- xc_cpuid_set(ctx->xch, domid, cpuid[i].input,
- (const char**)cpuid[i].policy);
+ xc_cpuid_set(ctx->xch, domid, &cpuid[i]);
}
static const char *input_names[2] = { "leaf", "subleaf" };
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index c7ece066c4..79c2bf5f5e 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2056,16 +2056,6 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
_hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
libxl__gen_json_callback gen, void *p);
- /* holds the CPUID response for a single CPUID leaf
- * input contains the value of the EAX and ECX register,
- * and each policy string contains a filter to apply to
- * the host given values for that particular leaf.
- */
-struct libxl__cpuid_policy {
- uint32_t input[2];
- char *policy[4];
-};
-
_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
libxl_domain_build_info *info);
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set()
2020-06-15 14:15 ` [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set() Andrew Cooper
@ 2020-06-15 14:51 ` Ian Jackson
0 siblings, 0 replies; 40+ messages in thread
From: Ian Jackson @ 2020-06-15 14:51 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Paul Durrant, Jan Beulich, Wei Liu, Roger Pau Monne
Andrew Cooper writes ("[PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set()"):
> In order to combine the functionality of xc_cpuid_set() with
> xc_cpuid_apply_policy(), arrange to pass the data in a single contained
> struct, rather than two arrays.
>
> libxl__cpuid_policy is the ideal structure to use, but that would introduce a
> reverse dependency between libxc and libxl. Introduce xc_xend_cpuid (with a
> transparent union to provide more useful names for the inputs), and use this
> structure in libxl.
>
> The public API has libxl_cpuid_policy as an opaque type referencing
> libxl__cpuid_policy. Drop the inappropriate comment about its internals, and
> use xc_xend_cpuid as a differently named opaque backing object. Users of both
> libxl and libxc are not permitted to look at the internals.
>
> No change in behaviour.
>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
2020-06-15 14:15 ` [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set() Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:52 ` Ian Jackson
2020-06-16 9:01 ` Jan Beulich
2020-06-15 14:15 ` [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set() Andrew Cooper
` (8 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
about to start relying on this. Extend the unit tests.
As test_cpuid_serialise_success() is a fairly limited set of synthetic
examples right now, introduce test_cpuid_current() to operate on the full
policy for the current CPU.
Tweak the fail() macro to allow for simplified control flow.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
---
tools/tests/cpu-policy/test-cpu-policy.c | 49 +++++++++++++++++++++++++++++++-
1 file changed, 48 insertions(+), 1 deletion(-)
diff --git a/tools/tests/cpu-policy/test-cpu-policy.c b/tools/tests/cpu-policy/test-cpu-policy.c
index fe8cdf6ea9..7ba9707236 100644
--- a/tools/tests/cpu-policy/test-cpu-policy.c
+++ b/tools/tests/cpu-policy/test-cpu-policy.c
@@ -16,7 +16,7 @@ static unsigned int nr_failures;
#define fail(fmt, ...) \
({ \
nr_failures++; \
- printf(fmt, ##__VA_ARGS__); \
+ (void)printf(fmt, ##__VA_ARGS__); \
})
#define memdup(ptr) \
@@ -66,6 +66,45 @@ static void test_vendor_identification(void)
}
}
+static bool leaves_are_sorted(const xen_cpuid_leaf_t *leaves, unsigned int nr)
+{
+ for ( unsigned int i = 1; i < nr; ++i )
+ {
+ /* leaf index went backwards => not sorted. */
+ if ( leaves[i - 1].leaf > leaves[i].leaf )
+ return false;
+
+ /* leaf index went forwards => ok */
+ if ( leaves[i - 1].leaf < leaves[i].leaf )
+ continue;
+
+ /* leave index the same, subleaf didn't increase => not sorted. */
+ if ( leaves[i - 1].subleaf >= leaves[i].subleaf )
+ return false;
+ }
+
+ return true;
+}
+
+static void test_cpuid_current(void)
+{
+ struct cpuid_policy p;
+ xen_cpuid_leaf_t leaves[CPUID_MAX_SERIALISED_LEAVES];
+ unsigned int nr = ARRAY_SIZE(leaves);
+ int rc;
+
+ printf("Testing CPUID on current CPU\n");
+
+ x86_cpuid_policy_fill_native(&p);
+
+ rc = x86_cpuid_copy_to_buffer(&p, leaves, &nr);
+ if ( rc != 0 )
+ return fail(" Serialise, expected rc 0, got %d\n", rc);
+
+ if ( !leaves_are_sorted(leaves, nr) )
+ return fail(" Leaves not sorted\n");
+}
+
static void test_cpuid_serialise_success(void)
{
static const struct test {
@@ -178,6 +217,13 @@ static void test_cpuid_serialise_success(void)
goto test_done;
}
+ if ( !leaves_are_sorted(leaves, nr) )
+ {
+ fail(" Test %s, leaves not sorted\n",
+ t->name);
+ goto test_done;
+ }
+
test_done:
free(leaves);
}
@@ -613,6 +659,7 @@ int main(int argc, char **argv)
test_vendor_identification();
+ test_cpuid_current();
test_cpuid_serialise_success();
test_cpuid_deserialise_failure();
test_cpuid_out_of_range_clearing();
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 14:15 ` [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted Andrew Cooper
@ 2020-06-15 14:52 ` Ian Jackson
2020-06-15 15:00 ` Andrew Cooper
2020-06-16 9:01 ` Jan Beulich
1 sibling, 1 reply; 40+ messages in thread
From: Ian Jackson @ 2020-06-15 14:52 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne
Andrew Cooper writes ("[PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
> about to start relying on this. Extend the unit tests.
>
> As test_cpuid_serialise_success() is a fairly limited set of synthetic
> examples right now, introduce test_cpuid_current() to operate on the full
> policy for the current CPU.
>
> Tweak the fail() macro to allow for simplified control flow.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
I don't think anything in our normal dev workflow runs this
automatically ? Maybe this would be something for us to think
about...
Ian.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 14:52 ` Ian Jackson
@ 2020-06-15 15:00 ` Andrew Cooper
2020-06-15 15:34 ` Ian Jackson
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 15:00 UTC (permalink / raw)
To: Ian Jackson
Cc: Xen-devel, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne
On 15/06/2020 15:52, Ian Jackson wrote:
> Andrew Cooper writes ("[PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
>> about to start relying on this. Extend the unit tests.
>>
>> As test_cpuid_serialise_success() is a fairly limited set of synthetic
>> examples right now, introduce test_cpuid_current() to operate on the full
>> policy for the current CPU.
>>
>> Tweak the fail() macro to allow for simplified control flow.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
>
> I don't think anything in our normal dev workflow runs this
> automatically ? Maybe this would be something for us to think
> about...
Nothing runs it by default, but it is part of my prepush testing for
anything in the relevant area.
We should do something better, but its not totally trivial. The x86
emulator test for example needs a fairly bleeding edge version of
binutils, given that we routinely add support for bleeding edge
instruction groups.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 15:00 ` Andrew Cooper
@ 2020-06-15 15:34 ` Ian Jackson
2020-06-15 16:12 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Ian Jackson @ 2020-06-15 15:34 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne
Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
> Nothing runs it by default, but it is part of my prepush testing for
> anything in the relevant area.
>
> We should do something better, but its not totally trivial. The x86
> emulator test for example needs a fairly bleeding edge version of
> binutils, given that we routinely add support for bleeding edge
> instruction groups.
Hmmm. Is it likely that installing the version from Debian testing
(say) would work ? Or do we want to build it from source ? These are
all possibilities.
We could build binutils from source with a job-specific --prefix
setting and that wouldn't stop it sharing the build host with other
jobs. Maybe it could be a seperate job which provides an anointed
binutils build.
What other awkward requirements are there ?
Ian.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 15:34 ` Ian Jackson
@ 2020-06-15 16:12 ` Andrew Cooper
2020-06-16 6:51 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 16:12 UTC (permalink / raw)
To: Ian Jackson
Cc: Xen-devel, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monne
On 15/06/2020 16:34, Ian Jackson wrote:
> Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>> Nothing runs it by default, but it is part of my prepush testing for
>> anything in the relevant area.
>>
>> We should do something better, but its not totally trivial. The x86
>> emulator test for example needs a fairly bleeding edge version of
>> binutils, given that we routinely add support for bleeding edge
>> instruction groups.
> Hmmm. Is it likely that installing the version from Debian testing
> (say) would work ? Or do we want to build it from source ? These are
> all possibilities.
Pulling from Sid may work, if they're fairly prompt to update to new
binutils releases. (They're certainly up to date ATM)
Jan: are we ever in a position where we need something more bleeding
edge than the latest binutils release?
>
> We could build binutils from source with a job-specific --prefix
> setting and that wouldn't stop it sharing the build host with other
> jobs. Maybe it could be a seperate job which provides an anointed
> binutils build.
>
> What other awkward requirements are there ?
Most of the tests require running under a suitably configured Xen, and
even then, require doing some fairly custom things with the guest.
Perhaps a good start would be to split our "unit test like" tests away
from our "need a specifically configured Xen" tests. The former would
be rather more amenable to running by default.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 16:12 ` Andrew Cooper
@ 2020-06-16 6:51 ` Jan Beulich
0 siblings, 0 replies; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 6:51 UTC (permalink / raw)
To: Andrew Cooper, Ian Jackson
Cc: Xen-devel, Paul Durrant, Wei Liu, Roger Pau Monne
On 15.06.2020 18:12, Andrew Cooper wrote:
> On 15/06/2020 16:34, Ian Jackson wrote:
>> Andrew Cooper writes ("Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted"):
>>> Nothing runs it by default, but it is part of my prepush testing for
>>> anything in the relevant area.
>>>
>>> We should do something better, but its not totally trivial. The x86
>>> emulator test for example needs a fairly bleeding edge version of
>>> binutils, given that we routinely add support for bleeding edge
>>> instruction groups.
>> Hmmm. Is it likely that installing the version from Debian testing
>> (say) would work ? Or do we want to build it from source ? These are
>> all possibilities.
>
> Pulling from Sid may work, if they're fairly prompt to update to new
> binutils releases. (They're certainly up to date ATM)
>
> Jan: are we ever in a position where we need something more bleeding
> edge than the latest binutils release?
Gcc needs to be about as recent: Right now the minimum is gcc 8 I
think, and I have a few hacks in place locally to make things
somewhat work back to gcc 5, as on my laptop (and hence when
working from home) I don't have any custom gcc builds.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted
2020-06-15 14:15 ` [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted Andrew Cooper
2020-06-15 14:52 ` Ian Jackson
@ 2020-06-16 9:01 ` Jan Beulich
1 sibling, 0 replies; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 9:01 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 15.06.2020 16:15, Andrew Cooper wrote:
> The existing x86_cpuid_copy_to_buffer() does produce sorted results, and we're
> about to start relying on this. Extend the unit tests.
>
> As test_cpuid_serialise_success() is a fairly limited set of synthetic
> examples right now, introduce test_cpuid_current() to operate on the full
> policy for the current CPU.
>
> Tweak the fail() macro to allow for simplified control flow.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
2020-06-15 14:15 ` [PATCH 1/9] tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set() Andrew Cooper
2020-06-15 14:15 ` [PATCH 2/9] tests/cpu-policy: Confirm that CPUID serialisation is sorted Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:54 ` Ian Jackson
2020-06-16 9:16 ` Jan Beulich
2020-06-15 14:15 ` [PATCH 4/9] tools/libx[cl]: Merge xc_cpuid_set() into xc_cpuid_apply_policy() Andrew Cooper
` (7 subsequent siblings)
10 siblings, 2 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
Currently, libxl__cpuid_legacy() passes each element of the policy list to
xc_cpuid_set() individually. This is wasteful both in terms of the number of
hypercalls made, and the quantity of repeated merging/auditing work performed
by Xen.
Move the loop processing down into xc_cpuid_set(), which allows us to do one
set of hypercalls, rather than one per list entry.
In xc_cpuid_set(), obtain the full host, guest max and current policies to
begin with, and loop over the xend array, processing one leaf at a time.
Replace the linear search with a binary search, seeing as the serialised
leaves are sorted.
No change in behaviour from the guests point of view.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
---
tools/libxc/xc_cpuid_x86.c | 159 +++++++++++++++++++++++++++------------------
tools/libxl/libxl_cpuid.c | 4 +-
2 files changed, 95 insertions(+), 68 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index edc2ad9b47..e827c48fd1 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -38,8 +38,6 @@ enum {
#define bitmaskof(idx) (1u << ((idx) & 31))
#define featureword_of(idx) ((idx) >> 5)
-#define clear_feature(idx, dst) ((dst) &= ~bitmaskof(idx))
-#define set_feature(idx, dst) ((dst) |= bitmaskof(idx))
int xc_get_cpu_levelling_caps(xc_interface *xch, uint32_t *caps)
{
@@ -259,15 +257,42 @@ int xc_set_domain_cpu_policy(xc_interface *xch, uint32_t domid,
return ret;
}
+static int compare_leaves(const void *l, const void *r)
+{
+ const xen_cpuid_leaf_t *lhs = l;
+ const xen_cpuid_leaf_t *rhs = r;
+
+ if ( lhs->leaf != rhs->leaf )
+ return lhs->leaf < rhs->leaf ? -1 : 1;
+
+ if ( lhs->subleaf != rhs->subleaf )
+ return lhs->subleaf < rhs->subleaf ? -1 : 1;
+
+ return 0;
+}
+
+static xen_cpuid_leaf_t *find_leaf(
+ xen_cpuid_leaf_t *leaves, unsigned int nr_leaves,
+ const struct xc_xend_cpuid *xend)
+{
+ const xen_cpuid_leaf_t key = { xend->leaf, xend->subleaf };
+
+ return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
+}
+
int xc_cpuid_set(
xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
{
int rc;
- unsigned int i, j, regs[4] = {}, polregs[4] = {};
xc_dominfo_t di;
- xen_cpuid_leaf_t *leaves = NULL;
- unsigned int nr_leaves, policy_leaves, nr_msrs;
+ unsigned int nr_leaves, nr_msrs;
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+ /*
+ * Three full policies. The host, domain max, and domain current for the
+ * domain type.
+ */
+ xen_cpuid_leaf_t *host = NULL, *max = NULL, *cur = NULL;
+ unsigned int nr_host, nr_max, nr_cur;
if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
di.domid != domid )
@@ -286,99 +311,101 @@ int xc_cpuid_set(
}
rc = -ENOMEM;
- if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
+ if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
+ (max = calloc(nr_leaves, sizeof(*max))) == NULL ||
+ (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
{
ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
goto fail;
}
+ /* Get the domain's current policy. */
+ nr_msrs = 0;
+ nr_cur = nr_leaves;
+ rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
+ if ( rc )
+ {
+ PERROR("Failed to obtain d%d current policy", domid);
+ rc = -errno;
+ goto fail;
+ }
+
/* Get the domain's max policy. */
nr_msrs = 0;
- policy_leaves = nr_leaves;
+ nr_max = nr_leaves;
rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
: XEN_SYSCTL_cpu_policy_pv_max,
- &policy_leaves, leaves, &nr_msrs, NULL);
+ &nr_max, max, &nr_msrs, NULL);
if ( rc )
{
PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
rc = -errno;
goto fail;
}
- for ( i = 0; i < policy_leaves; ++i )
- if ( leaves[i].leaf == xend->leaf &&
- leaves[i].subleaf == xend->subleaf )
- {
- polregs[0] = leaves[i].a;
- polregs[1] = leaves[i].b;
- polregs[2] = leaves[i].c;
- polregs[3] = leaves[i].d;
- break;
- }
/* Get the host policy. */
nr_msrs = 0;
- policy_leaves = nr_leaves;
+ nr_host = nr_leaves;
rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
- &policy_leaves, leaves, &nr_msrs, NULL);
+ &nr_host, host, &nr_msrs, NULL);
if ( rc )
{
PERROR("Failed to obtain host policy");
rc = -errno;
goto fail;
}
- for ( i = 0; i < policy_leaves; ++i )
- if ( leaves[i].leaf == xend->leaf &&
- leaves[i].subleaf == xend->subleaf )
- {
- regs[0] = leaves[i].a;
- regs[1] = leaves[i].b;
- regs[2] = leaves[i].c;
- regs[3] = leaves[i].d;
- break;
- }
- for ( i = 0; i < 4; i++ )
+ rc = -EINVAL;
+ for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
{
- if ( xend->policy[i] == NULL )
+ xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
+ const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
+ const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
+
+ if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
{
- regs[i] = polregs[i];
- continue;
+ ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
+ goto fail;
}
- /*
- * Notes for following this algorithm:
- *
- * While it will accept any leaf data, it only makes sense to use on
- * feature leaves. regs[] initially contains the host values. This,
- * with the fall-through chain, is how the 's' and 'k' options work.
- */
- for ( j = 0; j < 32; j++ )
+ for ( int i = 0; i < 4; i++ )
{
- unsigned char val = !!((regs[i] & (1U << (31 - j))));
- unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
-
- rc = -EINVAL;
- if ( !strchr("10xks", xend->policy[i][j]) )
- goto fail;
-
- if ( xend->policy[i][j] == '1' )
- val = 1;
- else if ( xend->policy[i][j] == '0' )
- val = 0;
- else if ( xend->policy[i][j] == 'x' )
- val = polval;
-
- if ( val )
- set_feature(31 - j, regs[i]);
- else
- clear_feature(31 - j, regs[i]);
+ uint32_t *cur_reg = &cur_leaf->a + i;
+ const uint32_t *max_reg = &max_leaf->a + i;
+ const uint32_t *host_reg = &host_leaf->a + i;
+
+ if ( xend->policy[i] == NULL )
+ continue;
+
+ for ( int j = 0; j < 32; j++ )
+ {
+ bool val;
+
+ if ( xend->policy[i][j] == '1' )
+ val = true;
+ else if ( xend->policy[i][j] == '0' )
+ val = false;
+ else if ( xend->policy[i][j] == 'x' )
+ val = test_bit(31 - j, max_reg);
+ else if ( xend->policy[i][j] == 'k' ||
+ xend->policy[i][j] == 's' )
+ val = test_bit(31 - j, host_reg);
+ else
+ {
+ ERROR("Bad character '%c' in policy[%d] string '%s'",
+ xend->policy[i][j], i, xend->policy[i]);
+ goto fail;
+ }
+
+ clear_bit(31 - j, cur_reg);
+ if ( val )
+ set_bit(31 - j, cur_reg);
+ }
}
}
- /* Feed the transformed leaf back up to Xen. */
- leaves[0] = (xen_cpuid_leaf_t){ xend->leaf, xend->subleaf,
- regs[0], regs[1], regs[2], regs[3] };
- rc = xc_set_domain_cpu_policy(xch, domid, 1, leaves, 0, NULL,
+ /* Feed the transformed currrent policy back up to Xen. */
+ rc = xc_set_domain_cpu_policy(xch, domid, nr_cur, cur, 0, NULL,
&err_leaf, &err_subleaf, &err_msr);
if ( rc )
{
@@ -391,7 +418,9 @@ int xc_cpuid_set(
/* Success! */
fail:
- free(leaves);
+ free(cur);
+ free(max);
+ free(host);
return rc;
}
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index e001cbcd4f..6f7cf2054e 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -420,7 +420,6 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
libxl_domain_build_info *info)
{
libxl_cpuid_policy_list cpuid = info->cpuid;
- int i;
bool pae = true;
/*
@@ -441,8 +440,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
if (!cpuid)
return;
- for (i = 0; cpuid[i].input[0] != XEN_CPUID_INPUT_UNUSED; i++)
- xc_cpuid_set(ctx->xch, domid, &cpuid[i]);
+ xc_cpuid_set(ctx->xch, domid, info->cpuid);
}
static const char *input_names[2] = { "leaf", "subleaf" };
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
2020-06-15 14:15 ` [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set() Andrew Cooper
@ 2020-06-15 14:54 ` Ian Jackson
2020-06-16 9:16 ` Jan Beulich
1 sibling, 0 replies; 40+ messages in thread
From: Ian Jackson @ 2020-06-15 14:54 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Paul Durrant, Jan Beulich, Wei Liu, Roger Pau Monne
Andrew Cooper writes ("[PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()"):
> Currently, libxl__cpuid_legacy() passes each element of the policy list to
> xc_cpuid_set() individually. This is wasteful both in terms of the number of
> hypercalls made, and the quantity of repeated merging/auditing work performed
> by Xen.
>
> Move the loop processing down into xc_cpuid_set(), which allows us to do one
> set of hypercalls, rather than one per list entry.
>
> In xc_cpuid_set(), obtain the full host, guest max and current policies to
> begin with, and loop over the xend array, processing one leaf at a time.
> Replace the linear search with a binary search, seeing as the serialised
> leaves are sorted.
>
> No change in behaviour from the guests point of view.
This is not my area of expertise. Ideally at this stage of the
release I would like an ack from a 2nd hypervisor maintainer.
The processing code in libxc looks OK to me.
Ian.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
2020-06-15 14:15 ` [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set() Andrew Cooper
2020-06-15 14:54 ` Ian Jackson
@ 2020-06-16 9:16 ` Jan Beulich
2020-06-16 15:58 ` Andrew Cooper
1 sibling, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 9:16 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 15.06.2020 16:15, Andrew Cooper wrote:
> Currently, libxl__cpuid_legacy() passes each element of the policy list to
> xc_cpuid_set() individually. This is wasteful both in terms of the number of
> hypercalls made, and the quantity of repeated merging/auditing work performed
> by Xen.
>
> Move the loop processing down into xc_cpuid_set(), which allows us to do one
> set of hypercalls, rather than one per list entry.
>
> In xc_cpuid_set(), obtain the full host, guest max and current policies to
> begin with, and loop over the xend array, processing one leaf at a time.
> Replace the linear search with a binary search, seeing as the serialised
> leaves are sorted.
>
> No change in behaviour from the guests point of view.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a few remarks:
> @@ -286,99 +311,101 @@ int xc_cpuid_set(
> }
>
> rc = -ENOMEM;
> - if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
> + if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
> + (max = calloc(nr_leaves, sizeof(*max))) == NULL ||
> + (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
> {
> ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
> goto fail;
> }
>
> + /* Get the domain's current policy. */
> + nr_msrs = 0;
> + nr_cur = nr_leaves;
> + rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
> + if ( rc )
> + {
> + PERROR("Failed to obtain d%d current policy", domid);
> + rc = -errno;
> + goto fail;
> + }
> +
> /* Get the domain's max policy. */
> nr_msrs = 0;
> - policy_leaves = nr_leaves;
> + nr_max = nr_leaves;
> rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
> : XEN_SYSCTL_cpu_policy_pv_max,
> - &policy_leaves, leaves, &nr_msrs, NULL);
> + &nr_max, max, &nr_msrs, NULL);
> if ( rc )
> {
> PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
> rc = -errno;
> goto fail;
> }
> - for ( i = 0; i < policy_leaves; ++i )
> - if ( leaves[i].leaf == xend->leaf &&
> - leaves[i].subleaf == xend->subleaf )
> - {
> - polregs[0] = leaves[i].a;
> - polregs[1] = leaves[i].b;
> - polregs[2] = leaves[i].c;
> - polregs[3] = leaves[i].d;
> - break;
> - }
>
> /* Get the host policy. */
> nr_msrs = 0;
> - policy_leaves = nr_leaves;
> + nr_host = nr_leaves;
> rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
> - &policy_leaves, leaves, &nr_msrs, NULL);
> + &nr_host, host, &nr_msrs, NULL);
> if ( rc )
> {
> PERROR("Failed to obtain host policy");
> rc = -errno;
> goto fail;
> }
> - for ( i = 0; i < policy_leaves; ++i )
> - if ( leaves[i].leaf == xend->leaf &&
> - leaves[i].subleaf == xend->subleaf )
> - {
> - regs[0] = leaves[i].a;
> - regs[1] = leaves[i].b;
> - regs[2] = leaves[i].c;
> - regs[3] = leaves[i].d;
> - break;
> - }
>
> - for ( i = 0; i < 4; i++ )
> + rc = -EINVAL;
> + for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
> {
> - if ( xend->policy[i] == NULL )
> + xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
> + const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
> + const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
> +
> + if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
> {
> - regs[i] = polregs[i];
> - continue;
> + ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
> + goto fail;
> }
>
> - /*
> - * Notes for following this algorithm:
> - *
> - * While it will accept any leaf data, it only makes sense to use on
> - * feature leaves. regs[] initially contains the host values. This,
> - * with the fall-through chain, is how the 's' and 'k' options work.
> - */
> - for ( j = 0; j < 32; j++ )
> + for ( int i = 0; i < 4; i++ )
As you move the declaration here, perhaps switch to unsigned int
as well? And express 4 as ARRAY_SIZE()?
> {
> - unsigned char val = !!((regs[i] & (1U << (31 - j))));
> - unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
> -
> - rc = -EINVAL;
> - if ( !strchr("10xks", xend->policy[i][j]) )
> - goto fail;
> -
> - if ( xend->policy[i][j] == '1' )
> - val = 1;
> - else if ( xend->policy[i][j] == '0' )
> - val = 0;
> - else if ( xend->policy[i][j] == 'x' )
> - val = polval;
> -
> - if ( val )
> - set_feature(31 - j, regs[i]);
> - else
> - clear_feature(31 - j, regs[i]);
> + uint32_t *cur_reg = &cur_leaf->a + i;
> + const uint32_t *max_reg = &max_leaf->a + i;
> + const uint32_t *host_reg = &host_leaf->a + i;
> +
> + if ( xend->policy[i] == NULL )
> + continue;
> +
> + for ( int j = 0; j < 32; j++ )
unsigned int again? I don't think there's a suitable array available
to also use ARRAY_SIZE() here.
> + {
> + bool val;
> +
> + if ( xend->policy[i][j] == '1' )
> + val = true;
> + else if ( xend->policy[i][j] == '0' )
> + val = false;
> + else if ( xend->policy[i][j] == 'x' )
> + val = test_bit(31 - j, max_reg);
Still seeing "max" used here is somewhat confusing given the purpose
of the series, and merely judging from the titles I can't yet spot
where later on it'll change. But I assume it will ...
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set()
2020-06-16 9:16 ` Jan Beulich
@ 2020-06-16 15:58 ` Andrew Cooper
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-16 15:58 UTC (permalink / raw)
To: Jan Beulich
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 16/06/2020 10:16, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> Currently, libxl__cpuid_legacy() passes each element of the policy list to
>> xc_cpuid_set() individually. This is wasteful both in terms of the number of
>> hypercalls made, and the quantity of repeated merging/auditing work performed
>> by Xen.
>>
>> Move the loop processing down into xc_cpuid_set(), which allows us to do one
>> set of hypercalls, rather than one per list entry.
>>
>> In xc_cpuid_set(), obtain the full host, guest max and current policies to
>> begin with, and loop over the xend array, processing one leaf at a time.
>> Replace the linear search with a binary search, seeing as the serialised
>> leaves are sorted.
>>
>> No change in behaviour from the guests point of view.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> with a few remarks:
>
>> @@ -286,99 +311,101 @@ int xc_cpuid_set(
>> }
>>
>> rc = -ENOMEM;
>> - if ( (leaves = calloc(nr_leaves, sizeof(*leaves))) == NULL )
>> + if ( (host = calloc(nr_leaves, sizeof(*host))) == NULL ||
>> + (max = calloc(nr_leaves, sizeof(*max))) == NULL ||
>> + (cur = calloc(nr_leaves, sizeof(*cur))) == NULL )
>> {
>> ERROR("Unable to allocate memory for %u CPUID leaves", nr_leaves);
>> goto fail;
>> }
>>
>> + /* Get the domain's current policy. */
>> + nr_msrs = 0;
>> + nr_cur = nr_leaves;
>> + rc = xc_get_domain_cpu_policy(xch, domid, &nr_cur, cur, &nr_msrs, NULL);
>> + if ( rc )
>> + {
>> + PERROR("Failed to obtain d%d current policy", domid);
>> + rc = -errno;
>> + goto fail;
>> + }
>> +
>> /* Get the domain's max policy. */
>> nr_msrs = 0;
>> - policy_leaves = nr_leaves;
>> + nr_max = nr_leaves;
>> rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_max
>> : XEN_SYSCTL_cpu_policy_pv_max,
>> - &policy_leaves, leaves, &nr_msrs, NULL);
>> + &nr_max, max, &nr_msrs, NULL);
>> if ( rc )
>> {
>> PERROR("Failed to obtain %s max policy", di.hvm ? "hvm" : "pv");
>> rc = -errno;
>> goto fail;
>> }
>> - for ( i = 0; i < policy_leaves; ++i )
>> - if ( leaves[i].leaf == xend->leaf &&
>> - leaves[i].subleaf == xend->subleaf )
>> - {
>> - polregs[0] = leaves[i].a;
>> - polregs[1] = leaves[i].b;
>> - polregs[2] = leaves[i].c;
>> - polregs[3] = leaves[i].d;
>> - break;
>> - }
>>
>> /* Get the host policy. */
>> nr_msrs = 0;
>> - policy_leaves = nr_leaves;
>> + nr_host = nr_leaves;
>> rc = xc_get_system_cpu_policy(xch, XEN_SYSCTL_cpu_policy_host,
>> - &policy_leaves, leaves, &nr_msrs, NULL);
>> + &nr_host, host, &nr_msrs, NULL);
>> if ( rc )
>> {
>> PERROR("Failed to obtain host policy");
>> rc = -errno;
>> goto fail;
>> }
>> - for ( i = 0; i < policy_leaves; ++i )
>> - if ( leaves[i].leaf == xend->leaf &&
>> - leaves[i].subleaf == xend->subleaf )
>> - {
>> - regs[0] = leaves[i].a;
>> - regs[1] = leaves[i].b;
>> - regs[2] = leaves[i].c;
>> - regs[3] = leaves[i].d;
>> - break;
>> - }
>>
>> - for ( i = 0; i < 4; i++ )
>> + rc = -EINVAL;
>> + for ( ; xend->leaf != XEN_CPUID_INPUT_UNUSED; ++xend )
>> {
>> - if ( xend->policy[i] == NULL )
>> + xen_cpuid_leaf_t *cur_leaf = find_leaf(cur, nr_cur, xend);
>> + const xen_cpuid_leaf_t *max_leaf = find_leaf(max, nr_max, xend);
>> + const xen_cpuid_leaf_t *host_leaf = find_leaf(host, nr_host, xend);
>> +
>> + if ( cur_leaf == NULL || max_leaf == NULL || host_leaf == NULL )
>> {
>> - regs[i] = polregs[i];
>> - continue;
>> + ERROR("Missing leaf %#x, subleaf %#x", xend->leaf, xend->subleaf);
>> + goto fail;
>> }
>>
>> - /*
>> - * Notes for following this algorithm:
>> - *
>> - * While it will accept any leaf data, it only makes sense to use on
>> - * feature leaves. regs[] initially contains the host values. This,
>> - * with the fall-through chain, is how the 's' and 'k' options work.
>> - */
>> - for ( j = 0; j < 32; j++ )
>> + for ( int i = 0; i < 4; i++ )
> As you move the declaration here, perhaps switch to unsigned int
> as well? And express 4 as ARRAY_SIZE()?
>
>> {
>> - unsigned char val = !!((regs[i] & (1U << (31 - j))));
>> - unsigned char polval = !!((polregs[i] & (1U << (31 - j))));
>> -
>> - rc = -EINVAL;
>> - if ( !strchr("10xks", xend->policy[i][j]) )
>> - goto fail;
>> -
>> - if ( xend->policy[i][j] == '1' )
>> - val = 1;
>> - else if ( xend->policy[i][j] == '0' )
>> - val = 0;
>> - else if ( xend->policy[i][j] == 'x' )
>> - val = polval;
>> -
>> - if ( val )
>> - set_feature(31 - j, regs[i]);
>> - else
>> - clear_feature(31 - j, regs[i]);
>> + uint32_t *cur_reg = &cur_leaf->a + i;
>> + const uint32_t *max_reg = &max_leaf->a + i;
>> + const uint32_t *host_reg = &host_leaf->a + i;
>> +
>> + if ( xend->policy[i] == NULL )
>> + continue;
>> +
>> + for ( int j = 0; j < 32; j++ )
> unsigned int again? I don't think there's a suitable array available
> to also use ARRAY_SIZE() here.
All fixed.
>
>> + {
>> + bool val;
>> +
>> + if ( xend->policy[i][j] == '1' )
>> + val = true;
>> + else if ( xend->policy[i][j] == '0' )
>> + val = false;
>> + else if ( xend->policy[i][j] == 'x' )
>> + val = test_bit(31 - j, max_reg);
> Still seeing "max" used here is somewhat confusing given the purpose
> of the series, and merely judging from the titles I can't yet spot
> where later on it'll change. But I assume it will ...
No - it won't change. The legacy xend adjustments have always been
based on the max featureset, and changing it will break VM migration.
The soon-to-exist (4.15 at this point) brand new "what do I do for a
fresh boot" path will do things differently even for the legacy Xend
leaves, but the logic here must not semantically change.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 4/9] tools/libx[cl]: Merge xc_cpuid_set() into xc_cpuid_apply_policy()
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (2 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 3/9] tools/libx[cl]: Move processing loop down into xc_cpuid_set() Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:55 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 5/9] tools/libx[cl]: Plumb bool restore down " Andrew Cooper
` (6 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
This reduces the number of CPUID handling entry-points to just one.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
---
tools/libxc/include/xenctrl.h | 9 ++++-----
tools/libxc/xc_cpuid_x86.c | 8 ++++++--
tools/libxl/libxl_cpuid.c | 8 +-------
3 files changed, 11 insertions(+), 14 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 178144e8e2..5f0978e0e5 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1819,20 +1819,19 @@ struct xc_xend_cpuid {
char *policy[4];
};
-int xc_cpuid_set(xc_interface *xch,
- uint32_t domid,
- const struct xc_xend_cpuid *xend);
-
/*
* Make adjustments to the CPUID settings for a domain.
*
* Either pass a full new @featureset (and @nr_features), or adjust individual
* features (@pae).
+ *
+ * Then (optionally) apply legacy XEND overrides (@xend) to the result.
*/
int xc_cpuid_apply_policy(xc_interface *xch,
uint32_t domid,
const uint32_t *featureset,
- unsigned int nr_features, bool pae);
+ unsigned int nr_features, bool pae,
+ const struct xc_xend_cpuid *xend);
int xc_mca_op(xc_interface *xch, struct xen_mc *mc);
int xc_mca_op_inject_v2(xc_interface *xch, unsigned int flags,
xc_cpumap_t cpumap, unsigned int nr_cpus);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index e827c48fd1..26a7b94dcf 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -280,7 +280,7 @@ static xen_cpuid_leaf_t *find_leaf(
return bsearch(&key, leaves, nr_leaves, sizeof(*leaves), compare_leaves);
}
-int xc_cpuid_set(
+static int xc_cpuid_xend_policy(
xc_interface *xch, uint32_t domid, const struct xc_xend_cpuid *xend)
{
int rc;
@@ -427,7 +427,8 @@ int xc_cpuid_set(
int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
const uint32_t *featureset, unsigned int nr_features,
- bool pae)
+ bool pae,
+ const struct xc_xend_cpuid *xend)
{
int rc;
xc_dominfo_t di;
@@ -637,6 +638,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
goto out;
}
+ if ( xend && (rc = xc_cpuid_xend_policy(xch, domid, xend)) )
+ goto out;
+
rc = 0;
out:
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index 6f7cf2054e..edfcf315ca 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -419,7 +419,6 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
libxl_domain_build_info *info)
{
- libxl_cpuid_policy_list cpuid = info->cpuid;
bool pae = true;
/*
@@ -435,12 +434,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
if (info->type == LIBXL_DOMAIN_TYPE_HVM)
pae = libxl_defbool_val(info->u.hvm.pae);
- xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae);
-
- if (!cpuid)
- return;
-
- xc_cpuid_set(ctx->xch, domid, info->cpuid);
+ xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae, info->cpuid);
}
static const char *input_names[2] = { "leaf", "subleaf" };
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 5/9] tools/libx[cl]: Plumb bool restore down into xc_cpuid_apply_policy()
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (3 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 4/9] tools/libx[cl]: Merge xc_cpuid_set() into xc_cpuid_apply_policy() Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:55 ` Ian Jackson
2020-06-15 14:15 ` [PATCH 6/9] x86/gen-cpuid: Distinguish default vs max in feature annotations Andrew Cooper
` (5 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
In order to safely disable some features by default, without breaking
migration from 4.13 or older, the CPUID logic needs to distinguish the two
cases.
Plumb a restore boolean down from the two callers of libxl__cpuid_legacy() all
the way down into xc_cpuid_apply_policy().
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
Ideally, I'd have started the brand new CPUID/MSR interface for the boot path
before cleaning up the legacy path, but that's far too much work to squeeze
into 4.14 at this point. The restore boolean will do for now, and will
disappear eventually.
---
tools/libxc/include/xenctrl.h | 7 ++++++-
tools/libxc/xc_cpuid_x86.c | 2 +-
tools/libxl/libxl_cpuid.c | 4 ++--
tools/libxl/libxl_create.c | 2 +-
tools/libxl/libxl_dom.c | 2 +-
tools/libxl/libxl_internal.h | 2 +-
tools/libxl/libxl_nocpuid.c | 2 +-
7 files changed, 13 insertions(+), 8 deletions(-)
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 5f0978e0e5..634be88ac1 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1822,13 +1822,18 @@ struct xc_xend_cpuid {
/*
* Make adjustments to the CPUID settings for a domain.
*
+ * This path is used in two cases. First, for fresh boots of the domain, and
+ * secondly for migrate-in/restore of pre-4.14 guests (where CPUID data was
+ * missing from the stream). The @restore parameter distinguishes these
+ * cases, and the generated policy must be compatible with a 4.13.
+ *
* Either pass a full new @featureset (and @nr_features), or adjust individual
* features (@pae).
*
* Then (optionally) apply legacy XEND overrides (@xend) to the result.
*/
int xc_cpuid_apply_policy(xc_interface *xch,
- uint32_t domid,
+ uint32_t domid, bool restore,
const uint32_t *featureset,
unsigned int nr_features, bool pae,
const struct xc_xend_cpuid *xend);
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 26a7b94dcf..e017abffce 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -425,7 +425,7 @@ static int xc_cpuid_xend_policy(
return rc;
}
-int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid,
+int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
const uint32_t *featureset, unsigned int nr_features,
bool pae,
const struct xc_xend_cpuid *xend)
diff --git a/tools/libxl/libxl_cpuid.c b/tools/libxl/libxl_cpuid.c
index edfcf315ca..db2f12d115 100644
--- a/tools/libxl/libxl_cpuid.c
+++ b/tools/libxl/libxl_cpuid.c
@@ -416,7 +416,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
return 0;
}
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
libxl_domain_build_info *info)
{
bool pae = true;
@@ -434,7 +434,7 @@ void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
if (info->type == LIBXL_DOMAIN_TYPE_HVM)
pae = libxl_defbool_val(info->u.hvm.pae);
- xc_cpuid_apply_policy(ctx->xch, domid, NULL, 0, pae, info->cpuid);
+ xc_cpuid_apply_policy(ctx->xch, domid, restore, NULL, 0, pae, info->cpuid);
}
static const char *input_names[2] = { "leaf", "subleaf" };
diff --git a/tools/libxl/libxl_create.c b/tools/libxl/libxl_create.c
index 75862dc6ed..2814818e34 100644
--- a/tools/libxl/libxl_create.c
+++ b/tools/libxl/libxl_create.c
@@ -1447,7 +1447,7 @@ int libxl__srm_callout_callback_static_data_done(unsigned int missing,
* stream doesn't contain any CPUID data.
*/
if (missing & XGR_SDD_MISSING_CPUID)
- libxl__cpuid_legacy(ctx, dcs->guest_domid, info);
+ libxl__cpuid_legacy(ctx, dcs->guest_domid, true, info);
return 0;
}
diff --git a/tools/libxl/libxl_dom.c b/tools/libxl/libxl_dom.c
index dd1aff89a3..f8661e90d4 100644
--- a/tools/libxl/libxl_dom.c
+++ b/tools/libxl/libxl_dom.c
@@ -391,7 +391,7 @@ int libxl__build_pre(libxl__gc *gc, uint32_t domid,
* being migrated-in/restored have CPUID handled during the
* static_data_done() callback. */
if (!state->restore)
- libxl__cpuid_legacy(ctx, domid, info);
+ libxl__cpuid_legacy(ctx, domid, false, info);
return rc;
}
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 79c2bf5f5e..94a23179d3 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -2056,7 +2056,7 @@ typedef yajl_gen_status (*libxl__gen_json_callback)(yajl_gen hand, void *);
_hidden char *libxl__object_to_json(libxl_ctx *ctx, const char *type,
libxl__gen_json_callback gen, void *p);
-_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+_hidden void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool retore,
libxl_domain_build_info *info);
/* Calls poll() again - useful to check whether a signaled condition
diff --git a/tools/libxl/libxl_nocpuid.c b/tools/libxl/libxl_nocpuid.c
index 3f30e148be..f47336565b 100644
--- a/tools/libxl/libxl_nocpuid.c
+++ b/tools/libxl/libxl_nocpuid.c
@@ -34,7 +34,7 @@ int libxl_cpuid_parse_config_xend(libxl_cpuid_policy_list *cpuid,
return 0;
}
-void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid,
+void libxl__cpuid_legacy(libxl_ctx *ctx, uint32_t domid, bool restore,
libxl_domain_build_info *info)
{
}
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 5/9] tools/libx[cl]: Plumb bool restore down into xc_cpuid_apply_policy()
2020-06-15 14:15 ` [PATCH 5/9] tools/libx[cl]: Plumb bool restore down " Andrew Cooper
@ 2020-06-15 14:55 ` Ian Jackson
0 siblings, 0 replies; 40+ messages in thread
From: Ian Jackson @ 2020-06-15 14:55 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Paul Durrant, Jan Beulich, Wei Liu, Roger Pau Monne
Andrew Cooper writes ("[PATCH 5/9] tools/libx[cl]: Plumb bool restore down into xc_cpuid_apply_policy()"):
> In order to safely disable some features by default, without breaking
> migration from 4.13 or older, the CPUID logic needs to distinguish the two
> cases.
>
> Plumb a restore boolean down from the two callers of libxl__cpuid_legacy() all
> the way down into xc_cpuid_apply_policy().
>
> No functional change.
Acked-by: Ian Jackson <ian.jackson@eu.citrix.com>
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 6/9] x86/gen-cpuid: Distinguish default vs max in feature annotations
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (4 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 5/9] tools/libx[cl]: Plumb bool restore down " Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-15 14:15 ` [PATCH 7/9] x86/hvm: Disable MPX by default Andrew Cooper
` (4 subsequent siblings)
10 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
The toolstack logic can now correctly distinguish a clean boot from a
migrate/restore.
Allow lowercase a/s/h to be used to annotate a non-default feature.
Due to the emulator work prepared earlier in 4.14, this now allows VMs to
explicity opt in to the TSXLDTRK, MOVDIR{I,64B} and SERIALIZE instructions via
their xl.cfg file, rather than getting them as a matter of default.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
---
xen/tools/gen-cpuid.py | 18 +++++++-----------
1 file changed, 7 insertions(+), 11 deletions(-)
diff --git a/xen/tools/gen-cpuid.py b/xen/tools/gen-cpuid.py
index 037954cfb8..ffd9529fdf 100755
--- a/xen/tools/gen-cpuid.py
+++ b/xen/tools/gen-cpuid.py
@@ -130,17 +130,13 @@ def crunch_numbers(state):
MTRR, PGE, MCA, CMOV, PAT, PSE36, MMX, FXSR)
state.common_1d = common_1d
- state.pv_def = state.raw['A']
- state.hvm_shadow_def = state.pv_def | state.raw['S']
- state.hvm_hap_def = state.hvm_shadow_def | state.raw['H']
-
- # TODO: Ignore def/max split until the toolstack migration logic is fixed
- state.pv_max = state.pv_def
- state.hvm_shadow_max = state.hvm_shadow_def
- state.hvm_hap_max = state.hvm_hap_def
- # state.pv_max = state.raw['A'] | state.raw['a']
- # state.hvm_shadow_max = state.pv_max | state.raw['S'] | state.raw['s']
- # state.hvm_hap_max = state.hvm_shadow_max | state.raw['H'] | state.raw['h']
+ state.pv_def = state.raw['A']
+ state.hvm_shadow_def = state.pv_def | state.raw['S']
+ state.hvm_hap_def = state.hvm_shadow_def | state.raw['H']
+
+ state.pv_max = state.raw['A'] | state.raw['a']
+ state.hvm_shadow_max = state.pv_max | state.raw['S'] | state.raw['s']
+ state.hvm_hap_max = state.hvm_shadow_max | state.raw['H'] | state.raw['h']
#
# Feature dependency information.
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (5 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 6/9] x86/gen-cpuid: Distinguish default vs max in feature annotations Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-16 9:33 ` Jan Beulich
2020-06-15 14:15 ` [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy() Andrew Cooper
` (3 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
Memory Protection eXtension support has been dropped from GCC and Linux, and
will be dropped from future Intel CPUs.
With all other default/max pieces in place, move MPX from default to max.
This means that VMs won't be offered it by default, but can explicitly opt
into using it via cpuid="host,mpx=1" in their vm.cfg file.
The difference as visible to the guest is:
diff --git a/default b/mpx
index 0e91765d6b..c8c33cd584 100644
--- a/default
+++ b/mpx
@@ -13,15 +13,17 @@ Native cpuid:
00000004:00000004 -> 00000000:00000000:00000000:00000000
00000005:ffffffff -> 00000000:00000000:00000000:00000000
00000006:ffffffff -> 00000000:00000000:00000000:00000000
- 00000007:00000000 -> 00000000:009c2fbb:00000000:9c000400
+ 00000007:00000000 -> 00000000:009c6fbb:00000000:9c000400
00000008:ffffffff -> 00000000:00000000:00000000:00000000
00000009:ffffffff -> 00000000:00000000:00000000:00000000
0000000a:ffffffff -> 00000000:00000000:00000000:00000000
0000000b:ffffffff -> 00000000:00000000:00000000:00000000
0000000c:ffffffff -> 00000000:00000000:00000000:00000000
- 0000000d:00000000 -> 00000007:00000240:00000340:00000000
+ 0000000d:00000000 -> 0000001f:00000240:00000440:00000000
0000000d:00000001 -> 0000000f:00000240:00000000:00000000
0000000d:00000002 -> 00000100:00000240:00000000:00000000
+ 0000000d:00000003 -> 00000040:000003c0:00000000:00000000
+ 0000000d:00000004 -> 00000040:00000400:00000000:00000000
40000000:ffffffff -> 40000005:566e6558:65584d4d:4d4d566e
40000001:ffffffff -> 0004000e:00000000:00000000:00000000
40000002:ffffffff -> 00000001:40000000:00000000:00000000
Adjust the legacy restore path in libxc to cope safely with pre-4.14 VMs.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Paul Durrant <paul@xen.org>
Dropped Jan's R-by from previous posting, as the patch has gained new
toolstack logic to avoid breaking migrate.
---
tools/libxc/xc_cpuid_x86.c | 48 ++++++++++++++++++-----------
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
2 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index e017abffce..5649913e69 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -436,6 +436,8 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
xen_cpuid_leaf_t *leaves = NULL;
struct cpuid_policy *p = NULL;
uint32_t err_leaf = -1, err_subleaf = -1, err_msr = -1;
+ uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
+ uint32_t len = ARRAY_SIZE(host_featureset);
if ( xc_domain_getinfo(xch, domid, 1, &di) != 1 ||
di.domid != domid )
@@ -458,6 +460,22 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
(p = calloc(1, sizeof(*p))) == NULL )
goto out;
+ /* Get the host policy. */
+ rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
+ &len, host_featureset);
+ if ( rc )
+ {
+ /* Tolerate "buffer too small", as we've got the bits we need. */
+ if ( errno == ENOBUFS )
+ rc = 0;
+ else
+ {
+ PERROR("Failed to obtain host featureset");
+ rc = -errno;
+ goto out;
+ }
+ }
+
/* Get the domain's default policy. */
nr_msrs = 0;
rc = xc_get_system_cpu_policy(xch, di.hvm ? XEN_SYSCTL_cpu_policy_hvm_default
@@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
goto out;
}
+ /*
+ * Account for feature which have been disabled by default since Xen 4.13,
+ * so migrated-in VM's don't risk seeing features disappearing.
+ */
+ if ( restore )
+ {
+ if ( di.hvm )
+ {
+ p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
+ }
+ }
+
if ( featureset )
{
uint32_t disabled_features[FEATURESET_NR_ENTRIES],
@@ -530,24 +560,6 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
if ( !di.hvm )
{
- uint32_t host_featureset[FEATURESET_NR_ENTRIES] = {};
- uint32_t len = ARRAY_SIZE(host_featureset);
-
- rc = xc_get_cpu_featureset(xch, XEN_SYSCTL_cpu_featureset_host,
- &len, host_featureset);
- if ( rc )
- {
- /* Tolerate "buffer too small", as we've got the bits we need. */
- if ( errno == ENOBUFS )
- rc = 0;
- else
- {
- PERROR("Failed to obtain host featureset");
- rc = -errno;
- goto out;
- }
- }
-
/*
* On hardware without CPUID Faulting, PV guests see real topology.
* As a consequence, they also need to see the host htt/cmp fields.
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index 5ca35d9d97..af1b8a96a6 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -207,7 +207,7 @@ XEN_CPUFEATURE(INVPCID, 5*32+10) /*H Invalidate Process Context ID */
XEN_CPUFEATURE(RTM, 5*32+11) /*A Restricted Transactional Memory */
XEN_CPUFEATURE(PQM, 5*32+12) /* Platform QoS Monitoring */
XEN_CPUFEATURE(NO_FPU_SEL, 5*32+13) /*! FPU CS/DS stored as zero */
-XEN_CPUFEATURE(MPX, 5*32+14) /*S Memory Protection Extensions */
+XEN_CPUFEATURE(MPX, 5*32+14) /*s Memory Protection Extensions */
XEN_CPUFEATURE(PQE, 5*32+15) /* Platform QoS Enforcement */
XEN_CPUFEATURE(AVX512F, 5*32+16) /*A AVX-512 Foundation Instructions */
XEN_CPUFEATURE(AVX512DQ, 5*32+17) /*A AVX-512 Doubleword & Quadword Instrs */
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-15 14:15 ` [PATCH 7/9] x86/hvm: Disable MPX by default Andrew Cooper
@ 2020-06-16 9:33 ` Jan Beulich
2020-06-16 16:15 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 9:33 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 15.06.2020 16:15, Andrew Cooper wrote:
> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
> goto out;
> }
>
> + /*
> + * Account for feature which have been disabled by default since Xen 4.13,
> + * so migrated-in VM's don't risk seeing features disappearing.
> + */
> + if ( restore )
> + {
> + if ( di.hvm )
> + {
> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
Why do you derive this from the host featureset instead of the max
one for the guest type? Also, while you modify p here, ...
> + }
> + }
> +
> if ( featureset )
> {
> uint32_t disabled_features[FEATURESET_NR_ENTRIES],
... the code in this if()'s body ignores p altogether. I realize the
only caller of the function passes NULL for "featureset", but I'd
like to understand the rationale here anyway before giving an R-b.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-16 9:33 ` Jan Beulich
@ 2020-06-16 16:15 ` Andrew Cooper
2020-06-17 10:32 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-16 16:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 16/06/2020 10:33, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>> goto out;
>> }
>>
>> + /*
>> + * Account for feature which have been disabled by default since Xen 4.13,
>> + * so migrated-in VM's don't risk seeing features disappearing.
>> + */
>> + if ( restore )
>> + {
>> + if ( di.hvm )
>> + {
>> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
> Why do you derive this from the host featureset instead of the max
> one for the guest type?
Because that is how the logic worked for 4.13.
Also, because we don't have easy access to the actual guest max
featureset at this point. I could add two new sysctl subops to
get_featureset, but the reason for not doing so before are still
applicable now.
There is a theoretical case where host MPX is visible but guest max is
hidden, and that is down to the vmentry controls. As this doesn't exist
in real hardware, I'm not terribly concerned about it.
> Also, while you modify p here, ...
>
>> + }
>> + }
>> +
>> if ( featureset )
>> {
>> uint32_t disabled_features[FEATURESET_NR_ENTRIES],
> ... the code in this if()'s body ignores p altogether.
That is correct.
> I realize the
> only caller of the function passes NULL for "featureset", but I'd
> like to understand the rationale here anyway before giving an R-b.
The meaning of 'featureset' is "here are the exact bits I want you to use".
It has existed since my original CPUID work in 4.6/4.7. Currently, the
only user in XenServer, and its a stopgap on the way to the "proper"
solution which I've been working on for the past several years.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-16 16:15 ` Andrew Cooper
@ 2020-06-17 10:32 ` Jan Beulich
2020-06-17 11:16 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-17 10:32 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 16.06.2020 18:15, Andrew Cooper wrote:
> On 16/06/2020 10:33, Jan Beulich wrote:
>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>> goto out;
>>> }
>>>
>>> + /*
>>> + * Account for feature which have been disabled by default since Xen 4.13,
>>> + * so migrated-in VM's don't risk seeing features disappearing.
>>> + */
>>> + if ( restore )
>>> + {
>>> + if ( di.hvm )
>>> + {
>>> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>> Why do you derive this from the host featureset instead of the max
>> one for the guest type?
>
> Because that is how the logic worked for 4.13.
>
> Also, because we don't have easy access to the actual guest max
> featureset at this point. I could add two new sysctl subops to
> get_featureset, but the reason for not doing so before are still
> applicable now.
>
> There is a theoretical case where host MPX is visible but guest max is
> hidden, and that is down to the vmentry controls. As this doesn't exist
> in real hardware, I'm not terribly concerned about it.
I'd also see us allow features to be kept for the host, but masked
off of the/some guest feature sets, by way of a to-be-introduced
command line option.
I take your reply to mean that you agree that conceptually it
ought to be max which gets used here, but there's no practical
difference at this point.
>> Also, while you modify p here, ...
>>
>>> + }
>>> + }
>>> +
>>> if ( featureset )
>>> {
>>> uint32_t disabled_features[FEATURESET_NR_ENTRIES],
>> ... the code in this if()'s body ignores p altogether.
>
> That is correct.
>
>> I realize the
>> only caller of the function passes NULL for "featureset", but I'd
>> like to understand the rationale here anyway before giving an R-b.
>
> The meaning of 'featureset' is "here are the exact bits I want you to use".
With validation to happen only in the hypervisor then, I suppose?
If for both parts my understanding is correct:
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-17 10:32 ` Jan Beulich
@ 2020-06-17 11:16 ` Andrew Cooper
2020-06-17 11:24 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-17 11:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 17/06/2020 11:32, Jan Beulich wrote:
> On 16.06.2020 18:15, Andrew Cooper wrote:
>> On 16/06/2020 10:33, Jan Beulich wrote:
>>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>>> goto out;
>>>> }
>>>>
>>>> + /*
>>>> + * Account for feature which have been disabled by default since Xen 4.13,
>>>> + * so migrated-in VM's don't risk seeing features disappearing.
>>>> + */
>>>> + if ( restore )
>>>> + {
>>>> + if ( di.hvm )
>>>> + {
>>>> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>>> Why do you derive this from the host featureset instead of the max
>>> one for the guest type?
>> Because that is how the logic worked for 4.13.
>>
>> Also, because we don't have easy access to the actual guest max
>> featureset at this point. I could add two new sysctl subops to
>> get_featureset, but the reason for not doing so before are still
>> applicable now.
>>
>> There is a theoretical case where host MPX is visible but guest max is
>> hidden, and that is down to the vmentry controls. As this doesn't exist
>> in real hardware, I'm not terribly concerned about it.
> I'd also see us allow features to be kept for the host, but masked
> off of the/some guest feature sets, by way of a to-be-introduced
> command line option.
What kind of usecase do you have in mind for this? We've got a load of
features which are blanket disabled for guests. I suppose `ler` et al
ought to have an impact, except for the fact that LBR at that level
isn't architectural and always expected.
> I take your reply to mean that you agree that conceptually it
> ought to be max which gets used here, but there's no practical
> difference at this point.
If max were trivially available, I'd use use that, but its not, and host
is equivalent in the cases we care about.
>
>>> Also, while you modify p here, ...
>>>
>>>> + }
>>>> + }
>>>> +
>>>> if ( featureset )
>>>> {
>>>> uint32_t disabled_features[FEATURESET_NR_ENTRIES],
>>> ... the code in this if()'s body ignores p altogether.
>> That is correct.
>>
>>> I realize the
>>> only caller of the function passes NULL for "featureset", but I'd
>>> like to understand the rationale here anyway before giving an R-b.
>> The meaning of 'featureset' is "here are the exact bits I want you to use".
> With validation to happen only in the hypervisor then, I suppose?
Almost none of this logic has any validation in the toolstack side (that
which does, was added by me fairly recently). Its going to be one of
several large changes in the new world.
> If for both parts my understanding is correct:
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-17 11:16 ` Andrew Cooper
@ 2020-06-17 11:24 ` Jan Beulich
2020-06-17 11:28 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-17 11:24 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 17.06.2020 13:16, Andrew Cooper wrote:
> On 17/06/2020 11:32, Jan Beulich wrote:
>> On 16.06.2020 18:15, Andrew Cooper wrote:
>>> On 16/06/2020 10:33, Jan Beulich wrote:
>>>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>>>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>>>> goto out;
>>>>> }
>>>>>
>>>>> + /*
>>>>> + * Account for feature which have been disabled by default since Xen 4.13,
>>>>> + * so migrated-in VM's don't risk seeing features disappearing.
>>>>> + */
>>>>> + if ( restore )
>>>>> + {
>>>>> + if ( di.hvm )
>>>>> + {
>>>>> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>>>> Why do you derive this from the host featureset instead of the max
>>>> one for the guest type?
>>> Because that is how the logic worked for 4.13.
>>>
>>> Also, because we don't have easy access to the actual guest max
>>> featureset at this point. I could add two new sysctl subops to
>>> get_featureset, but the reason for not doing so before are still
>>> applicable now.
>>>
>>> There is a theoretical case where host MPX is visible but guest max is
>>> hidden, and that is down to the vmentry controls. As this doesn't exist
>>> in real hardware, I'm not terribly concerned about it.
>> I'd also see us allow features to be kept for the host, but masked
>> off of the/some guest feature sets, by way of a to-be-introduced
>> command line option.
>
> What kind of usecase do you have in mind for this? We've got a load of
> features which are blanket disabled for guests. I suppose `ler` et al
> ought to have an impact, except for the fact that LBR at that level
> isn't architectural and always expected.
What I was thinking of was the kind of "none of my guests should use
AVX512 - let me disable it globally, rather than individually in
each guest's config" approach. Of course AVX512 is something we use
in Xen only to emulate guest insns, but I think the example still
serves the purpose.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-17 11:24 ` Jan Beulich
@ 2020-06-17 11:28 ` Andrew Cooper
2020-06-17 11:41 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-17 11:28 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 17/06/2020 12:24, Jan Beulich wrote:
> On 17.06.2020 13:16, Andrew Cooper wrote:
>> On 17/06/2020 11:32, Jan Beulich wrote:
>>> On 16.06.2020 18:15, Andrew Cooper wrote:
>>>> On 16/06/2020 10:33, Jan Beulich wrote:
>>>>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>>>>> @@ -479,6 +497,18 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>>>>> goto out;
>>>>>> }
>>>>>>
>>>>>> + /*
>>>>>> + * Account for feature which have been disabled by default since Xen 4.13,
>>>>>> + * so migrated-in VM's don't risk seeing features disappearing.
>>>>>> + */
>>>>>> + if ( restore )
>>>>>> + {
>>>>>> + if ( di.hvm )
>>>>>> + {
>>>>>> + p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
>>>>> Why do you derive this from the host featureset instead of the max
>>>>> one for the guest type?
>>>> Because that is how the logic worked for 4.13.
>>>>
>>>> Also, because we don't have easy access to the actual guest max
>>>> featureset at this point. I could add two new sysctl subops to
>>>> get_featureset, but the reason for not doing so before are still
>>>> applicable now.
>>>>
>>>> There is a theoretical case where host MPX is visible but guest max is
>>>> hidden, and that is down to the vmentry controls. As this doesn't exist
>>>> in real hardware, I'm not terribly concerned about it.
>>> I'd also see us allow features to be kept for the host, but masked
>>> off of the/some guest feature sets, by way of a to-be-introduced
>>> command line option.
>> What kind of usecase do you have in mind for this? We've got a load of
>> features which are blanket disabled for guests. I suppose `ler` et al
>> ought to have an impact, except for the fact that LBR at that level
>> isn't architectural and always expected.
> What I was thinking of was the kind of "none of my guests should use
> AVX512 - let me disable it globally, rather than individually in
> each guest's config" approach. Of course AVX512 is something we use
> in Xen only to emulate guest insns, but I think the example still
> serves the purpose.
We actually have AVX512 disabled by default in XenServer. The perf
implications of letting 1 guest play with it is very severe.
Now I think about it, I'm tempted to recommend it moves out of default
generally.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-17 11:28 ` Andrew Cooper
@ 2020-06-17 11:41 ` Jan Beulich
2020-06-17 11:47 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-17 11:41 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 17.06.2020 13:28, Andrew Cooper wrote:
> We actually have AVX512 disabled by default in XenServer. The perf
> implications of letting 1 guest play with it is very severe.
>
> Now I think about it, I'm tempted to recommend it moves out of default
> generally.
Hmm, I'm tempted to ask whether you're kidding. This is the kind of
feature that I see no reason at all to move out of default. Imo we
shouldn't put in place policy like this - if anything shouldn't be
on by default, it should imo be because of limitations in our
handling (I've recently revived my UMIP emulation patch, which
comes to mind here) or because of uncertainty on some aspects (like
is the case for MOVDIR / ENQCMD for example). Anything else should
be left to the admins to decide.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 7/9] x86/hvm: Disable MPX by default
2020-06-17 11:41 ` Jan Beulich
@ 2020-06-17 11:47 ` Andrew Cooper
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-17 11:47 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 17/06/2020 12:41, Jan Beulich wrote:
> On 17.06.2020 13:28, Andrew Cooper wrote:
>> We actually have AVX512 disabled by default in XenServer. The perf
>> implications of letting 1 guest play with it is very severe.
>>
>> Now I think about it, I'm tempted to recommend it moves out of default
>> generally.
> Hmm, I'm tempted to ask whether you're kidding.
I'm very definitely not.
AVX512 is a disaster, perf wise on Skylake/CascadeLake, and its very
easy to cripple the entire system, including the other guest.
So much so that "better AVX512 frequency transitions" is a headline
feature in IceLake.
> This is the kind of
> feature that I see no reason at all to move out of default. Imo we
> shouldn't put in place policy like this - if anything shouldn't be
> on by default, it should imo be because of limitations in our
> handling (I've recently revived my UMIP emulation patch, which
> comes to mind here) or because of uncertainty on some aspects (like
> is the case for MOVDIR / ENQCMD for example). Anything else should
> be left to the admins to decide.
"left to the admins to decide" does not mean "on by default".
"default" needs to be a sensible set, which migrates safely, and can't
be used to trivially DoS the rest of the system. An admin can always
opt into allowing this DoS, but shouldn't have it by default.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy()
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (6 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 7/9] x86/hvm: Disable MPX by default Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-16 9:40 ` Jan Beulich
2020-06-15 14:15 ` [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge Andrew Cooper
` (2 subsequent siblings)
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Andrew Cooper, Paul Durrant, Wei Liu, Jan Beulich, Roger Pau Monné
This was an accidental asymmetry with the HVM side.
No change in behaviour at this point.
Fixes: 83b387382 ("x86/cpuid: Introduce and use default CPUID policies")
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
---
xen/arch/x86/cpuid.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index ee11087626..f2fc0aa895 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -402,6 +402,8 @@ static void __init calculate_pv_def_policy(void)
for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
pv_featureset[i] &= pv_def_featuremask[i];
+ guest_common_feature_adjustments(pv_featureset);
+
sanitise_featureset(pv_featureset);
cpuid_featureset_to_policy(pv_featureset, p);
recalculate_xstate(p);
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy()
2020-06-15 14:15 ` [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy() Andrew Cooper
@ 2020-06-16 9:40 ` Jan Beulich
2020-06-16 16:17 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 9:40 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Paul Durrant, Wei Liu, Roger Pau Monné
On 15.06.2020 16:15, Andrew Cooper wrote:
> This was an accidental asymmetry with the HVM side.
>
> No change in behaviour at this point.
>
> Fixes: 83b387382 ("x86/cpuid: Introduce and use default CPUID policies")
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
with a remark:
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -402,6 +402,8 @@ static void __init calculate_pv_def_policy(void)
> for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
> pv_featureset[i] &= pv_def_featuremask[i];
>
> + guest_common_feature_adjustments(pv_featureset);
> +
> sanitise_featureset(pv_featureset);
> cpuid_featureset_to_policy(pv_featureset, p);
> recalculate_xstate(p);
These four calls are common to all three callers of the function.
Perhaps them going out of sync would be less likely if all four
called the same helper to carry out these four steps?
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy()
2020-06-16 9:40 ` Jan Beulich
@ 2020-06-16 16:17 ` Andrew Cooper
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-16 16:17 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Paul Durrant, Wei Liu, Roger Pau Monné
On 16/06/2020 10:40, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> This was an accidental asymmetry with the HVM side.
>>
>> No change in behaviour at this point.
>>
>> Fixes: 83b387382 ("x86/cpuid: Introduce and use default CPUID policies")
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
> with a remark:
>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -402,6 +402,8 @@ static void __init calculate_pv_def_policy(void)
>> for ( i = 0; i < ARRAY_SIZE(pv_featureset); ++i )
>> pv_featureset[i] &= pv_def_featuremask[i];
>>
>> + guest_common_feature_adjustments(pv_featureset);
>> +
>> sanitise_featureset(pv_featureset);
>> cpuid_featureset_to_policy(pv_featureset, p);
>> recalculate_xstate(p);
> These four calls are common to all three callers of the function.
> Perhaps them going out of sync would be less likely if all four
> called the same helper to carry out these four steps?
I'm not sure how many of them are going to survive the transformation to
a fully libx86 based world.
I expect it not to look exactly like this.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (7 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 8/9] x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy() Andrew Cooper
@ 2020-06-15 14:15 ` Andrew Cooper
2020-06-16 10:00 ` Jan Beulich
2020-06-15 17:04 ` [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Paul Durrant
2020-06-18 7:18 ` Jan Beulich
10 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-15 14:15 UTC (permalink / raw)
To: Xen-devel
Cc: Wei Liu, Paul Durrant, Andrew Cooper, Jan Beulich, Ian Jackson,
Roger Pau Monné
To combat the absence of mitigating microcode, arrange to hide RDRAND by
default on IvyBridge hardware.
Adjust the default feature derivation to hide RDRAND on IvyBridge client
parts, unless `cpuid=rdrand` is explicitly provided.
Adjust the restore path in xc_cpuid_apply_policy() to not hide RDRAND from VMs
which migrated from pre-4.14.
In all cases, individual guests can continue using RDRAND if explicitly
enabled in their config files.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Ian Jackson <Ian.Jackson@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul@xen.org>
---
docs/misc/xen-command-line.pandoc | 20 +++++++++++++++-----
tools/libxc/xc_cpuid_x86.c | 3 +++
xen/arch/x86/cpuid.c | 21 +++++++++++++++++++++
xen/include/public/arch-x86/cpufeatureset.h | 2 +-
4 files changed, 40 insertions(+), 6 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index fde749c669..c8ebfaf813 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -512,11 +512,21 @@ The Speculation Control hardware features `srbds-ctrl`, `md-clear`, `ibrsb`,
`stibp`, `ibpb`, `l1d-flush` and `ssbd` are used by default if available and
applicable. They can all be ignored.
-`rdrand` and `rdseed` can be ignored, as a mitigation to XSA-320 /
-CVE-2020-0543. The RDRAND feature is disabled by default on certain AMD
-systems, due to possible malfunctions after ACPI S3 suspend/resume. `rdrand`
-may be used in its positive form to override Xen's default behaviour on these
-systems, and make the feature fully usable.
+`rdrand` and `rdseed` have multiple interactions.
+
+* For Special Register Buffer Data Sampling (SRBDS, XSA-320, CVE-2020-0543),
+ RDRAND and RDSEED can be ignored.
+
+ Due to the absence microcode to address SRBDS on IvyBridge hardware, the
+ RDRAND feature is hidden by default for guests, unless `rdrand` is used in
+ its positive form. Irrespective of the default setting here, VMs can use
+ RDRAND if explicitly enabled in guest config file, and VMs already using
+ RDRAND can migrate in.
+
+* The RDRAND feature is disabled by default on AMD Fam15/16 systems, due to
+ possible malfunctions after ACPI S3 suspend/resume. `rdrand` may be used
+ in its positive form to override Xen's default behaviour on these systems,
+ and make the feature fully usable.
### cpuid_mask_cpu
> `= fam_0f_rev_[cdefg] | fam_10_rev_[bc] | fam_11_rev_b`
diff --git a/tools/libxc/xc_cpuid_x86.c b/tools/libxc/xc_cpuid_x86.c
index 5649913e69..877a5601f3 100644
--- a/tools/libxc/xc_cpuid_x86.c
+++ b/tools/libxc/xc_cpuid_x86.c
@@ -503,6 +503,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
*/
if ( restore )
{
+ if ( test_bit(X86_FEATURE_RDRAND, host_featureset) && !p->basic.rdrand )
+ p->basic.rdrand = true;
+
if ( di.hvm )
{
p->feat.mpx = test_bit(X86_FEATURE_MPX, host_featureset);
diff --git a/xen/arch/x86/cpuid.c b/xen/arch/x86/cpuid.c
index f2fc0aa895..6a4a787b68 100644
--- a/xen/arch/x86/cpuid.c
+++ b/xen/arch/x86/cpuid.c
@@ -340,6 +340,25 @@ static void __init calculate_host_policy(void)
}
}
+static void __init guest_common_default_feature_adjustments(uint32_t *fs)
+{
+ /*
+ * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
+ * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
+ * compensate.
+ *
+ * Mitigate by hiding RDRAND from guests by default, unless explicitly
+ * overridden on the Xen command line (cpuid=rdrand). Irrespective of the
+ * default setting, guests can use RDRAND if explicitly enabled
+ * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
+ * previously using RDRAND can migrate in.
+ */
+ if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
+ boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
+ cpu_has_rdrand && !is_forced_cpu_cap(X86_FEATURE_RDRAND) )
+ __clear_bit(X86_FEATURE_RDRAND, fs);
+}
+
static void __init guest_common_feature_adjustments(uint32_t *fs)
{
/* Unconditionally claim to be able to set the hypervisor bit. */
@@ -403,6 +422,7 @@ static void __init calculate_pv_def_policy(void)
pv_featureset[i] &= pv_def_featuremask[i];
guest_common_feature_adjustments(pv_featureset);
+ guest_common_default_feature_adjustments(pv_featureset);
sanitise_featureset(pv_featureset);
cpuid_featureset_to_policy(pv_featureset, p);
@@ -485,6 +505,7 @@ static void __init calculate_hvm_def_policy(void)
hvm_featureset[i] &= hvm_featuremask[i];
guest_common_feature_adjustments(hvm_featureset);
+ guest_common_default_feature_adjustments(hvm_featureset);
sanitise_featureset(hvm_featureset);
cpuid_featureset_to_policy(hvm_featureset, p);
diff --git a/xen/include/public/arch-x86/cpufeatureset.h b/xen/include/public/arch-x86/cpufeatureset.h
index af1b8a96a6..fe7492a225 100644
--- a/xen/include/public/arch-x86/cpufeatureset.h
+++ b/xen/include/public/arch-x86/cpufeatureset.h
@@ -149,7 +149,7 @@ XEN_CPUFEATURE(XSAVE, 1*32+26) /*A XSAVE/XRSTOR/XSETBV/XGETBV */
XEN_CPUFEATURE(OSXSAVE, 1*32+27) /*! OSXSAVE */
XEN_CPUFEATURE(AVX, 1*32+28) /*A Advanced Vector Extensions */
XEN_CPUFEATURE(F16C, 1*32+29) /*A Half-precision convert instruction */
-XEN_CPUFEATURE(RDRAND, 1*32+30) /*A Digital Random Number Generator */
+XEN_CPUFEATURE(RDRAND, 1*32+30) /*!A Digital Random Number Generator */
XEN_CPUFEATURE(HYPERVISOR, 1*32+31) /*!A Running under some hypervisor */
/* AMD-defined CPU features, CPUID level 0x80000001.edx, word 2 */
--
2.11.0
^ permalink raw reply related [flat|nested] 40+ messages in thread* Re: [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge
2020-06-15 14:15 ` [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge Andrew Cooper
@ 2020-06-16 10:00 ` Jan Beulich
2020-06-16 16:26 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-16 10:00 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 15.06.2020 16:15, Andrew Cooper wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -512,11 +512,21 @@ The Speculation Control hardware features `srbds-ctrl`, `md-clear`, `ibrsb`,
> `stibp`, `ibpb`, `l1d-flush` and `ssbd` are used by default if available and
> applicable. They can all be ignored.
>
> -`rdrand` and `rdseed` can be ignored, as a mitigation to XSA-320 /
> -CVE-2020-0543. The RDRAND feature is disabled by default on certain AMD
> -systems, due to possible malfunctions after ACPI S3 suspend/resume. `rdrand`
> -may be used in its positive form to override Xen's default behaviour on these
> -systems, and make the feature fully usable.
> +`rdrand` and `rdseed` have multiple interactions.
> +
> +* For Special Register Buffer Data Sampling (SRBDS, XSA-320, CVE-2020-0543),
> + RDRAND and RDSEED can be ignored.
> +
> + Due to the absence microcode to address SRBDS on IvyBridge hardware, the
Nit: "... absence of microcode ..."
> + RDRAND feature is hidden by default for guests, unless `rdrand` is used in
> + its positive form. Irrespective of the default setting here, VMs can use
> + RDRAND if explicitly enabled in guest config file, and VMs already using
> + RDRAND can migrate in.
I'm somewhat confused by the use of "default setting" here, when at the same
time you talk about our default behavior for guests. Aiui the two "default"
mean different things, so I'd suggest dropping that latter "default".
This raises a question though: Is disabling RDRAND just for guests good
enough? I.e. what about Xen's own uses of RDRAND? There may not be any
problematic ones right now, but wouldn't there be a latent issue no-one is
going to notice?
> --- a/tools/libxc/xc_cpuid_x86.c
> +++ b/tools/libxc/xc_cpuid_x86.c
> @@ -503,6 +503,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
> */
> if ( restore )
> {
> + if ( test_bit(X86_FEATURE_RDRAND, host_featureset) && !p->basic.rdrand )
> + p->basic.rdrand = true;
Same question as before: Why do you derive from the host feature set rather
than the domain type's maximum one?
> --- a/xen/arch/x86/cpuid.c
> +++ b/xen/arch/x86/cpuid.c
> @@ -340,6 +340,25 @@ static void __init calculate_host_policy(void)
> }
> }
>
> +static void __init guest_common_default_feature_adjustments(uint32_t *fs)
> +{
> + /*
> + * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
> + * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
> + * compensate.
> + *
> + * Mitigate by hiding RDRAND from guests by default, unless explicitly
> + * overridden on the Xen command line (cpuid=rdrand). Irrespective of the
> + * default setting, guests can use RDRAND if explicitly enabled
> + * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
> + * previously using RDRAND can migrate in.
> + */
> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
> + boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
This is the first time (description plus patch so far) that the issue
gets mentioned to be for and the workaround restricted to client parts
only. If so, I think at least the doc should say so too.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge
2020-06-16 10:00 ` Jan Beulich
@ 2020-06-16 16:26 ` Andrew Cooper
2020-06-17 10:39 ` Jan Beulich
0 siblings, 1 reply; 40+ messages in thread
From: Andrew Cooper @ 2020-06-16 16:26 UTC (permalink / raw)
To: Jan Beulich
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 16/06/2020 11:00, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> --- a/docs/misc/xen-command-line.pandoc
>> +++ b/docs/misc/xen-command-line.pandoc
>> @@ -512,11 +512,21 @@ The Speculation Control hardware features `srbds-ctrl`, `md-clear`, `ibrsb`,
>> `stibp`, `ibpb`, `l1d-flush` and `ssbd` are used by default if available and
>> applicable. They can all be ignored.
>>
>> -`rdrand` and `rdseed` can be ignored, as a mitigation to XSA-320 /
>> -CVE-2020-0543. The RDRAND feature is disabled by default on certain AMD
>> -systems, due to possible malfunctions after ACPI S3 suspend/resume. `rdrand`
>> -may be used in its positive form to override Xen's default behaviour on these
>> -systems, and make the feature fully usable.
>> +`rdrand` and `rdseed` have multiple interactions.
>> +
>> +* For Special Register Buffer Data Sampling (SRBDS, XSA-320, CVE-2020-0543),
>> + RDRAND and RDSEED can be ignored.
>> +
>> + Due to the absence microcode to address SRBDS on IvyBridge hardware, the
> Nit: "... absence of microcode ..."
Fixed.
>
>> + RDRAND feature is hidden by default for guests, unless `rdrand` is used in
>> + its positive form. Irrespective of the default setting here, VMs can use
>> + RDRAND if explicitly enabled in guest config file, and VMs already using
>> + RDRAND can migrate in.
> I'm somewhat confused by the use of "default setting" here, when at the same
> time you talk about our default behavior for guests. Aiui the two "default"
> mean different things, so I'd suggest dropping that latter "default".
Ok, done.
>
> This raises a question though: Is disabling RDRAND just for guests good
> enough? I.e. what about Xen's own uses of RDRAND? There may not be any
> problematic ones right now, but wouldn't there be a latent issue no-one is
> going to notice?
I was sorely tempted to delete all of Xen's use of RDRAND, seeing as its
not even safe to the AMD issue.
What we don't have is a "no-xen" concept for CPUID features, so I can't
stop Xen using it without hiding it from all guests, which in turn would
render the point of this series useless, and reintroduce the migration
problems we're trying to code around.
I was planning to leave the Xen uses as-are for now.
>
>> --- a/tools/libxc/xc_cpuid_x86.c
>> +++ b/tools/libxc/xc_cpuid_x86.c
>> @@ -503,6 +503,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>> */
>> if ( restore )
>> {
>> + if ( test_bit(X86_FEATURE_RDRAND, host_featureset) && !p->basic.rdrand )
>> + p->basic.rdrand = true;
> Same question as before: Why do you derive from the host feature set rather
> than the domain type's maximum one?
Answer the same as previous.
Although I do see now that this should be simplified to:
p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
which I've done.
>
>> --- a/xen/arch/x86/cpuid.c
>> +++ b/xen/arch/x86/cpuid.c
>> @@ -340,6 +340,25 @@ static void __init calculate_host_policy(void)
>> }
>> }
>>
>> +static void __init guest_common_default_feature_adjustments(uint32_t *fs)
>> +{
>> + /*
>> + * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
>> + * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
>> + * compensate.
>> + *
>> + * Mitigate by hiding RDRAND from guests by default, unless explicitly
>> + * overridden on the Xen command line (cpuid=rdrand). Irrespective of the
>> + * default setting, guests can use RDRAND if explicitly enabled
>> + * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
>> + * previously using RDRAND can migrate in.
>> + */
>> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>> + boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
> This is the first time (description plus patch so far) that the issue
> gets mentioned to be for and the workaround restricted to client parts
> only. If so, I think at least the doc should say so too.
I've updated the command line doc, and patch subject.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge
2020-06-16 16:26 ` Andrew Cooper
@ 2020-06-17 10:39 ` Jan Beulich
2020-06-17 11:21 ` Andrew Cooper
0 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-17 10:39 UTC (permalink / raw)
To: Andrew Cooper
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 16.06.2020 18:26, Andrew Cooper wrote:
> On 16/06/2020 11:00, Jan Beulich wrote:
>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>> --- a/tools/libxc/xc_cpuid_x86.c
>>> +++ b/tools/libxc/xc_cpuid_x86.c
>>> @@ -503,6 +503,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>> */
>>> if ( restore )
>>> {
>>> + if ( test_bit(X86_FEATURE_RDRAND, host_featureset) && !p->basic.rdrand )
>>> + p->basic.rdrand = true;
>> Same question as before: Why do you derive from the host feature set rather
>> than the domain type's maximum one?
>
> Answer the same as previous.
>
> Although I do see now that this should be simplified to:
>
> p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
>
> which I've done.
Right. It makes even more noticeable though that this may mean a
new feature suddenly appearing after the guest was migrated. But
aiui this still is the default behavior for all features anyway.
>>> --- a/xen/arch/x86/cpuid.c
>>> +++ b/xen/arch/x86/cpuid.c
>>> @@ -340,6 +340,25 @@ static void __init calculate_host_policy(void)
>>> }
>>> }
>>>
>>> +static void __init guest_common_default_feature_adjustments(uint32_t *fs)
>>> +{
>>> + /*
>>> + * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
>>> + * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
>>> + * compensate.
>>> + *
>>> + * Mitigate by hiding RDRAND from guests by default, unless explicitly
>>> + * overridden on the Xen command line (cpuid=rdrand). Irrespective of the
>>> + * default setting, guests can use RDRAND if explicitly enabled
>>> + * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
>>> + * previously using RDRAND can migrate in.
>>> + */
>>> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>>> + boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
>> This is the first time (description plus patch so far) that the issue
>> gets mentioned to be for and the workaround restricted to client parts
>> only. If so, I think at least the doc should say so too.
>
> I've updated the command line doc, and patch subject.
Thanks - with the adjustments
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge
2020-06-17 10:39 ` Jan Beulich
@ 2020-06-17 11:21 ` Andrew Cooper
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-17 11:21 UTC (permalink / raw)
To: Jan Beulich
Cc: Paul Durrant, Xen-devel, Roger Pau Monné, Wei Liu, Ian Jackson
On 17/06/2020 11:39, Jan Beulich wrote:
> On 16.06.2020 18:26, Andrew Cooper wrote:
>> On 16/06/2020 11:00, Jan Beulich wrote:
>>> On 15.06.2020 16:15, Andrew Cooper wrote:
>>>> --- a/tools/libxc/xc_cpuid_x86.c
>>>> +++ b/tools/libxc/xc_cpuid_x86.c
>>>> @@ -503,6 +503,9 @@ int xc_cpuid_apply_policy(xc_interface *xch, uint32_t domid, bool restore,
>>>> */
>>>> if ( restore )
>>>> {
>>>> + if ( test_bit(X86_FEATURE_RDRAND, host_featureset) && !p->basic.rdrand )
>>>> + p->basic.rdrand = true;
>>> Same question as before: Why do you derive from the host feature set rather
>>> than the domain type's maximum one?
>> Answer the same as previous.
>>
>> Although I do see now that this should be simplified to:
>>
>> p->basic.rdrand = test_bit(X86_FEATURE_RDRAND, host_featureset);
>>
>> which I've done.
> Right. It makes even more noticeable though that this may mean a
> new feature suddenly appearing after the guest was migrated. But
> aiui this still is the default behavior for all features anyway.
That is how migration always worked, until my migration v3 work in this
release.
I'm still surprised that it did, but both Linux and Windows were fine
with FMS changing on migrate (Linux because it never checked again,
while Windows would notice and install a new CPU HAL driver.)
>>>> --- a/xen/arch/x86/cpuid.c
>>>> +++ b/xen/arch/x86/cpuid.c
>>>> @@ -340,6 +340,25 @@ static void __init calculate_host_policy(void)
>>>> }
>>>> }
>>>>
>>>> +static void __init guest_common_default_feature_adjustments(uint32_t *fs)
>>>> +{
>>>> + /*
>>>> + * IvyBridge client parts suffer from leakage of RDRAND data due to SRBDS
>>>> + * (XSA-320 / CVE-2020-0543), and won't be receiving microcode to
>>>> + * compensate.
>>>> + *
>>>> + * Mitigate by hiding RDRAND from guests by default, unless explicitly
>>>> + * overridden on the Xen command line (cpuid=rdrand). Irrespective of the
>>>> + * default setting, guests can use RDRAND if explicitly enabled
>>>> + * (cpuid="host,rdrand=1") in the VM's config file, and VMs which were
>>>> + * previously using RDRAND can migrate in.
>>>> + */
>>>> + if ( boot_cpu_data.x86_vendor == X86_VENDOR_INTEL &&
>>>> + boot_cpu_data.x86 == 6 && boot_cpu_data.x86_model == 0x3a &&
>>> This is the first time (description plus patch so far) that the issue
>>> gets mentioned to be for and the workaround restricted to client parts
>>> only. If so, I think at least the doc should say so too.
>> I've updated the command line doc, and patch subject.
> Thanks - with the adjustments
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread
* RE: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (8 preceding siblings ...)
2020-06-15 14:15 ` [PATCH 9/9] x86/spec-ctrl: Hide RDRAND by default on IvyBridge Andrew Cooper
@ 2020-06-15 17:04 ` Paul Durrant
2020-06-17 12:46 ` Paul Durrant
2020-06-18 7:18 ` Jan Beulich
10 siblings, 1 reply; 40+ messages in thread
From: Paul Durrant @ 2020-06-15 17:04 UTC (permalink / raw)
To: 'Andrew Cooper', 'Xen-devel'
Cc: 'Ian Jackson', 'Jan Beulich', 'Wei Liu',
'Roger Pau Monné'
> -----Original Message-----
> From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
> Sent: 15 June 2020 15:15
> To: Xen-devel <xen-devel@lists.xenproject.org>
> Cc: Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>; Jan
> Beulich <JBeulich@suse.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné
> <roger.pau@citrix.com>
> Subject: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
>
> This is some work in light of IvyBridge not gaining microcode to combat SRBDS
> / XSA-320. It is a mix of some work I'd planned for 4.15, and some patches
> posted already and delayed due to dependence's I'd discovered after-the-fact.
>
> This provides a more user-friendly way of making IvyBridge safe by default
> without encountering migration incompatibilities.
>
> In terms of functionality, it finishes the "fresh boot" vs "migrate/restore
> from pre-4.14" split in the libxc CPUID logic, and uses this to let us safely
> hide features by default without breaking the "divine what a guest may have
> seen previously" logic on migrate.
>
> On top of that, we hide RDRAND by default to mitigate XSA-320.
>
> Additionally, take the opportunity of finally getting this logic working to
> hide MPX by default (as posted previously), due to upcoming Intel timelines.
>
> Request for 4.14. The IvyBridge angle only became apparent after the public
> embargo on Tue 9th. Otherwise, I would have made a concerted effort to get
> this logic sorted sooner and/or part of XSA-320 itself.
>
> Strictly speaking, patches 1-4 aren't necessary, but without them the logic is
> very confusing to follow, particularly the reasoning about the safely of later
> changes. As it is a simple set of transforms, we're better with them than
> without.
>
> Also, the MPX patch isn't related to the RDRAND issue, but I was planning to
> get it into 4.14 already, until realising that the migration path was broken.
> Now that the path is fixed for the RDRAND issue, include the MPX patch as it
> pertains to future hardware compatibility (and would be backported to 4.14.1
> if it misses 4.14.0).
>
Fair enough. Once the series has all the requisite maintainer acks then I'll release-ack it.
Paul
> Andrew Cooper (9):
> tools/libx[cl]: Introduce struct xc_xend_cpuid for xc_cpuid_set()
> tests/cpu-policy: Confirm that CPUID serialisation is sorted
> tools/libx[cl]: Move processing loop down into xc_cpuid_set()
> tools/libx[cl]: Merge xc_cpuid_set() into xc_cpuid_apply_policy()
> tools/libx[cl]: Plumb bool restore down into xc_cpuid_apply_policy()
> x86/gen-cpuid: Distinguish default vs max in feature annotations
> x86/hvm: Disable MPX by default
> x86/cpuid: Introduce missing feature adjustment in calculate_pv_def_policy()
> x86/spec-ctrl: Hide RDRAND by default on IvyBridge
>
> docs/misc/xen-command-line.pandoc | 20 ++-
> tools/libxc/include/xenctrl.h | 42 ++++-
> tools/libxc/xc_cpuid_x86.c | 239 ++++++++++++++++------------
> tools/libxl/libxl.h | 8 +-
> tools/libxl/libxl_cpuid.c | 17 +-
> tools/libxl/libxl_create.c | 2 +-
> tools/libxl/libxl_dom.c | 2 +-
> tools/libxl/libxl_internal.h | 12 +-
> tools/libxl/libxl_nocpuid.c | 2 +-
> tools/tests/cpu-policy/test-cpu-policy.c | 49 +++++-
> xen/arch/x86/cpuid.c | 23 +++
> xen/include/public/arch-x86/cpufeatureset.h | 4 +-
> xen/tools/gen-cpuid.py | 18 +--
> 13 files changed, 278 insertions(+), 160 deletions(-)
>
> --
> 2.11.0
>
^ permalink raw reply [flat|nested] 40+ messages in thread* RE: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
2020-06-15 17:04 ` [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Paul Durrant
@ 2020-06-17 12:46 ` Paul Durrant
0 siblings, 0 replies; 40+ messages in thread
From: Paul Durrant @ 2020-06-17 12:46 UTC (permalink / raw)
To: paul, 'Andrew Cooper', 'Xen-devel'
Cc: 'Ian Jackson', 'Jan Beulich', 'Wei Liu',
'Roger Pau Monné'
> -----Original Message-----
> From: Paul Durrant <xadimgnik@gmail.com>
> Sent: 15 June 2020 18:04
> To: 'Andrew Cooper' <andrew.cooper3@citrix.com>; 'Xen-devel' <xen-devel@lists.xenproject.org>
> Cc: 'Wei Liu' <wl@xen.org>; 'Jan Beulich' <JBeulich@suse.com>; 'Ian Jackson' <Ian.Jackson@citrix.com>;
> 'Roger Pau Monné' <roger.pau@citrix.com>
> Subject: RE: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
>
> > -----Original Message-----
> > From: Xen-devel <xen-devel-bounces@lists.xenproject.org> On Behalf Of Andrew Cooper
> > Sent: 15 June 2020 15:15
> > To: Xen-devel <xen-devel@lists.xenproject.org>
> > Cc: Wei Liu <wl@xen.org>; Paul Durrant <paul@xen.org>; Andrew Cooper <andrew.cooper3@citrix.com>;
> Jan
> > Beulich <JBeulich@suse.com>; Ian Jackson <Ian.Jackson@citrix.com>; Roger Pau Monné
> > <roger.pau@citrix.com>
> > Subject: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
> >
> > This is some work in light of IvyBridge not gaining microcode to combat SRBDS
> > / XSA-320. It is a mix of some work I'd planned for 4.15, and some patches
> > posted already and delayed due to dependence's I'd discovered after-the-fact.
> >
> > This provides a more user-friendly way of making IvyBridge safe by default
> > without encountering migration incompatibilities.
> >
> > In terms of functionality, it finishes the "fresh boot" vs "migrate/restore
> > from pre-4.14" split in the libxc CPUID logic, and uses this to let us safely
> > hide features by default without breaking the "divine what a guest may have
> > seen previously" logic on migrate.
> >
> > On top of that, we hide RDRAND by default to mitigate XSA-320.
> >
> > Additionally, take the opportunity of finally getting this logic working to
> > hide MPX by default (as posted previously), due to upcoming Intel timelines.
> >
> > Request for 4.14. The IvyBridge angle only became apparent after the public
> > embargo on Tue 9th. Otherwise, I would have made a concerted effort to get
> > this logic sorted sooner and/or part of XSA-320 itself.
> >
> > Strictly speaking, patches 1-4 aren't necessary, but without them the logic is
> > very confusing to follow, particularly the reasoning about the safely of later
> > changes. As it is a simple set of transforms, we're better with them than
> > without.
> >
> > Also, the MPX patch isn't related to the RDRAND issue, but I was planning to
> > get it into 4.14 already, until realising that the migration path was broken.
> > Now that the path is fixed for the RDRAND issue, include the MPX patch as it
> > pertains to future hardware compatibility (and would be backported to 4.14.1
> > if it misses 4.14.0).
> >
>
> Fair enough. Once the series has all the requisite maintainer acks then I'll release-ack it.
>
I believe all acks are now place so the series is...
Release-acked-by: Paul Durrant <paul@xen.org>
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
2020-06-15 14:15 [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Andrew Cooper
` (9 preceding siblings ...)
2020-06-15 17:04 ` [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge Paul Durrant
@ 2020-06-18 7:18 ` Jan Beulich
2020-06-18 9:37 ` Andrew Cooper
10 siblings, 1 reply; 40+ messages in thread
From: Jan Beulich @ 2020-06-18 7:18 UTC (permalink / raw)
To: Andrew Cooper
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 15.06.2020 16:15, Andrew Cooper wrote:
> This is some work in light of IvyBridge not gaining microcode to combat SRBDS
> / XSA-320. It is a mix of some work I'd planned for 4.15, and some patches
> posted already and delayed due to dependence's I'd discovered after-the-fact.
>
> This provides a more user-friendly way of making IvyBridge safe by default
> without encountering migration incompatibilities.
>
> In terms of functionality, it finishes the "fresh boot" vs "migrate/restore
> from pre-4.14" split in the libxc CPUID logic, and uses this to let us safely
> hide features by default without breaking the "divine what a guest may have
> seen previously" logic on migrate.
>
> On top of that, we hide RDRAND by default to mitigate XSA-320.
>
> Additionally, take the opportunity of finally getting this logic working to
> hide MPX by default (as posted previously), due to upcoming Intel timelines.
>
> Request for 4.14. The IvyBridge angle only became apparent after the public
> embargo on Tue 9th. Otherwise, I would have made a concerted effort to get
> this logic sorted sooner and/or part of XSA-320 itself.
>
> Strictly speaking, patches 1-4 aren't necessary, but without them the logic is
> very confusing to follow, particularly the reasoning about the safely of later
> changes. As it is a simple set of transforms, we're better with them than
> without.
>
> Also, the MPX patch isn't related to the RDRAND issue, but I was planning to
> get it into 4.14 already, until realising that the migration path was broken.
> Now that the path is fixed for the RDRAND issue, include the MPX patch as it
> pertains to future hardware compatibility (and would be backported to 4.14.1
> if it misses 4.14.0).
Just to be sure - it is my understanding that none of this can sensibly
be backported, even if it was nice for us to take care of the IvyBridge
situation on older trees as well.
Jan
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH for-4.14 0/9] XSA-320 follow for IvyBridge
2020-06-18 7:18 ` Jan Beulich
@ 2020-06-18 9:37 ` Andrew Cooper
0 siblings, 0 replies; 40+ messages in thread
From: Andrew Cooper @ 2020-06-18 9:37 UTC (permalink / raw)
To: Jan Beulich
Cc: Xen-devel, Ian Jackson, Paul Durrant, Wei Liu, Roger Pau Monné
On 18/06/2020 08:18, Jan Beulich wrote:
> On 15.06.2020 16:15, Andrew Cooper wrote:
>> This is some work in light of IvyBridge not gaining microcode to combat SRBDS
>> / XSA-320. It is a mix of some work I'd planned for 4.15, and some patches
>> posted already and delayed due to dependence's I'd discovered after-the-fact.
>>
>> This provides a more user-friendly way of making IvyBridge safe by default
>> without encountering migration incompatibilities.
>>
>> In terms of functionality, it finishes the "fresh boot" vs "migrate/restore
>> from pre-4.14" split in the libxc CPUID logic, and uses this to let us safely
>> hide features by default without breaking the "divine what a guest may have
>> seen previously" logic on migrate.
>>
>> On top of that, we hide RDRAND by default to mitigate XSA-320.
>>
>> Additionally, take the opportunity of finally getting this logic working to
>> hide MPX by default (as posted previously), due to upcoming Intel timelines.
>>
>> Request for 4.14. The IvyBridge angle only became apparent after the public
>> embargo on Tue 9th. Otherwise, I would have made a concerted effort to get
>> this logic sorted sooner and/or part of XSA-320 itself.
>>
>> Strictly speaking, patches 1-4 aren't necessary, but without them the logic is
>> very confusing to follow, particularly the reasoning about the safely of later
>> changes. As it is a simple set of transforms, we're better with them than
>> without.
>>
>> Also, the MPX patch isn't related to the RDRAND issue, but I was planning to
>> get it into 4.14 already, until realising that the migration path was broken.
>> Now that the path is fixed for the RDRAND issue, include the MPX patch as it
>> pertains to future hardware compatibility (and would be backported to 4.14.1
>> if it misses 4.14.0).
> Just to be sure - it is my understanding that none of this can sensibly
> be backported, even if it was nice for us to take care of the IvyBridge
> situation on older trees as well.
Correct. Much as I'd like this to be backported, it depends on
approximately all the toolstack work I got complete in 4.14.
The changes to xc_apply_cpuid_policy() are only safe because the
behaviour of 4.13 is known (and fixed), and that all versions of Xen
we've made "breaking" default changes have the boot CPUID settings
properly in the migrate stream.
~Andrew
^ permalink raw reply [flat|nested] 40+ messages in thread