From patchwork Thu Sep 26 13:53:29 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162785 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id F214F17D4 for ; Thu, 26 Sep 2019 13:51:09 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id CC397207FF for ; Thu, 26 Sep 2019 13:51:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CC397207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9S-00022Q-MJ; Thu, 26 Sep 2019 13:49:46 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9R-00022D-8P for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:49:45 +0000 X-Inumbo-ID: 7c129f88-e064-11e9-97fb-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 7c129f88-e064-11e9-97fb-bc764e2007e4; Thu, 26 Sep 2019 13:49:39 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126020" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:37 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:29 +0800 Message-Id: <1569506015-26938-2-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode() X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Ashok Raj , Wei Liu , Andrew Cooper , Jan Beulich , Chao Gao , =?utf-8?q?R?= =?utf-8?q?oger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Reviewed-by: Jan Beulich --- 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 */ From patchwork Thu Sep 26 13:53:30 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162787 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 08A19912 for ; Thu, 26 Sep 2019 13:51:15 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E1528207FF for ; Thu, 26 Sep 2019 13:51:14 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E1528207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9Y-00024N-65; Thu, 26 Sep 2019 13:49:52 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9W-00023m-83 for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:49:50 +0000 X-Inumbo-ID: 7d7576b6-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 7d7576b6-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:41 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:41 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126026" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:39 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:30 +0800 Message-Id: <1569506015-26938-3-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 2/7] microcode: unify ucode loading during system bootup and resuming X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Ashok Raj , Wei Liu , Andrew Cooper , Jan Beulich , Chao Gao , =?utf-8?q?R?= =?utf-8?q?oger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Reviewed-by: Jan Beulich --- 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); From patchwork Thu Sep 26 13:53:31 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162791 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 62B7017D4 for ; Thu, 26 Sep 2019 13:51:19 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3D209207FF for ; Thu, 26 Sep 2019 13:51:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3D209207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9c-000266-GP; Thu, 26 Sep 2019 13:49:56 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9b-00025f-8E for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:49:55 +0000 X-Inumbo-ID: 7f0a89b2-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 7f0a89b2-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:45 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:44 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126035" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:42 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:31 +0800 Message-Id: <1569506015-26938-4-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> MIME-Version: 1.0 Subject: [Xen-devel] [PATCH v11 3/7] microcode: reduce memory allocation and copy when creating a patch X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Ashok Raj , Wei Liu , Andrew Cooper , Jan Beulich , Chao Gao , =?utf-8?q?R?= =?utf-8?q?oger_Pau_Monn=C3=A9?= Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Reviewed-by: Roger Pau MonnĂ© Reviewed-by: Jan Beulich --- 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); From patchwork Thu Sep 26 13:53:32 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162799 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 3F53417D4 for ; Thu, 26 Sep 2019 13:51:28 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 198EA207FF for ; Thu, 26 Sep 2019 13:51:28 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 198EA207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9h-0002Bb-R0; Thu, 26 Sep 2019 13:50:01 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9g-00027m-8T for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:50:00 +0000 X-Inumbo-ID: 80e4a5a6-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 80e4a5a6-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:47 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:47 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126045" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:44 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:32 +0800 Message-Id: <1569506015-26938-5-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 4/7] x86/microcode: Synchronize late microcode loading X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Kevin Tian , Borislav Petkov , Ashok Raj , Wei Liu , Jun Nakajima , Andrew Cooper , Jan Beulich , Thomas Gleixner , Chao Gao , =?utf-8?q?Roger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Tested-by: Chao Gao [linux commit: a5321aec6412b20b5ad15db2d6b916c05349dbff] [linux commit: bb8c13d61a629276a162c1d2b1a20a815cbcfbb7] Cc: Kevin Tian Cc: Jun Nakajima Cc: Ashok Raj Cc: Borislav Petkov Cc: Thomas Gleixner Cc: Andrew Cooper Cc: Jan Beulich Reviewed-by: Jan Beulich --- 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 #include #include +#include #include #include #include +#include +#include #include #include #include #include +/* + * 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) From patchwork Thu Sep 26 13:53:33 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162789 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 792D217EE for ; Thu, 26 Sep 2019 13:51:21 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5F0E721A4A for ; Thu, 26 Sep 2019 13:51:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5F0E721A4A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9m-0002Vt-8w; Thu, 26 Sep 2019 13:50:06 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9l-0002Od-8E for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:50:05 +0000 X-Inumbo-ID: 8261c4fe-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 8261c4fe-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:50 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:49 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126059" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:47 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:33 +0800 Message-Id: <1569506015-26938-6-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 5/7] microcode: remove microcode_update_lock X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Ashok Raj , Wei Liu , Andrew Cooper , Jan Beulich , Chao Gao , =?utf-8?q?R?= =?utf-8?q?oger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Reviewed-by: Jan Beulich --- 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 " From patchwork Thu Sep 26 13:53:34 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162793 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 1D18117D4 for ; Thu, 26 Sep 2019 13:51:24 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EA06221A4A for ; Thu, 26 Sep 2019 13:51:23 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EA06221A4A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9q-0002ky-Ke; Thu, 26 Sep 2019 13:50:10 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9q-0002kZ-8m for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:50:10 +0000 X-Inumbo-ID: 846ce508-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 846ce508-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:53 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:53 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126069" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:50 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:34 +0800 Message-Id: <1569506015-26938-7-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 6/7] microcode: rendezvous CPUs in NMI handler and load ucode X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , 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 , =?utf-8?q?Roger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 Signed-off-by: Chao Gao --- 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) -> `= [ | scan]` +> `= List of [ | scan, nmi= ]` 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) > `= ` 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 #include +#include #include #include +#include #include #include #include @@ -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 '[|scan]'. Both options are optional. + * The format is '[|scan, nmi=]'. 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= 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); From patchwork Thu Sep 26 13:53:35 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Chao Gao X-Patchwork-Id: 11162797 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 5198617D4 for ; Thu, 26 Sep 2019 13:51:25 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 3699B207FF for ; Thu, 26 Sep 2019 13:51:25 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 3699B207FF Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9v-0002sQ-V9; Thu, 26 Sep 2019 13:50:15 +0000 Received: from us1-rack-iad1.inumbo.com ([172.99.69.81]) by lists.xenproject.org with esmtp (Exim 4.89) (envelope-from ) id 1iDU9v-0002rM-9Z for xen-devel@lists.xenproject.org; Thu, 26 Sep 2019 13:50:15 +0000 X-Inumbo-ID: 86042980-e064-11e9-8628-bc764e2007e4 Received: from mga04.intel.com (unknown [192.55.52.120]) by localhost (Halon) with ESMTPS id 86042980-e064-11e9-8628-bc764e2007e4; Thu, 26 Sep 2019 13:49:56 +0000 (UTC) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 26 Sep 2019 06:49:56 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,552,1559545200"; d="scan'208";a="189126073" Received: from gao-cwp.sh.intel.com ([10.239.159.26]) by fmsmga008.fm.intel.com with ESMTP; 26 Sep 2019 06:49:53 -0700 From: Chao Gao To: xen-devel@lists.xenproject.org Date: Thu, 26 Sep 2019 21:53:35 +0800 Message-Id: <1569506015-26938-8-git-send-email-chao.gao@intel.com> X-Mailer: git-send-email 1.9.1 In-Reply-To: <1569506015-26938-1-git-send-email-chao.gao@intel.com> References: <1569506015-26938-1-git-send-email-chao.gao@intel.com> Subject: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked X-BeenThere: xen-devel@lists.xenproject.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Cc: Sergey Dyasli , Ashok Raj , Wei Liu , Andrew Cooper , Jan Beulich , Chao Gao , =?utf-8?q?R?= =?utf-8?q?oger_Pau_Monn=C3=A9?= MIME-Version: 1.0 Errors-To: xen-devel-bounces@lists.xenproject.org Sender: "Xen-devel" 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 --- 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);