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 */