* [Xen-devel] [PATCH v11 0/7] improve late microcode loading
@ 2019-09-26 13:53 Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
` (6 more replies)
0 siblings, 7 replies; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Boris Ostrovsky, Chao Gao, Brian Woods, Suravee Suthikulpanit,
Roger Pau Monné
Changes in v11:
- reject late ucode loading if any core is parked
- correct the usage of microcode_mutex in microcode_update_cpu()
- extend 'ucode' boot option to enable/disable ucode loading in NMI
- drop the last two patches of v10 (about BDF90 and wbinvd, I haven't
get an answer to opens yet).
- other minor changes are described in each patch's change log
Regarding changes to AMD side, I didn't do any test for them due to
lack of hardware.
The intention of this series is to make the late microcode loading
more reliable by rendezvousing all cpus in stop_machine context.
This idea comes from Ashok. I am porting his linux patch to Xen
(see patch 4 more details).
This series includes below changes:
1. Patch 1-3: cleanup and preparation for synchronizing ucode loading
2. Patch 4: synchronize late microcode loading
3. Patch 5: support parallel microcodes update on different cores
4. Patch 6: rendezvous CPUs in NMI handler and load ucode
5. Patch 7: reject late ucode loading if any core is parked
Currently, late microcode loading does a lot of things including
parsing microcode blob, checking the signature/revision and performing
update. Putting all of them into stop_machine context is a bad idea
because of complexity (one issue I observed is memory allocation
triggered one assertion in stop_machine context). To simplify the
load process, parsing microcode is moved out of the load process.
Remaining parts of load process is put to stop_machine context.
Previous change log:
Major changes in version 10:
- add back the patch to call wbinvd() conditionally
- add a patch to disable late loading due to BDF90
- rendezvous CPUs in NMI handler and load ucode. But provide an option
to disable this behavior.
- avoid the call of self_nmi() on the control thread because it may
trigger the unknown_nmi_error() in do_nmi().
- ensure ->start_update is called during system resuming from
suspension
Changes in version 9:
- add Jan's Reviewed-by
- rendevzous threads in NMI handler to disable NMI. Note that NMI can
be served as usual on threads that are chosen to initiate ucode loading
on each core.
- avoid unnecessary memory allocation or copy when creating a microcode
patch (patch 12)
- rework patch 1 to avoid microcode_update_match() being used to
compare two arbitrary updates.
- call .end_update in early loading path.
Changes in version 8:
- block #NMI handling during microcode loading (Patch 16)
- Don't assume that all CPUs in the system have loaded a same ucode.
So when parsing a blob, we attempt to save a patch as long as it matches
with current cpu signature regardless of the revision of the patch.
And also for loading, we only require the patch to be loaded isn't old
than the cached one.
- store an update after the first successful loading on a CPU
- remove the patch that calls wbinvd() unconditionally before microcode
loading. It is under internal discussion.
- divide two big patches into several patches to improve readability.
Changes in version 7:
- cache one microcode update rather than a list of it. Assuming that all CPUs
(including those will be plugged in later) in the system have the same
signature, one update matches with one CPU should match with others. Thus, one
update is enough for microcode updating during CPU hot-plug and resuming.
- To handle load failure, microcode update is cached after it is applied to
avoid a broken update overriding a validated one. Unvalidated microcode updates
are passed by arguments rather than another global variable, where this series
slightly differs from Roger's suggestion in:
https://lists.xen.org/archives/html/xen-devel/2019-03/msg00776.html
- incorporate Sergey's patch (patch 10) to fix a bug: we maintain a variable
to reflect current microcode revision. But in some cases, this variable isn't
initialized during system boot time, which results in falsely reporting that
processor is susceptible to some known vulnerabilities.
- fix issues reported by Sergey:
https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg00901.html
- Responses to Sergey/Roger/Wei/Ashok's other comments.
Major changes in version 6:
- run wbinvd before updating microcode (patch 10)
- add an userspace tool for late microcode update (patch 1)
- scale time to wait by the number of remaining CPUs to respond
- remove 'cpu' parameters from some related callbacks and functins
- save an ucode patch only if its supported CPU is allowed to mix with
current cpu.
Changes in version 5:
- support parallel microcode updates for all cores (see patch 8)
- Address Roger's comments on the last version.
Chao Gao (7):
microcode: split out apply_microcode() from cpu_request_microcode()
microcode: unify ucode loading during system bootup and resuming
microcode: reduce memory allocation and copy when creating a patch
x86/microcode: Synchronize late microcode loading
microcode: remove microcode_update_lock
microcode: rendezvous CPUs in NMI handler and load ucode
microcode: reject late ucode loading if any core is parked
docs/misc/xen-command-line.pandoc | 6 +-
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/microcode.c | 638 ++++++++++++++++++++++++++++++++------
xen/arch/x86/microcode_amd.c | 132 ++++----
xen/arch/x86/microcode_intel.c | 99 +++---
xen/arch/x86/smpboot.c | 5 +-
xen/arch/x86/traps.c | 6 +-
xen/include/asm-x86/microcode.h | 5 +-
xen/include/asm-x86/nmi.h | 3 +
xen/include/asm-x86/processor.h | 5 +-
10 files changed, 658 insertions(+), 243 deletions(-)
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode()
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-27 9:38 ` Jan Beulich
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 2/7] microcode: unify ucode loading during system bootup and resuming Chao Gao
` (5 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
During late microcode loading, apply_microcode() is invoked in
cpu_request_microcode(). To make late microcode update more reliable,
we want to put the apply_microcode() into stop_machine context. So
we split out it from cpu_request_microcode(). In general, for both
early loading on BSP and late loading, cpu_request_microcode() is
called first to get the matching microcode update contained by
the blob and then apply_microcode() is invoked explicitly on each
cpu in common code.
Given that all CPUs are supposed to have the same signature, parsing
microcode only needs to be done once. So cpu_request_microcode() is
also moved out of microcode_update_cpu().
In some cases (e.g. a broken bios), the system may have multiple
revisions of microcode update. So we would try to load a microcode
update as long as it covers current cpu. And if a cpu loads this patch
successfully, the patch would be stored into the patch cache.
Note that calling ->apply_microcode() itself doesn't require any
lock being held. But the parameter passed to it may be protected
by some locks. E.g. microcode_update_cpu() acquires microcode_mutex
to avoid microcode_cache being updated by others.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v11:
- drop Roger's RB.
- acquire microcode_mutex before checking whether microcode_cache is
NULL
- ignore -EINVAL which indicates a equal/newer ucode is already loaded.
- free 'buffer' earlier to avoid goto clauses in microcode_update()
Changes in v10:
- make microcode_update_cache static
- raise an error if loading ucode failed with -EIO
- ensure end_update_percpu() is called following a successful call of
start_update()
Changes in v9:
- remove the calling of ->compare_patch in microcode_update_cpu().
- drop "microcode_" prefix for static function - microcode_parse_blob().
- rebase and fix conflict
Changes in v8:
- divide the original patch into three patches to improve readability
- load an update on each cpu as long as the update covers current cpu
- store an update after the first successful loading on a CPU
- Make sure the current CPU (especially pf value) is covered
by updates.
changes in v7:
- to handle load failure, unvalidated patches won't be cached. They
are passed as function arguments. So if update failed, we needn't
any cleanup to microcode cache.
---
xen/arch/x86/microcode.c | 173 +++++++++++++++++++++++++++-------------
xen/arch/x86/microcode_amd.c | 38 ++++-----
xen/arch/x86/microcode_intel.c | 66 +++++++--------
xen/include/asm-x86/microcode.h | 5 +-
4 files changed, 172 insertions(+), 110 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index b44e4d7..3ea2a6e 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -189,12 +189,19 @@ static DEFINE_SPINLOCK(microcode_mutex);
DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
-struct microcode_info {
- unsigned int cpu;
- uint32_t buffer_size;
- int error;
- char buffer[1];
-};
+/*
+ * Return a patch that covers current CPU. If there are multiple patches,
+ * return the one with the highest revision number. Return error If no
+ * patch is found and an error occurs during the parsing process. Otherwise
+ * return NULL.
+ */
+static struct microcode_patch *parse_blob(const char *buf, size_t len)
+{
+ if ( likely(!microcode_ops->collect_cpu_info(&this_cpu(cpu_sig))) )
+ return microcode_ops->cpu_request_microcode(buf, len);
+
+ return NULL;
+}
int microcode_resume_cpu(void)
{
@@ -220,15 +227,8 @@ void microcode_free_patch(struct microcode_patch *microcode_patch)
xfree(microcode_patch);
}
-const struct microcode_patch *microcode_get_cache(void)
-{
- ASSERT(spin_is_locked(µcode_mutex));
-
- return microcode_cache;
-}
-
/* Return true if cache gets updated. Otherwise, return false */
-bool microcode_update_cache(struct microcode_patch *patch)
+static bool microcode_update_cache(struct microcode_patch *patch)
{
ASSERT(spin_is_locked(µcode_mutex));
@@ -249,49 +249,82 @@ bool microcode_update_cache(struct microcode_patch *patch)
return true;
}
-static int microcode_update_cpu(const void *buf, size_t size)
+/*
+ * Load a microcode update to current CPU.
+ *
+ * If no patch is provided, the cached patch will be loaded. Microcode update
+ * during APs bringup and CPU resuming falls into this case.
+ */
+static int microcode_update_cpu(const struct microcode_patch *patch)
{
- int err;
- unsigned int cpu = smp_processor_id();
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+ int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
- spin_lock(µcode_mutex);
+ if ( unlikely(err) )
+ return err;
- err = microcode_ops->collect_cpu_info(sig);
- if ( likely(!err) )
- err = microcode_ops->cpu_request_microcode(buf, size);
+ spin_lock(µcode_mutex);
+ if ( patch )
+ err = microcode_ops->apply_microcode(patch);
+ else if ( microcode_cache )
+ {
+ err = microcode_ops->apply_microcode(microcode_cache);
+ if ( err == -EIO )
+ {
+ microcode_free_patch(microcode_cache);
+ microcode_cache = NULL;
+ }
+ }
+ else
+ /* No patch to update */
+ err = -ENOENT;
spin_unlock(µcode_mutex);
return err;
}
-static long do_microcode_update(void *_info)
+static long do_microcode_update(void *patch)
{
- struct microcode_info *info = _info;
- int error;
-
- BUG_ON(info->cpu != smp_processor_id());
+ unsigned int cpu;
+ int ret = microcode_update_cpu(patch);
- error = microcode_update_cpu(info->buffer, info->buffer_size);
- if ( error )
- info->error = error;
+ /* Store the patch after a successful loading */
+ if ( !ret && patch )
+ {
+ spin_lock(µcode_mutex);
+ microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+ patch = NULL;
+ }
if ( microcode_ops->end_update_percpu )
microcode_ops->end_update_percpu();
- info->cpu = cpumask_next(info->cpu, &cpu_online_map);
- if ( info->cpu < nr_cpu_ids )
- return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+ /*
+ * Each thread tries to load ucode. Only the first thread of a core
+ * would succeed while other threads would encounter -EINVAL which
+ * indicates current ucode revision is equal to or newer than the
+ * given patch. It is actually expected; so ignore this error.
+ */
+ if ( ret == -EINVAL )
+ ret = 0;
+
+ cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
+ if ( cpu < nr_cpu_ids )
+ return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?:
+ ret;
+
+ /* Free the patch if no CPU has loaded it successfully. */
+ if ( patch )
+ microcode_free_patch(patch);
- error = info->error;
- xfree(info);
- return error;
+ return ret;
}
int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
int ret;
- struct microcode_info *info;
+ void *buffer;
+ struct microcode_patch *patch;
if ( len != (uint32_t)len )
return -E2BIG;
@@ -299,32 +332,41 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
if ( microcode_ops == NULL )
return -EINVAL;
- info = xmalloc_bytes(sizeof(*info) + len);
- if ( info == NULL )
+ buffer = xmalloc_bytes(len);
+ if ( !buffer )
return -ENOMEM;
- ret = copy_from_guest(info->buffer, buf, len);
- if ( ret != 0 )
+ ret = copy_from_guest(buffer, buf, len);
+ if ( ret )
+ {
+ xfree(buffer);
+ return -EFAULT;
+ }
+
+ patch = parse_blob(buffer, len);
+ xfree(buffer);
+ if ( IS_ERR(patch) )
{
- xfree(info);
+ ret = PTR_ERR(patch);
+ printk(XENLOG_WARNING "Parsing microcode blob error %d\n", ret);
return ret;
}
- info->buffer_size = len;
- info->error = 0;
- info->cpu = cpumask_first(&cpu_online_map);
+ if ( !patch )
+ return -ENOENT;
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
if ( ret != 0 )
{
- xfree(info);
+ microcode_free_patch(patch);
return ret;
}
}
- return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+ return continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
+ do_microcode_update, patch);
}
static int __init microcode_init(void)
@@ -371,23 +413,42 @@ int __init early_microcode_update_cpu(bool start_update)
microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
- if ( data )
+ if ( !data )
+ return -ENOMEM;
+
+ if ( start_update )
{
- if ( start_update && microcode_ops->start_update )
+ struct microcode_patch *patch;
+
+ patch = parse_blob(data, len);
+ if ( IS_ERR(patch) )
+ {
+ printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
+ PTR_ERR(patch));
+ return PTR_ERR(patch);
+ }
+
+ if ( !patch )
+ return -ENOENT;
+
+ spin_lock(µcode_mutex);
+ rc = microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+ ASSERT(rc);
+
+ if ( microcode_ops->start_update )
rc = microcode_ops->start_update();
if ( rc )
return rc;
+ }
- rc = microcode_update_cpu(data, len);
+ rc = microcode_update_cpu(NULL);
- if ( microcode_ops->end_update_percpu )
- microcode_ops->end_update_percpu();
+ if ( microcode_ops->end_update_percpu )
+ microcode_ops->end_update_percpu();
- return rc;
- }
- else
- return -ENOMEM;
+ return rc;
}
int __init early_microcode_init(void)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 8e4cdab..0199308 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -455,9 +455,11 @@ static bool_t check_final_patch_levels(unsigned int cpu)
return 0;
}
-static int cpu_request_microcode(const void *buf, size_t bufsize)
+static struct microcode_patch *cpu_request_microcode(const void *buf,
+ size_t bufsize)
{
struct microcode_amd *mc_amd;
+ struct microcode_patch *patch = NULL;
size_t offset = 0;
int error = 0;
unsigned int current_cpu_id;
@@ -556,19 +558,22 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
break;
}
- /* Update cache if this patch covers current CPU */
- if ( microcode_fits(new_patch->mc_amd) != MIS_UCODE )
- microcode_update_cache(new_patch);
- else
- microcode_free_patch(new_patch);
-
- if ( match_cpu(microcode_get_cache()) )
+ /*
+ * If the new patch covers current CPU, compare patches and store the
+ * one with higher revision.
+ */
+ if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
+ (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
{
- error = apply_microcode(microcode_get_cache());
- if ( error )
- break;
+ struct microcode_patch *tmp = patch;
+
+ patch = new_patch;
+ new_patch = tmp;
}
+ if ( new_patch )
+ microcode_free_patch(new_patch);
+
if ( offset >= bufsize )
break;
@@ -599,13 +604,10 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
free_patch(mc_amd);
out:
- /*
- * In some cases we may return an error even if processor's microcode has
- * been updated. For example, the first patch in a container file is loaded
- * successfully but subsequent container file processing encounters a
- * failure.
- */
- return error;
+ if ( error && !patch )
+ patch = ERR_PTR(error);
+
+ return patch;
}
#ifdef CONFIG_HVM
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 23197ca..1b3ac93 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -285,14 +285,9 @@ static enum microcode_match_result compare_patch(
: OLD_UCODE;
}
-/*
- * return 0 - no update found
- * return 1 - found update
- * return < 0 - error
- */
-static int get_matching_microcode(const void *mc)
+static struct microcode_patch *alloc_microcode_patch(
+ const struct microcode_header_intel *mc_header)
{
- const struct microcode_header_intel *mc_header = mc;
unsigned long total_size = get_totalsize(mc_header);
void *new_mc = xmalloc_bytes(total_size);
struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
@@ -301,25 +296,12 @@ static int get_matching_microcode(const void *mc)
{
xfree(new_patch);
xfree(new_mc);
- return -ENOMEM;
+ return ERR_PTR(-ENOMEM);
}
- memcpy(new_mc, mc, total_size);
+ memcpy(new_mc, mc_header, total_size);
new_patch->mc_intel = new_mc;
- /* Make sure that this patch covers current CPU */
- if ( microcode_update_match(mc) == MIS_UCODE )
- {
- microcode_free_patch(new_patch);
- return 0;
- }
-
- microcode_update_cache(new_patch);
-
- pr_debug("microcode: CPU%d found a matching microcode update with"
- " version %#x (current=%#x)\n",
- smp_processor_id(), mc_header->rev, this_cpu(cpu_sig).rev);
-
- return 1;
+ return new_patch;
}
static int apply_microcode(const struct microcode_patch *patch)
@@ -398,26 +380,44 @@ static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
return offset + total_size;
}
-static int cpu_request_microcode(const void *buf, size_t size)
+static struct microcode_patch *cpu_request_microcode(const void *buf,
+ size_t size)
{
long offset = 0;
int error = 0;
void *mc;
+ struct microcode_patch *patch = NULL;
while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
{
+ struct microcode_patch *new_patch;
+
error = microcode_sanity_check(mc);
if ( error )
break;
- error = get_matching_microcode(mc);
- if ( error < 0 )
+
+ new_patch = alloc_microcode_patch(mc);
+ if ( IS_ERR(new_patch) )
+ {
+ error = PTR_ERR(new_patch);
break;
+ }
+
/*
- * It's possible the data file has multiple matching ucode,
- * lets keep searching till the latest version
+ * If the new patch covers current CPU, compare patches and store the
+ * one with higher revision.
*/
- if ( error == 1 )
- error = 0;
+ if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) &&
+ (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
+ {
+ struct microcode_patch *tmp = patch;
+
+ patch = new_patch;
+ new_patch = tmp;
+ }
+
+ if ( new_patch )
+ microcode_free_patch(new_patch);
xfree(mc);
}
@@ -426,10 +426,10 @@ static int cpu_request_microcode(const void *buf, size_t size)
if ( offset < 0 )
error = offset;
- if ( !error && match_cpu(microcode_get_cache()) )
- error = apply_microcode(microcode_get_cache());
+ if ( error && !patch )
+ patch = ERR_PTR(error);
- return error;
+ return patch;
}
static const struct microcode_ops microcode_intel_ops = {
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 02feb09..7d5a1f8 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -20,7 +20,8 @@ struct microcode_patch {
};
struct microcode_ops {
- int (*cpu_request_microcode)(const void *buf, size_t size);
+ struct microcode_patch *(*cpu_request_microcode)(const void *buf,
+ size_t size);
int (*collect_cpu_info)(struct cpu_signature *csig);
int (*apply_microcode)(const struct microcode_patch *patch);
int (*start_update)(void);
@@ -40,8 +41,6 @@ struct cpu_signature {
DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
extern const struct microcode_ops *microcode_ops;
-const struct microcode_patch *microcode_get_cache(void);
-bool microcode_update_cache(struct microcode_patch *patch);
void microcode_free_patch(struct microcode_patch *patch);
#endif /* ASM_X86__MICROCODE_H */
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 2/7] microcode: unify ucode loading during system bootup and resuming
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 3/7] microcode: reduce memory allocation and copy when creating a patch Chao Gao
` (4 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
During system bootup and resuming, CPUs just load the cached ucode.
So one unified function microcode_update_one() is introduced. It
takes a boolean to indicate whether ->start_update should be called.
Since early_microcode_update_cpu() is only called on BSP (APs call
the unified function), start_update is always true and so remove
this parameter.
There is a functional change: ->start_update is called on BSP and
->end_update_percpu is called during system resuming. They are not
invoked by previous microcode_resume_cpu().
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v10:
- call ->start_update for system resume from suspension
Changes in v9:
- return -EOPNOTSUPP rather than 0 if microcode_ops is NULL in
microcode_update_one()
- rebase and fix conflicts.
Changes in v8:
- split out from the previous patch
---
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/microcode.c | 91 +++++++++++++++++++----------------------
xen/arch/x86/smpboot.c | 5 +--
xen/include/asm-x86/processor.h | 4 +-
4 files changed, 45 insertions(+), 57 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 269b140..01e6aec 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -278,7 +278,7 @@ static int enter_state(u32 state)
console_end_sync();
- microcode_resume_cpu();
+ microcode_update_one(true);
if ( !recheck_cpu_features(0) )
panic("Missing previously available feature(s)\n");
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 3ea2a6e..9c0e5c4 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -203,24 +203,6 @@ static struct microcode_patch *parse_blob(const char *buf, size_t len)
return NULL;
}
-int microcode_resume_cpu(void)
-{
- int err;
- struct cpu_signature *sig = &this_cpu(cpu_sig);
-
- if ( !microcode_ops )
- return 0;
-
- spin_lock(µcode_mutex);
-
- err = microcode_ops->collect_cpu_info(sig);
- if ( likely(!err) )
- err = microcode_ops->apply_microcode(microcode_cache);
- spin_unlock(µcode_mutex);
-
- return err;
-}
-
void microcode_free_patch(struct microcode_patch *microcode_patch)
{
microcode_ops->free_patch(microcode_patch->mc);
@@ -391,11 +373,38 @@ static int __init microcode_init(void)
}
__initcall(microcode_init);
-int __init early_microcode_update_cpu(bool start_update)
+/* Load a cached update to current cpu */
+int microcode_update_one(bool start_update)
+{
+ int err;
+
+ if ( !microcode_ops )
+ return -EOPNOTSUPP;
+
+ microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
+
+ if ( start_update && microcode_ops->start_update )
+ {
+ err = microcode_ops->start_update();
+ if ( err )
+ return err;
+ }
+
+ err = microcode_update_cpu(NULL);
+
+ if ( microcode_ops->end_update_percpu )
+ microcode_ops->end_update_percpu();
+
+ return err;
+}
+
+/* BSP calls this function to parse ucode blob and then apply an update. */
+int __init early_microcode_update_cpu(void)
{
int rc = 0;
void *data = NULL;
size_t len;
+ struct microcode_patch *patch;
if ( !microcode_ops )
return -ENOSYS;
@@ -411,44 +420,26 @@ int __init early_microcode_update_cpu(bool start_update)
data = bootstrap_map(&ucode_mod);
}
- microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
-
if ( !data )
return -ENOMEM;
- if ( start_update )
+ patch = parse_blob(data, len);
+ if ( IS_ERR(patch) )
{
- struct microcode_patch *patch;
-
- patch = parse_blob(data, len);
- if ( IS_ERR(patch) )
- {
- printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
- PTR_ERR(patch));
- return PTR_ERR(patch);
- }
-
- if ( !patch )
- return -ENOENT;
-
- spin_lock(µcode_mutex);
- rc = microcode_update_cache(patch);
- spin_unlock(µcode_mutex);
- ASSERT(rc);
-
- if ( microcode_ops->start_update )
- rc = microcode_ops->start_update();
-
- if ( rc )
- return rc;
+ printk(XENLOG_WARNING "Parsing microcode blob error %ld\n",
+ PTR_ERR(patch));
+ return PTR_ERR(patch);
}
- rc = microcode_update_cpu(NULL);
+ if ( !patch )
+ return -ENOENT;
- if ( microcode_ops->end_update_percpu )
- microcode_ops->end_update_percpu();
+ spin_lock(µcode_mutex);
+ rc = microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+ ASSERT(rc);
- return rc;
+ return microcode_update_one(true);
}
int __init early_microcode_init(void)
@@ -468,7 +459,7 @@ int __init early_microcode_init(void)
microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
if ( ucode_mod.mod_end || ucode_blob.size )
- rc = early_microcode_update_cpu(true);
+ rc = early_microcode_update_cpu();
}
return rc;
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 73a1afc..179f6b6 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -360,10 +360,7 @@ void start_secondary(void *unused)
initialize_cpu_data(cpu);
- if ( system_state <= SYS_STATE_smp_boot )
- early_microcode_update_cpu(false);
- else
- microcode_resume_cpu();
+ microcode_update_one(false);
/*
* If MSR_SPEC_CTRL is available, apply Xen's default setting and discard
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index a673372..c92956f 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -569,9 +569,9 @@ int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val);
void microcode_set_module(unsigned int);
int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
-int microcode_resume_cpu(void);
-int early_microcode_update_cpu(bool start_update);
+int early_microcode_update_cpu(void);
int early_microcode_init(void);
+int microcode_update_one(bool start_update);
int microcode_init_intel(void);
int microcode_init_amd(void);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 3/7] microcode: reduce memory allocation and copy when creating a patch
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 2/7] microcode: unify ucode loading during system bootup and resuming Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading Chao Gao
` (3 subsequent siblings)
6 siblings, 0 replies; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
To create a microcode patch from a vendor-specific update,
allocate_microcode_patch() copied everything from the update.
It is not efficient. Essentially, we just need to go through
ucodes in the blob, find the one with the newest revision and
install it into the microcode_patch. In the process, buffers
like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel
side) can be reused. microcode_patch now is allocated after
it is sure that there is a matching ucode.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v11:
- correct parameter type issues of get_next_ucode_from_buffer
Changes in v10:
- avoid unnecessary type casting
* introduce compare_header on AMD side
* specify the type of the first parameter of get_next_ucode_from_buffer()
on Intel side
Changes in v9:
- new
---
xen/arch/x86/microcode_amd.c | 112 +++++++++++++++++------------------------
xen/arch/x86/microcode_intel.c | 67 +++++++++---------------
2 files changed, 69 insertions(+), 110 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 0199308..9a8f179 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -194,36 +194,6 @@ static bool match_cpu(const struct microcode_patch *patch)
return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
}
-static struct microcode_patch *alloc_microcode_patch(
- const struct microcode_amd *mc_amd)
-{
- struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch);
- struct microcode_amd *cache = xmalloc(struct microcode_amd);
- void *mpb = xmalloc_bytes(mc_amd->mpb_size);
- struct equiv_cpu_entry *equiv_cpu_table =
- xmalloc_bytes(mc_amd->equiv_cpu_table_size);
-
- if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table )
- {
- xfree(microcode_patch);
- xfree(cache);
- xfree(mpb);
- xfree(equiv_cpu_table);
- return ERR_PTR(-ENOMEM);
- }
-
- memcpy(mpb, mc_amd->mpb, mc_amd->mpb_size);
- cache->mpb = mpb;
- cache->mpb_size = mc_amd->mpb_size;
- memcpy(equiv_cpu_table, mc_amd->equiv_cpu_table,
- mc_amd->equiv_cpu_table_size);
- cache->equiv_cpu_table = equiv_cpu_table;
- cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size;
- microcode_patch->mc_amd = cache;
-
- return microcode_patch;
-}
-
static void free_patch(void *mc)
{
struct microcode_amd *mc_amd = mc;
@@ -236,6 +206,17 @@ static void free_patch(void *mc)
}
}
+static enum microcode_match_result compare_header(
+ const struct microcode_header_amd *new_header,
+ const struct microcode_header_amd *old_header)
+{
+ if ( new_header->processor_rev_id == old_header->processor_rev_id )
+ return (new_header->patch_id > old_header->patch_id) ? NEW_UCODE
+ : OLD_UCODE;
+
+ return MIS_UCODE;
+}
+
static enum microcode_match_result compare_patch(
const struct microcode_patch *new, const struct microcode_patch *old)
{
@@ -246,11 +227,7 @@ static enum microcode_match_result compare_patch(
ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
ASSERT(microcode_fits(new->mc_amd) != MIS_UCODE);
- if ( new_header->processor_rev_id == old_header->processor_rev_id )
- return (new_header->patch_id > old_header->patch_id) ?
- NEW_UCODE : OLD_UCODE;
-
- return MIS_UCODE;
+ return compare_header(new_header, old_header);
}
static int apply_microcode(const struct microcode_patch *patch)
@@ -328,18 +305,10 @@ static int get_ucode_from_buffer_amd(
return -EINVAL;
}
- if ( mc_amd->mpb_size < mpbuf->len )
- {
- if ( mc_amd->mpb )
- {
- xfree(mc_amd->mpb);
- mc_amd->mpb_size = 0;
- }
- mc_amd->mpb = xmalloc_bytes(mpbuf->len);
- if ( mc_amd->mpb == NULL )
- return -ENOMEM;
- mc_amd->mpb_size = mpbuf->len;
- }
+ mc_amd->mpb = xmalloc_bytes(mpbuf->len);
+ if ( !mc_amd->mpb )
+ return -ENOMEM;
+ mc_amd->mpb_size = mpbuf->len;
memcpy(mc_amd->mpb, mpbuf->data, mpbuf->len);
pr_debug("microcode: CPU%d size %zu, block size %u offset %zu equivID %#x rev %#x\n",
@@ -459,8 +428,9 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
size_t bufsize)
{
struct microcode_amd *mc_amd;
+ struct microcode_header_amd *saved = NULL;
struct microcode_patch *patch = NULL;
- size_t offset = 0;
+ size_t offset = 0, saved_size = 0;
int error = 0;
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
@@ -550,29 +520,22 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
&offset)) == 0 )
{
- struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd);
-
- if ( IS_ERR(new_patch) )
- {
- error = PTR_ERR(new_patch);
- break;
- }
-
/*
- * If the new patch covers current CPU, compare patches and store the
+ * If the new ucode covers current CPU, compare ucodes and store the
* one with higher revision.
*/
- if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
- (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
+ if ( (microcode_fits(mc_amd) != MIS_UCODE) &&
+ (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
{
- struct microcode_patch *tmp = patch;
-
- patch = new_patch;
- new_patch = tmp;
+ xfree(saved);
+ saved = mc_amd->mpb;
+ saved_size = mc_amd->mpb_size;
+ }
+ else
+ {
+ xfree(mc_amd->mpb);
+ mc_amd->mpb = NULL;
}
-
- if ( new_patch )
- microcode_free_patch(new_patch);
if ( offset >= bufsize )
break;
@@ -601,7 +564,22 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
*(const uint32_t *)(buf + offset) == UCODE_MAGIC )
break;
}
- free_patch(mc_amd);
+
+ if ( saved )
+ {
+ mc_amd->mpb = saved;
+ mc_amd->mpb_size = saved_size;
+ patch = xmalloc(struct microcode_patch);
+ if ( patch )
+ patch->mc_amd = mc_amd;
+ else
+ {
+ free_patch(mc_amd);
+ error = -ENOMEM;
+ }
+ }
+ else
+ free_patch(mc_amd);
out:
if ( error && !patch )
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 1b3ac93..c083e17 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -285,25 +285,6 @@ static enum microcode_match_result compare_patch(
: OLD_UCODE;
}
-static struct microcode_patch *alloc_microcode_patch(
- const struct microcode_header_intel *mc_header)
-{
- unsigned long total_size = get_totalsize(mc_header);
- void *new_mc = xmalloc_bytes(total_size);
- struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
-
- if ( !new_patch || !new_mc )
- {
- xfree(new_patch);
- xfree(new_mc);
- return ERR_PTR(-ENOMEM);
- }
- memcpy(new_mc, mc_header, total_size);
- new_patch->mc_intel = new_mc;
-
- return new_patch;
-}
-
static int apply_microcode(const struct microcode_patch *patch)
{
unsigned long flags;
@@ -352,8 +333,9 @@ static int apply_microcode(const struct microcode_patch *patch)
return 0;
}
-static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
- unsigned long size, long offset)
+static long get_next_ucode_from_buffer(struct microcode_intel **mc,
+ const uint8_t *buf, unsigned long size,
+ unsigned long offset)
{
struct microcode_header_intel *mc_header;
unsigned long total_size;
@@ -385,47 +367,46 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
{
long offset = 0;
int error = 0;
- void *mc;
+ struct microcode_intel *mc, *saved = NULL;
struct microcode_patch *patch = NULL;
while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
{
- struct microcode_patch *new_patch;
-
error = microcode_sanity_check(mc);
if ( error )
- break;
-
- new_patch = alloc_microcode_patch(mc);
- if ( IS_ERR(new_patch) )
{
- error = PTR_ERR(new_patch);
+ xfree(mc);
break;
}
/*
- * If the new patch covers current CPU, compare patches and store the
+ * If the new update covers current CPU, compare updates and store the
* one with higher revision.
*/
- if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) &&
- (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
+ if ( (microcode_update_match(&mc->hdr) != MIS_UCODE) &&
+ (!saved || (mc->hdr.rev > saved->hdr.rev)) )
{
- struct microcode_patch *tmp = patch;
-
- patch = new_patch;
- new_patch = tmp;
+ xfree(saved);
+ saved = mc;
}
-
- if ( new_patch )
- microcode_free_patch(new_patch);
-
- xfree(mc);
+ else
+ xfree(mc);
}
- if ( offset > 0 )
- xfree(mc);
if ( offset < 0 )
error = offset;
+ if ( saved )
+ {
+ patch = xmalloc(struct microcode_patch);
+ if ( patch )
+ patch->mc_intel = saved;
+ else
+ {
+ xfree(saved);
+ error = -ENOMEM;
+ }
+ }
+
if ( error && !patch )
patch = ERR_PTR(error);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
` (2 preceding siblings ...)
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 3/7] microcode: reduce memory allocation and copy when creating a patch Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-27 9:51 ` Jan Beulich
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 5/7] microcode: remove microcode_update_lock Chao Gao
` (2 subsequent siblings)
6 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Kevin Tian, Borislav Petkov, Ashok Raj, Wei Liu,
Jun Nakajima, Andrew Cooper, Jan Beulich, Thomas Gleixner,
Chao Gao, Roger Pau Monné
This patch ports microcode improvement patches from linux kernel.
Before you read any further: the early loading method is still the
preferred one and you should always do that. The following patch is
improving the late loading mechanism for long running jobs and cloud use
cases.
Gather all cores and serialize the microcode update on them by doing it
one-by-one to make the late update process as reliable as possible and
avoid potential issues caused by the microcode update.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Tested-by: Chao Gao <chao.gao@intel.com>
[linux commit: a5321aec6412b20b5ad15db2d6b916c05349dbff]
[linux commit: bb8c13d61a629276a162c1d2b1a20a815cbcfbb7]
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes in v11:
- Use the sample code of wait_for_state() provided by Jan
- make wait_cpu_call{in,out} take unsigned int to avoid type casting
- do assignment in while clause in control_thread_fn() to eliminate
duplication.
Changes in v10:
- introduce wait_for_state() and set_state() helper functions
- make wait_for_condition() return bool and take const void *
- disable/enable watchdog in control thread
- rename "master" and "slave" thread to "primary" and "secondary"
Changes in v9:
- log __buildin_return_address(0) when timeout
- divide CPUs into three logical sets and they will call different
functions during ucode loading. The 'control thread' is chosen to
coordinate ucode loading on all CPUs. Since only control thread would
set 'loading_state', we can get rid of 'cmpxchg' stuff in v8.
- s/rep_nop/cpu_relax
- each thread updates its revision number itself
- add XENLOG_ERR prefix for each line of multi-line log messages
Changes in v8:
- to support blocking #NMI handling during loading ucode
* introduce a flag, 'loading_state', to mark the start or end of
ucode loading.
* use a bitmap for cpu callin since if cpu may stay in #NMI handling,
there are two places for a cpu to call in. bitmap won't be counted
twice.
* don't wait for all CPUs callout, just wait for CPUs that perform the
update. We have to do this because some threads may be stuck in NMI
handling (where cannot reach the rendezvous).
- emit a warning if the system stays in stop_machine context for more
than 1s
- comment that rdtsc is fine while loading an update
- use cmpxchg() to avoid panic being called on multiple CPUs
- Propagate revision number to other threads
- refine comments and prompt messages
Changes in v7:
- Check whether 'timeout' is 0 rather than "<=0" since it is unsigned int.
- reword the comment above microcode_update_cpu() to clearly state that
one thread per core should do the update.
---
xen/arch/x86/microcode.c | 297 ++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 267 insertions(+), 30 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 9c0e5c4..6c23879 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -30,18 +30,52 @@
#include <xen/smp.h>
#include <xen/softirq.h>
#include <xen/spinlock.h>
+#include <xen/stop_machine.h>
#include <xen/tasklet.h>
#include <xen/guest_access.h>
#include <xen/earlycpio.h>
+#include <xen/watchdog.h>
+#include <asm/delay.h>
#include <asm/msr.h>
#include <asm/processor.h>
#include <asm/setup.h>
#include <asm/microcode.h>
+/*
+ * Before performing a late microcode update on any thread, we
+ * rendezvous all cpus in stop_machine context. The timeout for
+ * waiting for cpu rendezvous is 30ms. It is the timeout used by
+ * live patching
+ */
+#define MICROCODE_CALLIN_TIMEOUT_US 30000
+
+/*
+ * Timeout for each thread to complete update is set to 1s. It is a
+ * conservative choice considering all possible interference.
+ */
+#define MICROCODE_UPDATE_TIMEOUT_US 1000000
+
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
static bool_t __initdata ucode_mod_forced;
+static unsigned int nr_cores;
+
+/*
+ * These states help to coordinate CPUs during loading an update.
+ *
+ * The semantics of each state is as follow:
+ * - LOADING_PREPARE: initial state of 'loading_state'.
+ * - LOADING_CALLIN: CPUs are allowed to callin.
+ * - LOADING_ENTER: all CPUs have called in. Initiate ucode loading.
+ * - LOADING_EXIT: ucode loading is done or aborted.
+ */
+static enum {
+ LOADING_PREPARE,
+ LOADING_CALLIN,
+ LOADING_ENTER,
+ LOADING_EXIT,
+} loading_state;
/*
* If we scan the initramfs.cpio for the early microcode code
@@ -190,6 +224,16 @@ static DEFINE_SPINLOCK(microcode_mutex);
DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
/*
+ * Count the CPUs that have entered, exited the rendezvous and succeeded in
+ * microcode update during late microcode update respectively.
+ *
+ * Note that a bitmap is used for callin to allow cpu to set a bit multiple
+ * times. It is required to do busy-loop in #NMI handling.
+ */
+static cpumask_t cpu_callin_map;
+static atomic_t cpu_out, cpu_updated;
+
+/*
* Return a patch that covers current CPU. If there are multiple patches,
* return the one with the highest revision number. Return error If no
* patch is found and an error occurs during the parsing process. Otherwise
@@ -231,6 +275,34 @@ static bool microcode_update_cache(struct microcode_patch *patch)
return true;
}
+/* Wait for a condition to be met with a timeout (us). */
+static int wait_for_condition(bool (*func)(unsigned int data),
+ unsigned int data, unsigned int timeout)
+{
+ while ( !func(data) )
+ {
+ if ( !timeout-- )
+ {
+ printk("CPU%u: Timeout in %pS\n",
+ smp_processor_id(), __builtin_return_address(0));
+ return -EBUSY;
+ }
+ udelay(1);
+ }
+
+ return 0;
+}
+
+static bool wait_cpu_callin(unsigned int nr)
+{
+ return cpumask_weight(&cpu_callin_map) >= nr;
+}
+
+static bool wait_cpu_callout(unsigned int nr)
+{
+ return atomic_read(&cpu_out) >= nr;
+}
+
/*
* Load a microcode update to current CPU.
*
@@ -264,40 +336,150 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
return err;
}
-static long do_microcode_update(void *patch)
+static bool wait_for_state(typeof(loading_state) state)
{
- unsigned int cpu;
- int ret = microcode_update_cpu(patch);
+ typeof(loading_state) cur_state;
- /* Store the patch after a successful loading */
- if ( !ret && patch )
+ while ( (cur_state = ACCESS_ONCE(loading_state)) != state )
{
- spin_lock(µcode_mutex);
- microcode_update_cache(patch);
- spin_unlock(µcode_mutex);
- patch = NULL;
+ if ( cur_state == LOADING_EXIT )
+ return false;
+ cpu_relax();
}
- if ( microcode_ops->end_update_percpu )
- microcode_ops->end_update_percpu();
+ return true;
+}
+
+static void set_state(unsigned int state)
+{
+ loading_state = state;
+ smp_wmb();
+}
+
+static int secondary_thread_fn(void)
+{
+ unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
+
+ if ( !wait_for_state(LOADING_CALLIN) )
+ return -EBUSY;
+
+ cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
+
+ if ( !wait_for_state(LOADING_EXIT) )
+ return -EBUSY;
+
+ /* Copy update revision from the primary thread. */
+ this_cpu(cpu_sig).rev = per_cpu(cpu_sig, primary).rev;
+
+ return 0;
+}
+
+static int primary_thread_fn(const struct microcode_patch *patch)
+{
+ int ret = 0;
+
+ if ( !wait_for_state(LOADING_CALLIN) )
+ return -EBUSY;
+
+ cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
+
+ if ( !wait_for_state(LOADING_ENTER) )
+ return -EBUSY;
+
+ ret = microcode_ops->apply_microcode(patch);
+ if ( !ret )
+ atomic_inc(&cpu_updated);
+ atomic_inc(&cpu_out);
+
+ return ret;
+}
+
+static int control_thread_fn(const struct microcode_patch *patch)
+{
+ unsigned int cpu = smp_processor_id(), done;
+ unsigned long tick;
+ int ret;
/*
- * Each thread tries to load ucode. Only the first thread of a core
- * would succeed while other threads would encounter -EINVAL which
- * indicates current ucode revision is equal to or newer than the
- * given patch. It is actually expected; so ignore this error.
+ * We intend to keep interrupt disabled for a long time, which may lead to
+ * watchdog timeout.
*/
- if ( ret == -EINVAL )
- ret = 0;
+ watchdog_disable();
- cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
- if ( cpu < nr_cpu_ids )
- return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?:
- ret;
+ /* Allow threads to call in */
+ set_state(LOADING_CALLIN);
- /* Free the patch if no CPU has loaded it successfully. */
- if ( patch )
- microcode_free_patch(patch);
+ cpumask_set_cpu(cpu, &cpu_callin_map);
+
+ /* Waiting for all threads calling in */
+ ret = wait_for_condition(wait_cpu_callin, num_online_cpus(),
+ MICROCODE_CALLIN_TIMEOUT_US);
+ if ( ret )
+ {
+ set_state(LOADING_EXIT);
+ return ret;
+ }
+
+ /* Let primary threads load the given ucode update */
+ set_state(LOADING_ENTER);
+
+ ret = microcode_ops->apply_microcode(patch);
+ if ( !ret )
+ atomic_inc(&cpu_updated);
+ atomic_inc(&cpu_out);
+
+ tick = rdtsc_ordered();
+ /* Wait for primary threads finishing update */
+ while ( (done = atomic_read(&cpu_out)) != nr_cores )
+ {
+ /*
+ * During each timeout interval, at least a CPU is expected to
+ * finish its update. Otherwise, something goes wrong.
+ *
+ * Note that RDTSC (in wait_for_condition()) is safe for threads to
+ * execute while waiting for completion of loading an update.
+ */
+ if ( wait_for_condition(wait_cpu_callout, (done + 1),
+ MICROCODE_UPDATE_TIMEOUT_US) )
+ panic("Timeout when finished updating microcode (finished %u/%u)",
+ done, nr_cores);
+
+ /* Print warning message once if long time is spent here */
+ if ( tick && rdtsc_ordered() - tick >= cpu_khz * 1000 )
+ {
+ printk(XENLOG_WARNING
+ "WARNING: UPDATING MICROCODE HAS CONSUMED MORE THAN 1 SECOND!\n");
+ tick = 0;
+ }
+ }
+
+ /* Mark loading is done to unblock other threads */
+ set_state(LOADING_EXIT);
+
+ watchdog_enable();
+
+ return ret;
+}
+
+static int do_microcode_update(void *patch)
+{
+ unsigned int cpu = smp_processor_id();
+ int ret;
+
+ /*
+ * The control thread set state to coordinate ucode loading. Primary
+ * threads load the given ucode patch. Secondary threads just wait for
+ * the completion of the ucode loading process.
+ */
+ if ( cpu == cpumask_first(&cpu_online_map) )
+ ret = control_thread_fn(patch);
+ else if ( cpu == cpumask_first(this_cpu(cpu_sibling_mask)) )
+ ret = primary_thread_fn(patch);
+ else
+ ret = secondary_thread_fn();
+
+ if ( microcode_ops->end_update_percpu )
+ microcode_ops->end_update_percpu();
return ret;
}
@@ -306,6 +488,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
int ret;
void *buffer;
+ unsigned int cpu, updated;
struct microcode_patch *patch;
if ( len != (uint32_t)len )
@@ -325,30 +508,84 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return -EFAULT;
}
+ /* cpu_online_map must not change during update */
+ if ( !get_cpu_maps() )
+ {
+ xfree(buffer);
+ return -EBUSY;
+ }
+
patch = parse_blob(buffer, len);
xfree(buffer);
if ( IS_ERR(patch) )
{
ret = PTR_ERR(patch);
printk(XENLOG_WARNING "Parsing microcode blob error %d\n", ret);
- return ret;
+ goto put;
}
if ( !patch )
- return -ENOENT;
+ {
+ ret = -ENOENT;
+ goto put;
+ }
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
- if ( ret != 0 )
+ if ( ret )
{
microcode_free_patch(patch);
- return ret;
+ goto put;
}
}
- return continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
- do_microcode_update, patch);
+ cpumask_clear(&cpu_callin_map);
+ atomic_set(&cpu_out, 0);
+ atomic_set(&cpu_updated, 0);
+ loading_state = LOADING_PREPARE;
+
+ /* Calculate the number of online CPU core */
+ nr_cores = 0;
+ for_each_online_cpu(cpu)
+ if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
+ nr_cores++;
+
+ printk(XENLOG_INFO "%u cores are to update their microcode\n", nr_cores);
+
+ /*
+ * Late loading dance. Why the heavy-handed stop_machine effort?
+ *
+ * - HT siblings must be idle and not execute other code while the other
+ * sibling is loading microcode in order to avoid any negative
+ * interactions cause by the loading.
+ *
+ * - In addition, microcode update on the cores must be serialized until
+ * this requirement can be relaxed in the future. Right now, this is
+ * conservative and good.
+ */
+ ret = stop_machine_run(do_microcode_update, patch, NR_CPUS);
+
+ updated = atomic_read(&cpu_updated);
+ if ( updated > 0 )
+ {
+ spin_lock(µcode_mutex);
+ microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+ }
+ else
+ microcode_free_patch(patch);
+
+ if ( updated && updated != nr_cores )
+ printk(XENLOG_ERR "ERROR: Updating microcode succeeded on %u cores and failed\n"
+ XENLOG_ERR "on other %u cores. A system with differing microcode\n"
+ XENLOG_ERR "revisions is considered unstable. Please reboot and do not\n"
+ XENLOG_ERR "load the microcode that triggers this warning!\n",
+ updated, nr_cores - updated);
+
+ put:
+ put_cpu_maps();
+ return ret;
}
static int __init microcode_init(void)
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 5/7] microcode: remove microcode_update_lock
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
` (3 preceding siblings ...)
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked Chao Gao
6 siblings, 0 replies; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
microcode_update_lock is to prevent logic threads of a same core from
updating microcode at the same time. But due to using a global lock, it
also prevented parallel microcode updating on different cores.
Remove this lock in order to update microcode in parallel. It is safe
because we have already ensured serialization of sibling threads at the
caller side.
1.For late microcode update, do_microcode_update() ensures that only one
sibiling thread of a core can update microcode.
2.For microcode update during system startup or CPU-hotplug,
microcode_mutex() guarantees update serialization of logical threads.
3.get/put_cpu_bitmaps() prevents the concurrency of CPU-hotplug and
late microcode update.
Note that printk in apply_microcode() and svm_host_osvm_init() (for AMD
only) are still processed sequentially.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v7:
- reworked. Remove complex lock logics introduced in v5 and v6. The microcode
patch to be applied is passed as an argument without any global variable. Thus
no lock is added to serialize potential readers/writers. Callers of
apply_microcode() will guarantee the correctness: the patch poninted by the
arguments won't be changed by others.
Changes in v6:
- introduce early_ucode_update_lock to serialize early ucode update.
Changes in v5:
- newly add
---
xen/arch/x86/microcode_amd.c | 8 +-------
xen/arch/x86/microcode_intel.c | 8 +-------
2 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 9a8f179..1e52f7f 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -74,9 +74,6 @@ struct mpbhdr {
uint8_t data[];
};
-/* serialize access to the physical write */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
/* See comment in start_update() for cases when this routine fails */
static int collect_cpu_info(struct cpu_signature *csig)
{
@@ -232,7 +229,6 @@ static enum microcode_match_result compare_patch(
static int apply_microcode(const struct microcode_patch *patch)
{
- unsigned long flags;
uint32_t rev;
int hw_err;
unsigned int cpu = smp_processor_id();
@@ -247,15 +243,13 @@ static int apply_microcode(const struct microcode_patch *patch)
hdr = patch->mc_amd->mpb;
- spin_lock_irqsave(µcode_update_lock, flags);
+ BUG_ON(local_irq_is_enabled());
hw_err = wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
/* get patch id after patching */
rdmsrl(MSR_AMD_PATCHLEVEL, rev);
- spin_unlock_irqrestore(µcode_update_lock, flags);
-
/*
* Some processors leave the ucode blob mapping as UC after the update.
* Flush the mapping to regain normal cacheability.
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index c083e17..9ededcc 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -93,9 +93,6 @@ struct extended_sigtable {
#define exttable_size(et) ((et)->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE)
-/* serialize access to the physical write to MSR 0x79 */
-static DEFINE_SPINLOCK(microcode_update_lock);
-
static int collect_cpu_info(struct cpu_signature *csig)
{
unsigned int cpu_num = smp_processor_id();
@@ -287,7 +284,6 @@ static enum microcode_match_result compare_patch(
static int apply_microcode(const struct microcode_patch *patch)
{
- unsigned long flags;
uint64_t msr_content;
unsigned int val[2];
unsigned int cpu_num = raw_smp_processor_id();
@@ -302,8 +298,7 @@ static int apply_microcode(const struct microcode_patch *patch)
mc_intel = patch->mc_intel;
- /* serialize access to the physical write to MSR 0x79 */
- spin_lock_irqsave(µcode_update_lock, flags);
+ BUG_ON(local_irq_is_enabled());
/* write microcode via MSR 0x79 */
wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
@@ -316,7 +311,6 @@ static int apply_microcode(const struct microcode_patch *patch)
rdmsrl(MSR_IA32_UCODE_REV, msr_content);
val[1] = (uint32_t)(msr_content >> 32);
- spin_unlock_irqrestore(µcode_update_lock, flags);
if ( val[1] != mc_intel->hdr.rev )
{
printk(KERN_ERR "microcode: CPU%d update from revision "
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
` (4 preceding siblings ...)
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 5/7] microcode: remove microcode_update_lock Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-27 10:19 ` Jan Beulich
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked Chao Gao
6 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Stefano Stabellini, Ashok Raj, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Ian Jackson, Tim Deegan,
Julien Grall, Jan Beulich, Andrew Cooper, Chao Gao,
Roger Pau Monné
When one core is loading ucode, handling NMI on sibling threads or
on other cores in the system might be problematic. By rendezvousing
all CPUs in NMI handler, it prevents NMI acceptance during ucode
loading.
Basically, some work previously done in stop_machine context is
moved to NMI handler. Primary threads call in and load ucode in
NMI handler. Secondary threads wait for the completion of ucode
loading on all CPU cores. An option is introduced to disable this
behavior.
Control thread doesn't rendezvous in NMI handler by calling self_nmi()
(in case of unknown_nmi_error() being triggered). The side effect is
control thread might be handling an NMI and interacting with the old
ucode not in a controlled way while other threads are loading ucode.
Update ucode on the control thread first to mitigate this issue.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v11:
- Extend existing 'nmi' option rather than use a new one.
- use per-cpu variable to store error code of xxx_nmi_work()
- rename secondary_thread_work to secondary_nmi_work.
- intialize nmi_patch to ZERO_BLOCK_PTR and make it static.
- constify nmi_cpu
- explain why control thread loads ucode first in patch description
Changes in v10:
- rewrite based on Sergey's idea and patch
- add Sergey's SOB.
- add an option to disable ucode loading in NMI handler
- don't send IPI NMI to the control thread to avoid unknown_nmi_error()
in do_nmi().
- add an assertion to make sure the cpu chosen to handle platform NMI
won't send self NMI. Otherwise, there is a risk that we encounter
unknown_nmi_error() and system crashes.
Changes in v9:
- control threads send NMI to all other threads. Slave threads will
stay in the NMI handling to prevent NMI acceptance during ucode
loading. Note that self-nmi is invalid according to SDM.
- s/rep_nop/cpu_relax
- remove debug message in microcode_nmi_callback(). Printing debug
message would take long times and control thread may timeout.
- rebase and fix conflicts
Changes in v8:
- new
---
docs/misc/xen-command-line.pandoc | 6 +-
xen/arch/x86/microcode.c | 156 ++++++++++++++++++++++++++++++--------
xen/arch/x86/traps.c | 6 +-
xen/include/asm-x86/nmi.h | 3 +
4 files changed, 138 insertions(+), 33 deletions(-)
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 832797e..8beb285 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2036,7 +2036,7 @@ pages) must also be specified via the tbuf_size parameter.
> `= unstable | skewed | stable:socket`
### ucode (x86)
-> `= [<integer> | scan]`
+> `= List of [ <integer> | scan, nmi=<bool> ]`
Specify how and where to find CPU microcode update blob.
@@ -2057,6 +2057,10 @@ microcode in the cpio name space must be:
- on Intel: kernel/x86/microcode/GenuineIntel.bin
- on AMD : kernel/x86/microcode/AuthenticAMD.bin
+'nmi' determines late loading is performed in NMI handler or just in
+stop_machine context. In NMI handler, even NMIs are blocked, which is
+considered safer. The default value is `true`.
+
### unrestricted_guest (Intel)
> `= <boolean>`
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 6c23879..b9fa8bb 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -36,8 +36,10 @@
#include <xen/earlycpio.h>
#include <xen/watchdog.h>
+#include <asm/apic.h>
#include <asm/delay.h>
#include <asm/msr.h>
+#include <asm/nmi.h>
#include <asm/processor.h>
#include <asm/setup.h>
#include <asm/microcode.h>
@@ -95,6 +97,9 @@ static struct ucode_mod_blob __initdata ucode_blob;
*/
static bool_t __initdata ucode_scan;
+/* By default, ucode loading is done in NMI handler */
+static bool ucode_in_nmi = true;
+
/* Protected by microcode_mutex */
static struct microcode_patch *microcode_cache;
@@ -105,23 +110,42 @@ void __init microcode_set_module(unsigned int idx)
}
/*
- * The format is '[<integer>|scan]'. Both options are optional.
+ * The format is '[<integer>|scan, nmi=<bool>]'. Both options are optional.
* If the EFI has forced which of the multiboot payloads is to be used,
- * no parsing will be attempted.
+ * only nmi=<bool> is parsed.
*/
static int __init parse_ucode(const char *s)
{
- const char *q = NULL;
+ const char *ss;
+ int val, rc = 0;
- if ( ucode_mod_forced ) /* Forced by EFI */
- return 0;
+ do {
+ ss = strchr(s, ',');
+ if ( !ss )
+ ss = strchr(s, '\0');
- if ( !strncmp(s, "scan", 4) )
- ucode_scan = 1;
- else
- ucode_mod_idx = simple_strtol(s, &q, 0);
+ if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
+ ucode_in_nmi = val;
+ else if ( !ucode_mod_forced ) /* Not forced by EFI */
+ {
+ const char *q = NULL;
+
+ if ( !strncmp(s, "scan", 4) )
+ {
+ ucode_scan = true;
+ q = s + 4;
+ }
+ else
+ ucode_mod_idx = simple_strtol(s, &q, 0);
+
+ if ( q != ss )
+ rc = -EINVAL;
+ }
+
+ s = ss + 1;
+ } while ( *ss );
- return (q && *q) ? -EINVAL : 0;
+ return rc;
}
custom_param("ucode", parse_ucode);
@@ -222,6 +246,8 @@ const struct microcode_ops *microcode_ops;
static DEFINE_SPINLOCK(microcode_mutex);
DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
+/* Store error code of the work done in NMI handler */
+DEFINE_PER_CPU(int, loading_err);
/*
* Count the CPUs that have entered, exited the rendezvous and succeeded in
@@ -232,6 +258,7 @@ DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
*/
static cpumask_t cpu_callin_map;
static atomic_t cpu_out, cpu_updated;
+static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
/*
* Return a patch that covers current CPU. If there are multiple patches,
@@ -356,42 +383,88 @@ static void set_state(unsigned int state)
smp_wmb();
}
-static int secondary_thread_fn(void)
+static int secondary_nmi_work(void)
{
- unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
+ cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
- if ( !wait_for_state(LOADING_CALLIN) )
- return -EBUSY;
+ return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
+}
+
+static int primary_thread_work(const struct microcode_patch *patch)
+{
+ int ret;
cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
- if ( !wait_for_state(LOADING_EXIT) )
+ if ( !wait_for_state(LOADING_ENTER) )
return -EBUSY;
- /* Copy update revision from the primary thread. */
- this_cpu(cpu_sig).rev = per_cpu(cpu_sig, primary).rev;
+ ret = microcode_ops->apply_microcode(patch);
+ if ( !ret )
+ atomic_inc(&cpu_updated);
+ atomic_inc(&cpu_out);
- return 0;
+ return ret;
}
-static int primary_thread_fn(const struct microcode_patch *patch)
+static int primary_nmi_work(const struct microcode_patch *patch)
+{
+ return primary_thread_work(patch);
+}
+
+static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
{
- int ret = 0;
+ unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
+ int ret;
+
+ /* System-generated NMI, leave to main handler */
+ if ( ACCESS_ONCE(loading_state) != LOADING_CALLIN )
+ return 0;
+
+ /*
+ * Primary threads load ucode in NMI handler on if ucode_in_nmi is true.
+ * Secondary threads are expected to stay in NMI handler regardless of
+ * ucode_in_nmi.
+ */
+ if ( cpu == cpumask_first(&cpu_online_map) ||
+ (!ucode_in_nmi && cpu == primary) )
+ return 0;
+
+ if ( cpu == primary )
+ ret = primary_nmi_work(nmi_patch);
+ else
+ ret = secondary_nmi_work();
+ this_cpu(loading_err) = ret;
+
+ return 0;
+}
+static int secondary_thread_fn(void)
+{
if ( !wait_for_state(LOADING_CALLIN) )
return -EBUSY;
- cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
+ self_nmi();
- if ( !wait_for_state(LOADING_ENTER) )
+ /* Copy update revision from the primary thread. */
+ this_cpu(cpu_sig).rev =
+ per_cpu(cpu_sig, cpumask_first(this_cpu(cpu_sibling_mask))).rev;
+
+ return this_cpu(loading_err);
+}
+
+static int primary_thread_fn(const struct microcode_patch *patch)
+{
+ if ( !wait_for_state(LOADING_CALLIN) )
return -EBUSY;
- ret = microcode_ops->apply_microcode(patch);
- if ( !ret )
- atomic_inc(&cpu_updated);
- atomic_inc(&cpu_out);
+ if ( ucode_in_nmi )
+ {
+ self_nmi();
+ return this_cpu(loading_err);
+ }
- return ret;
+ return primary_thread_work(patch);
}
static int control_thread_fn(const struct microcode_patch *patch)
@@ -399,6 +472,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
unsigned int cpu = smp_processor_id(), done;
unsigned long tick;
int ret;
+ nmi_callback_t *saved_nmi_callback;
/*
* We intend to keep interrupt disabled for a long time, which may lead to
@@ -406,6 +480,10 @@ static int control_thread_fn(const struct microcode_patch *patch)
*/
watchdog_disable();
+ nmi_patch = patch;
+ smp_wmb();
+ saved_nmi_callback = set_nmi_callback(microcode_nmi_callback);
+
/* Allow threads to call in */
set_state(LOADING_CALLIN);
@@ -420,14 +498,23 @@ static int control_thread_fn(const struct microcode_patch *patch)
return ret;
}
- /* Let primary threads load the given ucode update */
- set_state(LOADING_ENTER);
-
+ /* Control thread loads ucode first while others are in NMI handler. */
ret = microcode_ops->apply_microcode(patch);
if ( !ret )
atomic_inc(&cpu_updated);
atomic_inc(&cpu_out);
+ if ( ret == -EIO )
+ {
+ printk(XENLOG_ERR
+ "Late loading aborted: CPU%u failed to update ucode\n", cpu);
+ set_state(LOADING_EXIT);
+ return ret;
+ }
+
+ /* Let primary threads load the given ucode update */
+ set_state(LOADING_ENTER);
+
tick = rdtsc_ordered();
/* Wait for primary threads finishing update */
while ( (done = atomic_read(&cpu_out)) != nr_cores )
@@ -456,6 +543,8 @@ static int control_thread_fn(const struct microcode_patch *patch)
/* Mark loading is done to unblock other threads */
set_state(LOADING_EXIT);
+ set_nmi_callback(saved_nmi_callback);
+ nmi_patch = ZERO_BLOCK_PTR;
watchdog_enable();
return ret;
@@ -515,6 +604,13 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return -EBUSY;
}
+ /*
+ * CPUs except the first online CPU would send a fake (self) NMI to
+ * rendezvous in NMI handler. But a fake NMI to nmi_cpu may trigger
+ * unknown_nmi_error(). It ensures nmi_cpu won't receive a fake NMI.
+ */
+ ASSERT(cpumask_first(&cpu_online_map) == nmi_cpu);
+
patch = parse_blob(buffer, len);
xfree(buffer);
if ( IS_ERR(patch) )
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 16c590d..2cd5e29 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -126,6 +126,8 @@ boolean_param("ler", opt_ler);
/* LastExceptionFromIP on this hardware. Zero if LER is not in use. */
unsigned int __read_mostly ler_msr;
+const unsigned int nmi_cpu;
+
#define stack_words_per_line 4
#define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
@@ -1679,7 +1681,7 @@ void do_nmi(const struct cpu_user_regs *regs)
* this port before we re-arm the NMI watchdog, we reduce the chance
* of having an NMI watchdog expire while in the SMI handler.
*/
- if ( cpu == 0 )
+ if ( cpu == nmi_cpu )
reason = inb(0x61);
if ( (nmi_watchdog == NMI_NONE) ||
@@ -1687,7 +1689,7 @@ void do_nmi(const struct cpu_user_regs *regs)
handle_unknown = true;
/* Only the BSP gets external NMIs from the system. */
- if ( cpu == 0 )
+ if ( cpu == nmi_cpu )
{
if ( reason & 0x80 )
pci_serr_error(regs);
diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
index 99f6284..f9dfca6 100644
--- a/xen/include/asm-x86/nmi.h
+++ b/xen/include/asm-x86/nmi.h
@@ -11,6 +11,9 @@ extern bool opt_watchdog;
/* Watchdog force parameter from the command line */
extern bool watchdog_force;
+
+/* CPU to handle platform NMI */
+extern const unsigned int nmi_cpu;
typedef int nmi_callback_t(const struct cpu_user_regs *regs, int cpu);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
` (5 preceding siblings ...)
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode Chao Gao
@ 2019-09-26 13:53 ` Chao Gao
2019-09-27 11:19 ` Jan Beulich
6 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-26 13:53 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
If a core with all of its thread being parked, late ucode loading
which currently only loads ucode on online threads would lead to
differing ucode revisions in the system. In general, keeping ucode
revision consistent would be less error-prone. To this end, if there
is a parked thread doesn't have an online sibling thread, late ucode
loading is rejected.
Two threads are on the same core or computing unit iff they have
the same phys_proc_id and cpu_core_id/compute_unit_id. Based on
phys_proc_id and cpu_core_id/compute_unit_id, an unique core id
is generated for each thread. And use a bitmap to reduce the
number of comparison.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Alternatively, we can mask the thread id off apicid and use it
as the unique core id. It needs to introduce new field in cpuinfo_x86
to record the mask for thread id. So I don't take this way.
---
xen/arch/x86/microcode.c | 75 +++++++++++++++++++++++++++++++++++++++++
xen/include/asm-x86/processor.h | 1 +
2 files changed, 76 insertions(+)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index b9fa8bb..b70eb16 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -573,6 +573,64 @@ static int do_microcode_update(void *patch)
return ret;
}
+static unsigned int unique_core_id(unsigned int cpu, unsigned int socket_shift)
+{
+ unsigned int core_id = cpu_to_cu(cpu);
+
+ if ( core_id == INVALID_CUID )
+ core_id = cpu_to_core(cpu);
+
+ return (cpu_to_socket(cpu) << socket_shift) + core_id;
+}
+
+static int has_parked_core(void)
+{
+ int ret = 0;
+
+ if ( park_offline_cpus )
+ {
+ unsigned int cpu, max_bits, core_width;
+ unsigned int max_sockets = 1, max_cores = 1;
+ struct cpuinfo_x86 *c = cpu_data;
+ unsigned long *bitmap;
+
+ for_each_present_cpu(cpu)
+ {
+ if ( x86_cpu_to_apicid[cpu] == BAD_APICID )
+ continue;
+
+ /* Note that cpu_to_socket() get an ID starting from 0. */
+ if ( cpu_to_socket(cpu) + 1 > max_sockets )
+ max_sockets = cpu_to_socket(cpu) + 1;
+
+ if ( c[cpu].x86_max_cores > max_cores )
+ max_cores = c[cpu].x86_max_cores;
+ }
+
+ core_width = fls(max_cores);
+ max_bits = max_sockets << core_width;
+ bitmap = xzalloc_array(unsigned long, BITS_TO_LONGS(max_bits));
+ if ( !bitmap )
+ return -ENOMEM;
+
+ for_each_present_cpu(cpu)
+ {
+ if ( cpu_online(cpu) || x86_cpu_to_apicid[cpu] == BAD_APICID )
+ continue;
+
+ __set_bit(unique_core_id(cpu, core_width), bitmap);
+ }
+
+ for_each_online_cpu(cpu)
+ __clear_bit(unique_core_id(cpu, core_width), bitmap);
+
+ ret = (find_first_bit(bitmap, max_bits) < max_bits);
+ xfree(bitmap);
+ }
+
+ return ret;
+}
+
int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
int ret;
@@ -611,6 +669,23 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
*/
ASSERT(cpumask_first(&cpu_online_map) == nmi_cpu);
+ /*
+ * If there is a core with all of its threads parked, late loading may
+ * cause differing ucode revisions in the system. Refuse this operation.
+ */
+ ret = has_parked_core();
+ if ( ret )
+ {
+ if ( ret > 0 )
+ {
+ printk(XENLOG_WARNING
+ "Ucode loading aborted: found a parked core\n");
+ ret = -EPERM;
+ }
+ xfree(buffer);
+ goto put;
+ }
+
patch = parse_blob(buffer, len);
xfree(buffer);
if ( IS_ERR(patch) )
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index c92956f..753deec 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -171,6 +171,7 @@ extern unsigned int init_intel_cacheinfo(struct cpuinfo_x86 *c);
#define cpu_to_core(_cpu) (cpu_data[_cpu].cpu_core_id)
#define cpu_to_socket(_cpu) (cpu_data[_cpu].phys_proc_id)
+#define cpu_to_cu(_cpu) (cpu_data[_cpu].compute_unit_id)
unsigned int apicid_to_socket(unsigned int);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode()
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
@ 2019-09-27 9:38 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2019-09-27 9:38 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, xen-devel,
Roger Pau Monné
On 26.09.2019 15:53, Chao Gao wrote:
> @@ -249,49 +249,82 @@ bool microcode_update_cache(struct microcode_patch *patch)
> return true;
> }
>
> -static int microcode_update_cpu(const void *buf, size_t size)
> +/*
> + * Load a microcode update to current CPU.
> + *
> + * If no patch is provided, the cached patch will be loaded. Microcode update
> + * during APs bringup and CPU resuming falls into this case.
> + */
> +static int microcode_update_cpu(const struct microcode_patch *patch)
> {
> - int err;
> - unsigned int cpu = smp_processor_id();
> - struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
> + int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
>
> - spin_lock(µcode_mutex);
> + if ( unlikely(err) )
> + return err;
>
> - err = microcode_ops->collect_cpu_info(sig);
> - if ( likely(!err) )
> - err = microcode_ops->cpu_request_microcode(buf, size);
> + spin_lock(µcode_mutex);
> + if ( patch )
> + err = microcode_ops->apply_microcode(patch);
> + else if ( microcode_cache )
> + {
> + err = microcode_ops->apply_microcode(microcode_cache);
> + if ( err == -EIO )
> + {
> + microcode_free_patch(microcode_cache);
> + microcode_cache = NULL;
> + }
> + }
> + else
> + /* No patch to update */
> + err = -ENOENT;
> spin_unlock(µcode_mutex);
This is still not optimal (because of the locked region being
larger than really needed), but close enough to the original
code, i.e. I guess fine for now.
> -static long do_microcode_update(void *_info)
> +static long do_microcode_update(void *patch)
> {
> - struct microcode_info *info = _info;
> - int error;
> -
> - BUG_ON(info->cpu != smp_processor_id());
> + unsigned int cpu;
> + int ret = microcode_update_cpu(patch);
>
> - error = microcode_update_cpu(info->buffer, info->buffer_size);
> - if ( error )
> - info->error = error;
> + /* Store the patch after a successful loading */
> + if ( !ret && patch )
> + {
> + spin_lock(µcode_mutex);
> + microcode_update_cache(patch);
> + spin_unlock(µcode_mutex);
> + patch = NULL;
> + }
>
> if ( microcode_ops->end_update_percpu )
> microcode_ops->end_update_percpu();
>
> - info->cpu = cpumask_next(info->cpu, &cpu_online_map);
> - if ( info->cpu < nr_cpu_ids )
> - return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> + /*
> + * Each thread tries to load ucode. Only the first thread of a core
> + * would succeed while other threads would encounter -EINVAL which
> + * indicates current ucode revision is equal to or newer than the
> + * given patch. It is actually expected; so ignore this error.
> + */
> + if ( ret == -EINVAL )
> + ret = 0;
-EINVAL is a pretty generic error, and hence I'm wondering: Is the
described situation really the only one where -EINVAL would come
back? I'm again willing to accept this for now, but I think this
wants further refinement (i.e. use of a truly dedicated error code
for just the one condition you want to filter here, maybe -EEXIST.)
> @@ -556,19 +558,22 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
> break;
> }
>
> - /* Update cache if this patch covers current CPU */
> - if ( microcode_fits(new_patch->mc_amd) != MIS_UCODE )
> - microcode_update_cache(new_patch);
> - else
> - microcode_free_patch(new_patch);
> -
> - if ( match_cpu(microcode_get_cache()) )
> + /*
> + * If the new patch covers current CPU, compare patches and store the
> + * one with higher revision.
> + */
> + if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
> + (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
> {
> - error = apply_microcode(microcode_get_cache());
> - if ( error )
> - break;
> + struct microcode_patch *tmp = patch;
> +
> + patch = new_patch;
> + new_patch = tmp;
Looks like you're open-coding SWAP() here.
> @@ -398,26 +380,44 @@ static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
> return offset + total_size;
> }
>
> -static int cpu_request_microcode(const void *buf, size_t size)
> +static struct microcode_patch *cpu_request_microcode(const void *buf,
> + size_t size)
> {
> long offset = 0;
> int error = 0;
> void *mc;
> + struct microcode_patch *patch = NULL;
>
> while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
> {
> + struct microcode_patch *new_patch;
> +
> error = microcode_sanity_check(mc);
> if ( error )
> break;
> - error = get_matching_microcode(mc);
> - if ( error < 0 )
> +
> + new_patch = alloc_microcode_patch(mc);
> + if ( IS_ERR(new_patch) )
> + {
> + error = PTR_ERR(new_patch);
> break;
> + }
> +
> /*
> - * It's possible the data file has multiple matching ucode,
> - * lets keep searching till the latest version
> + * If the new patch covers current CPU, compare patches and store the
> + * one with higher revision.
> */
> - if ( error == 1 )
> - error = 0;
> + if ( (microcode_update_match(&new_patch->mc_intel->hdr) != MIS_UCODE) &&
> + (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
> + {
> + struct microcode_patch *tmp = patch;
> +
> + patch = new_patch;
> + new_patch = tmp;
And again here. Preferably with these last two taken care of
(which I'll take the liberty to do if I end up committing
this)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading Chao Gao
@ 2019-09-27 9:51 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2019-09-27 9:51 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Kevin Tian, Ashok Raj, Wei Liu, Andrew Cooper,
Jun Nakajima, xen-devel, Thomas Gleixner, Borislav Petkov,
Roger Pau Monné
On 26.09.2019 15:53, Chao Gao wrote:
> @@ -264,40 +336,150 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
> return err;
> }
>
> -static long do_microcode_update(void *patch)
> +static bool wait_for_state(typeof(loading_state) state)
> {
> - unsigned int cpu;
> - int ret = microcode_update_cpu(patch);
> + typeof(loading_state) cur_state;
>
> - /* Store the patch after a successful loading */
> - if ( !ret && patch )
> + while ( (cur_state = ACCESS_ONCE(loading_state)) != state )
With ACCESS_ONCE() used here, I think ...
> {
> - spin_lock(µcode_mutex);
> - microcode_update_cache(patch);
> - spin_unlock(µcode_mutex);
> - patch = NULL;
> + if ( cur_state == LOADING_EXIT )
> + return false;
> + cpu_relax();
> }
>
> - if ( microcode_ops->end_update_percpu )
> - microcode_ops->end_update_percpu();
> + return true;
> +}
> +
> +static void set_state(unsigned int state)
> +{
> + loading_state = state;
> + smp_wmb();
... it also wants to be used here (instead of the explicit barrier).
With this (which I'd be fine to be adjusted while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode Chao Gao
@ 2019-09-27 10:19 ` Jan Beulich
2019-09-27 13:53 ` Chao Gao
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2019-09-27 10:19 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Stefano Stabellini, Ashok Raj, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel, Roger Pau Monné
On 26.09.2019 15:53, Chao Gao wrote:
> @@ -105,23 +110,42 @@ void __init microcode_set_module(unsigned int idx)
> }
>
> /*
> - * The format is '[<integer>|scan]'. Both options are optional.
> + * The format is '[<integer>|scan, nmi=<bool>]'. Both options are optional.
> * If the EFI has forced which of the multiboot payloads is to be used,
> - * no parsing will be attempted.
> + * only nmi=<bool> is parsed.
> */
> static int __init parse_ucode(const char *s)
> {
> - const char *q = NULL;
> + const char *ss;
> + int val, rc = 0;
>
> - if ( ucode_mod_forced ) /* Forced by EFI */
> - return 0;
> + do {
> + ss = strchr(s, ',');
> + if ( !ss )
> + ss = strchr(s, '\0');
>
> - if ( !strncmp(s, "scan", 4) )
> - ucode_scan = 1;
> - else
> - ucode_mod_idx = simple_strtol(s, &q, 0);
> + if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
> + ucode_in_nmi = val;
> + else if ( !ucode_mod_forced ) /* Not forced by EFI */
> + {
> + const char *q = NULL;
> +
> + if ( !strncmp(s, "scan", 4) )
> + {
> + ucode_scan = true;
I guess it would have resulted in more consistent code if you had
used parse_boolean() here, too.
> @@ -222,6 +246,8 @@ const struct microcode_ops *microcode_ops;
> static DEFINE_SPINLOCK(microcode_mutex);
>
> DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
> +/* Store error code of the work done in NMI handler */
> +DEFINE_PER_CPU(int, loading_err);
static
> @@ -356,42 +383,88 @@ static void set_state(unsigned int state)
> smp_wmb();
> }
>
> -static int secondary_thread_fn(void)
> +static int secondary_nmi_work(void)
> {
> - unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
> + cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>
> - if ( !wait_for_state(LOADING_CALLIN) )
> - return -EBUSY;
> + return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
> +}
> +
> +static int primary_thread_work(const struct microcode_patch *patch)
> +{
> + int ret;
>
> cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>
> - if ( !wait_for_state(LOADING_EXIT) )
> + if ( !wait_for_state(LOADING_ENTER) )
> return -EBUSY;
>
> - /* Copy update revision from the primary thread. */
> - this_cpu(cpu_sig).rev = per_cpu(cpu_sig, primary).rev;
> + ret = microcode_ops->apply_microcode(patch);
> + if ( !ret )
> + atomic_inc(&cpu_updated);
> + atomic_inc(&cpu_out);
>
> - return 0;
> + return ret;
> }
>
> -static int primary_thread_fn(const struct microcode_patch *patch)
> +static int primary_nmi_work(const struct microcode_patch *patch)
> +{
> + return primary_thread_work(patch);
> +}
Why this wrapper? The function signatures are identical. I guess
you want to emphasize the environment the function is to be used
in, so perhaps fine despite the redundancy. At least there's no
address taken of this function, so the compiler can eliminate it.
> +static int secondary_thread_fn(void)
> +{
> if ( !wait_for_state(LOADING_CALLIN) )
> return -EBUSY;
>
> - cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
> + self_nmi();
>
> - if ( !wait_for_state(LOADING_ENTER) )
> + /* Copy update revision from the primary thread. */
> + this_cpu(cpu_sig).rev =
> + per_cpu(cpu_sig, cpumask_first(this_cpu(cpu_sibling_mask))).rev;
_alternative_instructions() takes specific care to avoid relying on
the NMI potentially not arriving synchronously (in which case you'd
potentially copy a not-yet-updated CPU signature above). I think the
same care wants applying here, which I guess would be another
wait_for_state(LOADING_EXIT);
> + return this_cpu(loading_err);
> +}
> +
> +static int primary_thread_fn(const struct microcode_patch *patch)
> +{
> + if ( !wait_for_state(LOADING_CALLIN) )
> return -EBUSY;
>
> - ret = microcode_ops->apply_microcode(patch);
> - if ( !ret )
> - atomic_inc(&cpu_updated);
> - atomic_inc(&cpu_out);
> + if ( ucode_in_nmi )
> + {
> + self_nmi();
> + return this_cpu(loading_err);
Same here than, to protect against returning a not-yet-updated error
indicator.
> @@ -420,14 +498,23 @@ static int control_thread_fn(const struct microcode_patch *patch)
> return ret;
> }
>
> - /* Let primary threads load the given ucode update */
> - set_state(LOADING_ENTER);
> -
> + /* Control thread loads ucode first while others are in NMI handler. */
> ret = microcode_ops->apply_microcode(patch);
> if ( !ret )
> atomic_inc(&cpu_updated);
> atomic_inc(&cpu_out);
>
> + if ( ret == -EIO )
> + {
> + printk(XENLOG_ERR
> + "Late loading aborted: CPU%u failed to update ucode\n", cpu);
> + set_state(LOADING_EXIT);
> + return ret;
> + }
> +
> + /* Let primary threads load the given ucode update */
> + set_state(LOADING_ENTER);
While the description goes to some lengths to explain this ordering of
updates, I still don't really see the point: How is it better for the
control CPU to have updated its ucode early and then hit an NMI before
the other CPUs have even started updating, than the other way around
in the opposite case?
> @@ -456,6 +543,8 @@ static int control_thread_fn(const struct microcode_patch *patch)
> /* Mark loading is done to unblock other threads */
> set_state(LOADING_EXIT);
>
> + set_nmi_callback(saved_nmi_callback);
> + nmi_patch = ZERO_BLOCK_PTR;
Another smp_wmb() between them, just to be on the safe side?
> @@ -515,6 +604,13 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> return -EBUSY;
> }
>
> + /*
> + * CPUs except the first online CPU would send a fake (self) NMI to
> + * rendezvous in NMI handler. But a fake NMI to nmi_cpu may trigger
> + * unknown_nmi_error(). It ensures nmi_cpu won't receive a fake NMI.
> + */
> + ASSERT(cpumask_first(&cpu_online_map) == nmi_cpu);
Looking at this again, I don't think it should be ASSERT(). Instead
you want to return an (easy to recognize) error in this case - maybe
-EPERM or -ENOEXEC or -EXDEV. This is not the least to also be safe
in non-debug builds.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked Chao Gao
@ 2019-09-27 11:19 ` Jan Beulich
2019-09-30 6:43 ` Chao Gao
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2019-09-27 11:19 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, xen-devel,
Roger Pau Monné
On 26.09.2019 15:53, Chao Gao wrote:
> If a core with all of its thread being parked, late ucode loading
> which currently only loads ucode on online threads would lead to
> differing ucode revisions in the system. In general, keeping ucode
> revision consistent would be less error-prone. To this end, if there
> is a parked thread doesn't have an online sibling thread, late ucode
> loading is rejected.
>
> Two threads are on the same core or computing unit iff they have
> the same phys_proc_id and cpu_core_id/compute_unit_id. Based on
> phys_proc_id and cpu_core_id/compute_unit_id, an unique core id
> is generated for each thread. And use a bitmap to reduce the
> number of comparison.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> Alternatively, we can mask the thread id off apicid and use it
> as the unique core id. It needs to introduce new field in cpuinfo_x86
> to record the mask for thread id. So I don't take this way.
It feels a little odd that you introduce a "custom" ID, but it
should be fine without going this alternative route. (You
wouldn't need a new field though, I think, as we've got the
x86_num_siblings one already.)
What I continue to be unconvinced of is for the chosen approach
to be better than briefly unparking a thread on each core, as
previously suggested.
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -573,6 +573,64 @@ static int do_microcode_update(void *patch)
> return ret;
> }
>
> +static unsigned int unique_core_id(unsigned int cpu, unsigned int socket_shift)
> +{
> + unsigned int core_id = cpu_to_cu(cpu);
> +
> + if ( core_id == INVALID_CUID )
> + core_id = cpu_to_core(cpu);
> +
> + return (cpu_to_socket(cpu) << socket_shift) + core_id;
> +}
> +
> +static int has_parked_core(void)
> +{
> + int ret = 0;
I don't think you need the initializer here.
> + if ( park_offline_cpus )
if ( !park_offline_cpus )
return 0;
would allow one level less of indentation of the main part of
the function body.
> + {
> + unsigned int cpu, max_bits, core_width;
> + unsigned int max_sockets = 1, max_cores = 1;
> + struct cpuinfo_x86 *c = cpu_data;
> + unsigned long *bitmap;
> +
> + for_each_present_cpu(cpu)
> + {
> + if ( x86_cpu_to_apicid[cpu] == BAD_APICID )
> + continue;
> +
> + /* Note that cpu_to_socket() get an ID starting from 0. */
> + if ( cpu_to_socket(cpu) + 1 > max_sockets )
Instead of "+ 1", why not >= ?
> + max_sockets = cpu_to_socket(cpu) + 1;
> +
> + if ( c[cpu].x86_max_cores > max_cores )
> + max_cores = c[cpu].x86_max_cores;
What guarantees .x86_max_cores to be valid? Onlining a hot-added
CPU is a two step process afaict, XENPF_cpu_hotadd followed by
XENPF_cpu_online. In between the CPU would be marked present
(and cpu_add() would also have filled x86_cpu_to_apicid[cpu]),
but cpu_data[cpu] wouldn't have been filled yet afaict. This
also makes the results of the cpu_to_*() unreliable that you use
in unique_core_id().
However, if we assume sufficient similarity between CPU
packages (as you've done elsewhere in this series iirc), this
may not be an actual problem. But it wants mentioning in a code
comment, I think. Plus at the very least you depend on the used
cpu_data[] fields to not contain unduly large values (and hence
you e.g. depend on cpu_data[] not gaining an initializer,
setting the three fields of interest to their INVALID_* values,
as currently done by identify_cpu()).
> + }
> +
> + core_width = fls(max_cores);
> + max_bits = max_sockets << core_width;
> + bitmap = xzalloc_array(unsigned long, BITS_TO_LONGS(max_bits));
> + if ( !bitmap )
> + return -ENOMEM;
> +
> + for_each_present_cpu(cpu)
> + {
> + if ( cpu_online(cpu) || x86_cpu_to_apicid[cpu] == BAD_APICID )
> + continue;
> +
> + __set_bit(unique_core_id(cpu, core_width), bitmap);
> + }
> +
> + for_each_online_cpu(cpu)
> + __clear_bit(unique_core_id(cpu, core_width), bitmap);
> +
> + ret = (find_first_bit(bitmap, max_bits) < max_bits);
I think bitmap_empty() would be a cheaper operation for the purpose
you have, especially if further up you rounded up max_bits to a
multiple of BITS_PER_LONG.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode
2019-09-27 10:19 ` Jan Beulich
@ 2019-09-27 13:53 ` Chao Gao
2019-09-27 13:55 ` Jan Beulich
0 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-27 13:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Stefano Stabellini, Ashok Raj, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel, Roger Pau Monné
On Fri, Sep 27, 2019 at 12:19:22PM +0200, Jan Beulich wrote:
>On 26.09.2019 15:53, Chao Gao wrote:
>> @@ -105,23 +110,42 @@ void __init microcode_set_module(unsigned int idx)
>> }
>>
>> /*
>> - * The format is '[<integer>|scan]'. Both options are optional.
>> + * The format is '[<integer>|scan, nmi=<bool>]'. Both options are optional.
>> * If the EFI has forced which of the multiboot payloads is to be used,
>> - * no parsing will be attempted.
>> + * only nmi=<bool> is parsed.
>> */
>> static int __init parse_ucode(const char *s)
>> {
>> - const char *q = NULL;
>> + const char *ss;
>> + int val, rc = 0;
>>
>> - if ( ucode_mod_forced ) /* Forced by EFI */
>> - return 0;
>> + do {
>> + ss = strchr(s, ',');
>> + if ( !ss )
>> + ss = strchr(s, '\0');
>>
>> - if ( !strncmp(s, "scan", 4) )
>> - ucode_scan = 1;
>> - else
>> - ucode_mod_idx = simple_strtol(s, &q, 0);
>> + if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
>> + ucode_in_nmi = val;
>> + else if ( !ucode_mod_forced ) /* Not forced by EFI */
>> + {
>> + const char *q = NULL;
>> +
>> + if ( !strncmp(s, "scan", 4) )
>> + {
>> + ucode_scan = true;
>
>I guess it would have resulted in more consistent code if you had
>used parse_boolean() here, too.
>
>> @@ -222,6 +246,8 @@ const struct microcode_ops *microcode_ops;
>> static DEFINE_SPINLOCK(microcode_mutex);
>>
>> DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
>> +/* Store error code of the work done in NMI handler */
>> +DEFINE_PER_CPU(int, loading_err);
>
>static
>
>> @@ -356,42 +383,88 @@ static void set_state(unsigned int state)
>> smp_wmb();
>> }
>>
>> -static int secondary_thread_fn(void)
>> +static int secondary_nmi_work(void)
>> {
>> - unsigned int primary = cpumask_first(this_cpu(cpu_sibling_mask));
>> + cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>>
>> - if ( !wait_for_state(LOADING_CALLIN) )
>> - return -EBUSY;
>> + return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
>> +}
>> +
>> +static int primary_thread_work(const struct microcode_patch *patch)
>> +{
>> + int ret;
>>
>> cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>>
>> - if ( !wait_for_state(LOADING_EXIT) )
>> + if ( !wait_for_state(LOADING_ENTER) )
>> return -EBUSY;
>>
>> - /* Copy update revision from the primary thread. */
>> - this_cpu(cpu_sig).rev = per_cpu(cpu_sig, primary).rev;
>> + ret = microcode_ops->apply_microcode(patch);
>> + if ( !ret )
>> + atomic_inc(&cpu_updated);
>> + atomic_inc(&cpu_out);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> -static int primary_thread_fn(const struct microcode_patch *patch)
>> +static int primary_nmi_work(const struct microcode_patch *patch)
>> +{
>> + return primary_thread_work(patch);
>> +}
>
>Why this wrapper? The function signatures are identical. I guess
>you want to emphasize the environment the function is to be used
>in, so perhaps fine despite the redundancy. At least there's no
>address taken of this function, so the compiler can eliminate it.
>
>> +static int secondary_thread_fn(void)
>> +{
>> if ( !wait_for_state(LOADING_CALLIN) )
>> return -EBUSY;
>>
>> - cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
>> + self_nmi();
>>
>> - if ( !wait_for_state(LOADING_ENTER) )
>> + /* Copy update revision from the primary thread. */
>> + this_cpu(cpu_sig).rev =
>> + per_cpu(cpu_sig, cpumask_first(this_cpu(cpu_sibling_mask))).rev;
>
>_alternative_instructions() takes specific care to avoid relying on
>the NMI potentially not arriving synchronously (in which case you'd
>potentially copy a not-yet-updated CPU signature above). I think the
>same care wants applying here, which I guess would be another
>
> wait_for_state(LOADING_EXIT);
>
>> + return this_cpu(loading_err);
>> +}
>> +
>> +static int primary_thread_fn(const struct microcode_patch *patch)
>> +{
>> + if ( !wait_for_state(LOADING_CALLIN) )
>> return -EBUSY;
>>
>> - ret = microcode_ops->apply_microcode(patch);
>> - if ( !ret )
>> - atomic_inc(&cpu_updated);
>> - atomic_inc(&cpu_out);
>> + if ( ucode_in_nmi )
>> + {
>> + self_nmi();
>> + return this_cpu(loading_err);
>
>Same here than, to protect against returning a not-yet-updated error
>indicator.
>
>> @@ -420,14 +498,23 @@ static int control_thread_fn(const struct microcode_patch *patch)
>> return ret;
>> }
>>
>> - /* Let primary threads load the given ucode update */
>> - set_state(LOADING_ENTER);
>> -
>> + /* Control thread loads ucode first while others are in NMI handler. */
>> ret = microcode_ops->apply_microcode(patch);
>> if ( !ret )
>> atomic_inc(&cpu_updated);
>> atomic_inc(&cpu_out);
>>
>> + if ( ret == -EIO )
>> + {
>> + printk(XENLOG_ERR
>> + "Late loading aborted: CPU%u failed to update ucode\n", cpu);
>> + set_state(LOADING_EXIT);
>> + return ret;
>> + }
>> +
>> + /* Let primary threads load the given ucode update */
>> + set_state(LOADING_ENTER);
>
>While the description goes to some lengths to explain this ordering of
>updates, I still don't really see the point: How is it better for the
>control CPU to have updated its ucode early and then hit an NMI before
>the other CPUs have even started updating, than the other way around
>in the opposite case?
We want to be conservative here. If an ucode is to update something
shared by a whole socket, for the latter case, control thread may
be accessing things that are being updating by the ucode loading on
other cores. It is not safe, just like sibling thread isn't expected
to access features exposed by the old ucode when primary thread is
loading ucode.
Do you think it makes a little sense? If yes, I would like to post
a new version of this patch later this day to catch up Xen 4.13.
Other comments make sense to me.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode
2019-09-27 13:53 ` Chao Gao
@ 2019-09-27 13:55 ` Jan Beulich
2019-09-27 14:15 ` Chao Gao
0 siblings, 1 reply; 17+ messages in thread
From: Jan Beulich @ 2019-09-27 13:55 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Stefano Stabellini, Ashok Raj, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel, Roger Pau Monné
On 27.09.2019 15:53, Chao Gao wrote:
> On Fri, Sep 27, 2019 at 12:19:22PM +0200, Jan Beulich wrote:
>> On 26.09.2019 15:53, Chao Gao wrote:
>>> @@ -420,14 +498,23 @@ static int control_thread_fn(const struct microcode_patch *patch)
>>> return ret;
>>> }
>>>
>>> - /* Let primary threads load the given ucode update */
>>> - set_state(LOADING_ENTER);
>>> -
>>> + /* Control thread loads ucode first while others are in NMI handler. */
>>> ret = microcode_ops->apply_microcode(patch);
>>> if ( !ret )
>>> atomic_inc(&cpu_updated);
>>> atomic_inc(&cpu_out);
>>>
>>> + if ( ret == -EIO )
>>> + {
>>> + printk(XENLOG_ERR
>>> + "Late loading aborted: CPU%u failed to update ucode\n", cpu);
>>> + set_state(LOADING_EXIT);
>>> + return ret;
>>> + }
>>> +
>>> + /* Let primary threads load the given ucode update */
>>> + set_state(LOADING_ENTER);
>>
>> While the description goes to some lengths to explain this ordering of
>> updates, I still don't really see the point: How is it better for the
>> control CPU to have updated its ucode early and then hit an NMI before
>> the other CPUs have even started updating, than the other way around
>> in the opposite case?
>
> We want to be conservative here. If an ucode is to update something
> shared by a whole socket, for the latter case, control thread may
> be accessing things that are being updating by the ucode loading on
> other cores. It is not safe, just like sibling thread isn't expected
> to access features exposed by the old ucode when primary thread is
> loading ucode.
Ah yes, considering a socket-wide effect didn't occur to me (although
it should have). So if you mention this aspect in the description, I
think I'm going to be fine with the change in this regard. Yet (as so
often) this raises another question: What about "secondary" sockets?
Shouldn't we entertain a similar two-step approach there then?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode
2019-09-27 13:55 ` Jan Beulich
@ 2019-09-27 14:15 ` Chao Gao
0 siblings, 0 replies; 17+ messages in thread
From: Chao Gao @ 2019-09-27 14:15 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Stefano Stabellini, Ashok Raj, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, xen-devel, Roger Pau Monné
On Fri, Sep 27, 2019 at 03:55:00PM +0200, Jan Beulich wrote:
>On 27.09.2019 15:53, Chao Gao wrote:
>> On Fri, Sep 27, 2019 at 12:19:22PM +0200, Jan Beulich wrote:
>>> On 26.09.2019 15:53, Chao Gao wrote:
>>>> @@ -420,14 +498,23 @@ static int control_thread_fn(const struct microcode_patch *patch)
>>>> return ret;
>>>> }
>>>>
>>>> - /* Let primary threads load the given ucode update */
>>>> - set_state(LOADING_ENTER);
>>>> -
>>>> + /* Control thread loads ucode first while others are in NMI handler. */
>>>> ret = microcode_ops->apply_microcode(patch);
>>>> if ( !ret )
>>>> atomic_inc(&cpu_updated);
>>>> atomic_inc(&cpu_out);
>>>>
>>>> + if ( ret == -EIO )
>>>> + {
>>>> + printk(XENLOG_ERR
>>>> + "Late loading aborted: CPU%u failed to update ucode\n", cpu);
>>>> + set_state(LOADING_EXIT);
>>>> + return ret;
>>>> + }
>>>> +
>>>> + /* Let primary threads load the given ucode update */
>>>> + set_state(LOADING_ENTER);
>>>
>>> While the description goes to some lengths to explain this ordering of
>>> updates, I still don't really see the point: How is it better for the
>>> control CPU to have updated its ucode early and then hit an NMI before
>>> the other CPUs have even started updating, than the other way around
>>> in the opposite case?
>>
>> We want to be conservative here. If an ucode is to update something
>> shared by a whole socket, for the latter case, control thread may
>> be accessing things that are being updating by the ucode loading on
>> other cores. It is not safe, just like sibling thread isn't expected
>> to access features exposed by the old ucode when primary thread is
>> loading ucode.
>
>Ah yes, considering a socket-wide effect didn't occur to me (although
>it should have). So if you mention this aspect in the description, I
>think I'm going to be fine with the change in this regard. Yet (as so
>often) this raises another question: What about "secondary" sockets?
>Shouldn't we entertain a similar two-step approach there then?
No. The two-step approach is because control thread cannot call
self_nmi() in case of triggering unknown_nmi_error() and what is done
in the main NMI handler isn't well controlled. All cores on other
sockets will rendezvous in NMI handler. It means every core's behavior
on other sockets is well controlled.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked
2019-09-27 11:19 ` Jan Beulich
@ 2019-09-30 6:43 ` Chao Gao
2019-09-30 8:12 ` Jan Beulich
0 siblings, 1 reply; 17+ messages in thread
From: Chao Gao @ 2019-09-30 6:43 UTC (permalink / raw)
To: Jan Beulich
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, xen-devel,
Roger Pau Monné
On Fri, Sep 27, 2019 at 01:19:16PM +0200, Jan Beulich wrote:
>On 26.09.2019 15:53, Chao Gao wrote:
>> If a core with all of its thread being parked, late ucode loading
>> which currently only loads ucode on online threads would lead to
>> differing ucode revisions in the system. In general, keeping ucode
>> revision consistent would be less error-prone. To this end, if there
>> is a parked thread doesn't have an online sibling thread, late ucode
>> loading is rejected.
>>
>> Two threads are on the same core or computing unit iff they have
>> the same phys_proc_id and cpu_core_id/compute_unit_id. Based on
>> phys_proc_id and cpu_core_id/compute_unit_id, an unique core id
>> is generated for each thread. And use a bitmap to reduce the
>> number of comparison.
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> Alternatively, we can mask the thread id off apicid and use it
>> as the unique core id. It needs to introduce new field in cpuinfo_x86
>> to record the mask for thread id. So I don't take this way.
>
>It feels a little odd that you introduce a "custom" ID, but it
>should be fine without going this alternative route. (You
>wouldn't need a new field though, I think, as we've got the
>x86_num_siblings one already.)
>
>What I continue to be unconvinced of is for the chosen approach
>to be better than briefly unparking a thread on each core, as
>previously suggested.
It isn't so easy to go the same way as set_cx_pminfo().
1. NMI handler on parked threads is changed to a nop. To load ucode in
NMI handler, we have to switch back to normal NMI handler in
default_idle(). But it conflicts with what the comments in play_dead()
implies: it is not safe to call normal NMI handler after
cpu_exit_clear().
2. A precondition of unparking a thread on each core, we need to find
out exactly all parked cores and wake up one thread of each of them.
Then in theory, what this patch does is only part of unparking a thread
on each core.
I don't mean they are hard to address. But we need to take care of them.
Given that, IMO, this patch is much straightforward.
>
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -573,6 +573,64 @@ static int do_microcode_update(void *patch)
>> return ret;
>> }
>>
>> +static unsigned int unique_core_id(unsigned int cpu, unsigned int socket_shift)
>> +{
>> + unsigned int core_id = cpu_to_cu(cpu);
>> +
>> + if ( core_id == INVALID_CUID )
>> + core_id = cpu_to_core(cpu);
>> +
>> + return (cpu_to_socket(cpu) << socket_shift) + core_id;
>> +}
>> +
>> +static int has_parked_core(void)
>> +{
>> + int ret = 0;
>
>I don't think you need the initializer here.
>
>> + if ( park_offline_cpus )
>
> if ( !park_offline_cpus )
> return 0;
>
>would allow one level less of indentation of the main part of
>the function body.
>
>> + {
>> + unsigned int cpu, max_bits, core_width;
>> + unsigned int max_sockets = 1, max_cores = 1;
>> + struct cpuinfo_x86 *c = cpu_data;
>> + unsigned long *bitmap;
>
+
>> + for_each_present_cpu(cpu)
>> + {
>> + if ( x86_cpu_to_apicid[cpu] == BAD_APICID )
>> + continue;
>> +
>> + /* Note that cpu_to_socket() get an ID starting from 0. */
>> + if ( cpu_to_socket(cpu) + 1 > max_sockets )
>
>Instead of "+ 1", why not >= ?
>
>> + max_sockets = cpu_to_socket(cpu) + 1;
>> +
>> + if ( c[cpu].x86_max_cores > max_cores )
>> + max_cores = c[cpu].x86_max_cores;
>
>What guarantees .x86_max_cores to be valid? Onlining a hot-added
>CPU is a two step process afaict, XENPF_cpu_hotadd followed by
>XENPF_cpu_online. In between the CPU would be marked present
>(and cpu_add() would also have filled x86_cpu_to_apicid[cpu]),
>but cpu_data[cpu] wouldn't have been filled yet afaict. This
>also makes the results of the cpu_to_*() unreliable that you use
>in unique_core_id().
Indeed. I agree.
>
>However, if we assume sufficient similarity between CPU
>packages (as you've done elsewhere in this series iirc), this
Yes.
>may not be an actual problem. But it wants mentioning in a code
>comment, I think. Plus at the very least you depend on the used
>cpu_data[] fields to not contain unduly large values (and hence
>you e.g. depend on cpu_data[] not gaining an initializer,
>setting the three fields of interest to their INVALID_* values,
>as currently done by identify_cpu()).
Can we skip those threads whose socket ID is invalid and initialize
the three fields in cpu_add()?
Or maintain a bitmap for parked threads to help distinguish them from
real offlined threads, and go through parked threads here?
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked
2019-09-30 6:43 ` Chao Gao
@ 2019-09-30 8:12 ` Jan Beulich
0 siblings, 0 replies; 17+ messages in thread
From: Jan Beulich @ 2019-09-30 8:12 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, xen-devel,
Roger Pau Monné
On 30.09.2019 08:43, Chao Gao wrote:
> On Fri, Sep 27, 2019 at 01:19:16PM +0200, Jan Beulich wrote:
>> What I continue to be unconvinced of is for the chosen approach
>> to be better than briefly unparking a thread on each core, as
>> previously suggested.
>
> It isn't so easy to go the same way as set_cx_pminfo().
>
> 1. NMI handler on parked threads is changed to a nop. To load ucode in
> NMI handler, we have to switch back to normal NMI handler in
> default_idle(). But it conflicts with what the comments in play_dead()
> implies: it is not safe to call normal NMI handler after
> cpu_exit_clear().
Right - pointing at the Cx handling for reference was perhaps not
the best choice. Here we'd need to truly online a core, remembering
to offline it again right after the ucode update.
> 2. A precondition of unparking a thread on each core, we need to find
> out exactly all parked cores and wake up one thread of each of them.
> Then in theory, what this patch does is only part of unparking a thread
> on each core.
Possibly, but you've suggested a possibly better alternative further
down.
>> may not be an actual problem. But it wants mentioning in a code
>> comment, I think. Plus at the very least you depend on the used
>> cpu_data[] fields to not contain unduly large values (and hence
>> you e.g. depend on cpu_data[] not gaining an initializer,
>> setting the three fields of interest to their INVALID_* values,
>> as currently done by identify_cpu()).
>
> Can we skip those threads whose socket ID is invalid and initialize
> the three fields in cpu_add()?
What would you initialize them to in cpu_add()? You don't know
their values yet, do you?
> Or maintain a bitmap for parked threads to help distinguish them from
> real offlined threads, and go through parked threads here?
I think this is the better approach in the long run. I've been at
least considering addition of such a bitmap for other reasons as well.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2019-09-30 8:12 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-26 13:53 [Xen-devel] [PATCH v11 0/7] improve late microcode loading Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
2019-09-27 9:38 ` Jan Beulich
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 2/7] microcode: unify ucode loading during system bootup and resuming Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 3/7] microcode: reduce memory allocation and copy when creating a patch Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading Chao Gao
2019-09-27 9:51 ` Jan Beulich
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 5/7] microcode: remove microcode_update_lock Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode Chao Gao
2019-09-27 10:19 ` Jan Beulich
2019-09-27 13:53 ` Chao Gao
2019-09-27 13:55 ` Jan Beulich
2019-09-27 14:15 ` Chao Gao
2019-09-26 13:53 ` [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked Chao Gao
2019-09-27 11:19 ` Jan Beulich
2019-09-30 6:43 ` Chao Gao
2019-09-30 8:12 ` Jan Beulich
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).