* [Xen-devel] [PATCH v8 01/16] misc/xen-ucode: Upload a microcode blob to the hypervisor
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot Chao Gao
` (16 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Konrad Rzeszutek Wilk, Ian Jackson, Ashok Raj, Wei Liu, Chao Gao
This patch provides a tool for late microcode update.
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes in v8:
- Correct two coding style issues. No functional changes.
Changes in v7:
- introduce xc_microcode_update() rather than xc_platform_op()
- avoid creating bounce buffer twice
- rename xenmicrocode to xen-ucode, following naming tradition
of other tools there.
---
tools/libxc/include/xenctrl.h | 1 +
tools/libxc/xc_misc.c | 23 ++++++++++++++
tools/misc/Makefile | 4 +++
tools/misc/xen-ucode.c | 73 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 101 insertions(+)
create mode 100644 tools/misc/xen-ucode.c
diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 0a7d894..0ff6ed9 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1244,6 +1244,7 @@ typedef uint32_t xc_node_to_node_dist_t;
int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
xc_cputopo_t *cputopo);
+int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
xc_meminfo_t *meminfo, uint32_t *distance);
int xc_pcitopoinfo(xc_interface *xch, unsigned num_devs,
diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c
index 5e6714a..8e60b6e 100644
--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -226,6 +226,29 @@ int xc_physinfo(xc_interface *xch,
return 0;
}
+int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
+{
+ int ret;
+ DECLARE_PLATFORM_OP;
+ DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
+
+ uc = xc_hypercall_buffer_alloc(xch, uc, len);
+ if ( uc == NULL )
+ return -1;
+
+ memcpy(uc, buf, len);
+
+ platform_op.cmd = XENPF_microcode_update;
+ platform_op.u.microcode.length = len;
+ set_xen_guest_handle(platform_op.u.microcode.data, uc);
+
+ ret = do_platform_op(xch, &platform_op);
+
+ xc_hypercall_buffer_free(xch, uc);
+
+ return ret;
+}
+
int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
xc_cputopo_t *cputopo)
{
diff --git a/tools/misc/Makefile b/tools/misc/Makefile
index d4320dc..63947bf 100644
--- a/tools/misc/Makefile
+++ b/tools/misc/Makefile
@@ -22,6 +22,7 @@ INSTALL_SBIN-$(CONFIG_X86) += xen-hvmcrash
INSTALL_SBIN-$(CONFIG_X86) += xen-hvmctx
INSTALL_SBIN-$(CONFIG_X86) += xen-lowmemd
INSTALL_SBIN-$(CONFIG_X86) += xen-mfndump
+INSTALL_SBIN-$(CONFIG_X86) += xen-ucode
INSTALL_SBIN += xencov
INSTALL_SBIN += xenlockprof
INSTALL_SBIN += xenperf
@@ -113,4 +114,7 @@ xen-lowmemd: xen-lowmemd.o
xencov: xencov.o
$(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+xen-ucode: xen-ucode.o
+ $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS)
+
-include $(DEPS_INCLUDE)
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
new file mode 100644
index 0000000..2b14194
--- /dev/null
+++ b/tools/misc/xen-ucode.c
@@ -0,0 +1,73 @@
+#define _GNU_SOURCE
+
+#include <stdio.h>
+#include <stdlib.h>
+#include <sys/mman.h>
+#include <errno.h>
+#include <string.h>
+#include <inttypes.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <fcntl.h>
+#include <xenctrl.h>
+
+int main(int argc, char *argv[])
+{
+ int fd, len, ret;
+ char *filename, *buf;
+ struct stat st;
+ xc_interface *xch;
+
+ if (argc < 2)
+ {
+ fprintf(stderr,
+ "xen-ucode: Xen microcode updating tool\n"
+ "Usage: %s <microcode blob>\n", argv[0]);
+ return 0;
+ }
+
+ filename = argv[1];
+ fd = open(filename, O_RDONLY);
+ if (fd < 0) {
+ fprintf(stderr, "Could not open %s. (err: %s)\n",
+ filename, strerror(errno));
+ return errno;
+ }
+
+ if (stat(filename, &st) != 0) {
+ fprintf(stderr, "Could not get the size of %s. (err: %s)\n",
+ filename, strerror(errno));
+ return errno;
+ }
+
+ len = st.st_size;
+ buf = mmap(0, len, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (buf == MAP_FAILED) {
+ fprintf(stderr, "mmap failed. (error: %s)\n", strerror(errno));
+ return errno;
+ }
+
+ xch = xc_interface_open(0,0,0);
+ if (xch == NULL)
+ {
+ fprintf(stderr, "Error opening xc interface. (err: %s)\n",
+ strerror(errno));
+ return errno;
+ }
+
+ ret = xc_microcode_update(xch, buf, len);
+ if (ret)
+ fprintf(stderr, "Failed to update microcode. (err: %s)\n",
+ strerror(errno));
+
+ xc_interface_close(xch);
+
+ if (munmap(buf, len)) {
+ printf("Could not unmap: %d(%s)\n", errno, strerror(errno));
+ return errno;
+ }
+ close(fd);
+
+ return 0;
+}
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 01/16] misc/xen-ucode: Upload a microcode blob to the hypervisor Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-01 10:35 ` Andrew Cooper
2019-08-02 13:23 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match() Chao Gao
` (15 subsequent siblings)
17 siblings, 2 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
Chao Gao, Roger Pau Monné
From: Sergey Dyasli <sergey.dyasli@citrix.com>
Currently cpu_sig struct is not updated during boot if no microcode blob
is specified by "ucode=[<interger>| scan]".
It will result in cpu_sig.rev being 0 which affects APIC's
check_deadline_errata() and retpoline_safe() functions.
Fix this by getting ucode revision early during boot and SMP bring up.
While at it, protect early_microcode_update_cpu() for cases when
microcode_ops is NULL.
Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- refine description.
- Jan asked if we could drop the call of collect_cpu_info() from
microcode_update_cpu(). In theory, yes, but should be placed later
in the series. Because there is an error path (__microcode_fini_cpu())
in which cpu_sig.rev is cleared, it is hard to make things right
in all cases without removing the error path (which is done by following
patches). Considering it is a good fix, put it here so that it can
be merged without following patches.
---
xen/arch/x86/microcode.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 4163f50..421d57e 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -383,10 +383,15 @@ static struct notifier_block microcode_percpu_nfb = {
int __init early_microcode_update_cpu(bool start_update)
{
+ unsigned int cpu = smp_processor_id();
+ struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
int rc = 0;
void *data = NULL;
size_t len;
+ if ( !microcode_ops )
+ return -ENOSYS;
+
if ( ucode_blob.size )
{
len = ucode_blob.size;
@@ -397,6 +402,9 @@ int __init early_microcode_update_cpu(bool start_update)
len = ucode_mod.mod_end;
data = bootstrap_map(&ucode_mod);
}
+
+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
if ( data )
{
if ( start_update && microcode_ops->start_update )
@@ -413,6 +421,8 @@ int __init early_microcode_update_cpu(bool start_update)
int __init early_microcode_init(void)
{
+ unsigned int cpu = smp_processor_id();
+ struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
int rc;
rc = microcode_init_intel();
@@ -425,6 +435,8 @@ int __init early_microcode_init(void)
if ( microcode_ops )
{
+ microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+
if ( ucode_mod.mod_end || ucode_blob.size )
rc = early_microcode_update_cpu(true);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot Chao Gao
@ 2019-08-01 10:35 ` Andrew Cooper
2019-08-02 13:23 ` Jan Beulich
1 sibling, 0 replies; 49+ messages in thread
From: Andrew Cooper @ 2019-08-01 10:35 UTC (permalink / raw)
To: Chao Gao, xen-devel
Cc: Sergey Dyasli, Jan Beulich, Ashok Raj, Wei Liu, Roger Pau Monné
On 01/08/2019 11:22, Chao Gao wrote:
> From: Sergey Dyasli <sergey.dyasli@citrix.com>
>
> Currently cpu_sig struct is not updated during boot if no microcode blob
> is specified by "ucode=[<interger>| scan]".
>
> It will result in cpu_sig.rev being 0 which affects APIC's
> check_deadline_errata() and retpoline_safe() functions.
>
> Fix this by getting ucode revision early during boot and SMP bring up.
> While at it, protect early_microcode_update_cpu() for cases when
> microcode_ops is NULL.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> Changes in v8:
> - refine description.
> - Jan asked if we could drop the call of collect_cpu_info() from
> microcode_update_cpu(). In theory, yes, but should be placed later
> in the series. Because there is an error path (__microcode_fini_cpu())
> in which cpu_sig.rev is cleared, it is hard to make things right
> in all cases without removing the error path (which is done by following
> patches). Considering it is a good fix, put it here so that it can
> be merged without following patches.
Ok.
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>, and this does
really want backporting.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot Chao Gao
2019-08-01 10:35 ` Andrew Cooper
@ 2019-08-02 13:23 ` Jan Beulich
1 sibling, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 13:23 UTC (permalink / raw)
To: Chao Gao, Sergey Dyasli
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -383,10 +383,15 @@ static struct notifier_block microcode_percpu_nfb = {
>
> int __init early_microcode_update_cpu(bool start_update)
> {
> + unsigned int cpu = smp_processor_id();
> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> int rc = 0;
> void *data = NULL;
> size_t len;
>
> + if ( !microcode_ops )
> + return -ENOSYS;
For posterity (realizing the patch has gone in already) - I'm not
convinced this should be an error condition, but the error
produced here is being ignored anyway afaics (i.e. the function
could as well return void). In no case is this an appropriate use
of ENOSYS: This isn't even on a hypercall path.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 01/16] misc/xen-ucode: Upload a microcode blob to the hypervisor Chao Gao
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 02/16] x86/microcode: always collect_cpu_info() during boot Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 13:29 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 04/16] microcode/amd: fix memory leak Chao Gao
` (14 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
to a more generic function. Then, this function can compare two given
microcodes' signature/revision as well. Comparing two microcodes is
used to update the global microcode cache (introduced by the later
patches in this series) when a new microcode is given.
Note that enum microcode_match_result will be used in common code
(aka microcode.c), it has been placed in the common header.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes in v8:
- make sure enough room for an extended header and signature array
Changes in v6:
- eliminate unnecessary type casting in microcode_update_match
- check if a patch has an extend header
Changes in v5:
- constify the extended_signature
- use named enum type for the return value of microcode_update_match
---
xen/arch/x86/microcode_intel.c | 57 ++++++++++++++++++++++-------------------
xen/include/asm-x86/microcode.h | 6 +++++
2 files changed, 36 insertions(+), 27 deletions(-)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 22fdeca..644660d 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
return 0;
}
-static inline int microcode_update_match(
- unsigned int cpu_num, const struct microcode_header_intel *mc_header,
- int sig, int pf)
+static enum microcode_match_result microcode_update_match(
+ const struct microcode_header_intel *mc_header, unsigned int sig,
+ unsigned int pf, unsigned int rev)
{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
-
- return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
- (mc_header->rev > uci->cpu_sig.rev));
+ const struct extended_sigtable *ext_header;
+ const struct extended_signature *ext_sig;
+ unsigned long data_size = get_datasize(mc_header);
+ unsigned int i;
+ const void *end = (const void *)mc_header + get_totalsize(mc_header);
+
+ if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
+ return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+ ext_header = (const void *)(mc_header + 1) + data_size;
+ ext_sig = (const void *)(ext_header + 1);
+
+ /*
+ * Make sure there is enough space to hold an extended header and enough
+ * array elements.
+ */
+ if ( (end < (const void *)ext_sig) ||
+ (end < (const void *)(ext_sig + ext_header->count)) )
+ return MIS_UCODE;
+
+ for ( i = 0; i < ext_header->count; i++ )
+ if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
+ return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+ return MIS_UCODE;
}
static int microcode_sanity_check(void *mc)
@@ -243,31 +264,13 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
{
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
const struct microcode_header_intel *mc_header = mc;
- const struct extended_sigtable *ext_header;
unsigned long total_size = get_totalsize(mc_header);
- int ext_sigcount, i;
- struct extended_signature *ext_sig;
void *new_mc;
- if ( microcode_update_match(cpu, mc_header,
- mc_header->sig, mc_header->pf) )
- goto find;
-
- if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) )
+ if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf,
+ uci->cpu_sig.rev) != NEW_UCODE )
return 0;
- ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE;
- ext_sigcount = ext_header->count;
- ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
- for ( i = 0; i < ext_sigcount; i++ )
- {
- if ( microcode_update_match(cpu, mc_header,
- ext_sig->sig, ext_sig->pf) )
- goto find;
- ext_sig++;
- }
- return 0;
- find:
pr_debug("microcode: CPU%d found a matching microcode update with"
" version %#x (current=%#x)\n",
cpu, mc_header->rev, uci->cpu_sig.rev);
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 23ea954..882f560 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -3,6 +3,12 @@
#include <xen/percpu.h>
+enum microcode_match_result {
+ OLD_UCODE, /* signature matched, but revision id is older or equal */
+ NEW_UCODE, /* signature matched, but revision id is newer */
+ MIS_UCODE, /* signature mismatched */
+};
+
struct cpu_signature;
struct ucode_cpu_info;
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match() Chao Gao
@ 2019-08-02 13:29 ` Jan Beulich
2019-08-05 5:58 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 13:29 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
> return 0;
> }
>
> -static inline int microcode_update_match(
> - unsigned int cpu_num, const struct microcode_header_intel *mc_header,
> - int sig, int pf)
> +static enum microcode_match_result microcode_update_match(
> + const struct microcode_header_intel *mc_header, unsigned int sig,
> + unsigned int pf, unsigned int rev)
> {
> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
> -
> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
> - (mc_header->rev > uci->cpu_sig.rev));
> + const struct extended_sigtable *ext_header;
> + const struct extended_signature *ext_sig;
> + unsigned long data_size = get_datasize(mc_header);
> + unsigned int i;
> + const void *end = (const void *)mc_header + get_totalsize(mc_header);
> +
> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
Both here and ...
> + ext_header = (const void *)(mc_header + 1) + data_size;
> + ext_sig = (const void *)(ext_header + 1);
> +
> + /*
> + * Make sure there is enough space to hold an extended header and enough
> + * array elements.
> + */
> + if ( (end < (const void *)ext_sig) ||
> + (end < (const void *)(ext_sig + ext_header->count)) )
> + return MIS_UCODE;
> +
> + for ( i = 0; i < ext_header->count; i++ )
> + if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
... here there's again an assumption that there's strict ordering
between blobs, but as mentioned in reply to a later patch of an
earlier version this isn't the case. Therefore the function can't
be used to compare two arbitrary blobs, it may only be used to
compare a blob with what is already loaded into a CPU. I think it
is quite important to mention this restriction in a comment ahead
of the function.
The code itself looks fine to me, and a comment could perhaps be
added while committing; with such a comment
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-02 13:29 ` Jan Beulich
@ 2019-08-05 5:58 ` Chao Gao
2019-08-05 9:27 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-05 5:58 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On Fri, Aug 02, 2019 at 01:29:14PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>> return 0;
>> }
>>
>> -static inline int microcode_update_match(
>> - unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>> - int sig, int pf)
>> +static enum microcode_match_result microcode_update_match(
>> + const struct microcode_header_intel *mc_header, unsigned int sig,
>> + unsigned int pf, unsigned int rev)
>> {
>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> -
>> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>> - (mc_header->rev > uci->cpu_sig.rev));
>> + const struct extended_sigtable *ext_header;
>> + const struct extended_signature *ext_sig;
>> + unsigned long data_size = get_datasize(mc_header);
>> + unsigned int i;
>> + const void *end = (const void *)mc_header + get_totalsize(mc_header);
>> +
>> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>Both here and ...
>
>> + ext_header = (const void *)(mc_header + 1) + data_size;
>> + ext_sig = (const void *)(ext_header + 1);
>> +
>> + /*
>> + * Make sure there is enough space to hold an extended header and enough
>> + * array elements.
>> + */
>> + if ( (end < (const void *)ext_sig) ||
>> + (end < (const void *)(ext_sig + ext_header->count)) )
>> + return MIS_UCODE;
>> +
>> + for ( i = 0; i < ext_header->count; i++ )
>> + if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>... here there's again an assumption that there's strict ordering
>between blobs, but as mentioned in reply to a later patch of an
>earlier version this isn't the case. Therefore the function can't
>be used to compare two arbitrary blobs, it may only be used to
>compare a blob with what is already loaded into a CPU. I think it
>is quite important to mention this restriction in a comment ahead
>of the function.
>
>The code itself looks fine to me, and a comment could perhaps be
>added while committing; with such a comment
Agree. Because there will be a version 9, I can add a comment.
>Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks.
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-05 5:58 ` Chao Gao
@ 2019-08-05 9:27 ` Jan Beulich
2019-08-05 11:51 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 9:27 UTC (permalink / raw)
To: Chao Gao
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 05.08.2019 07:58, Chao Gao wrote:
> On Fri, Aug 02, 2019 at 01:29:14PM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> --- a/xen/arch/x86/microcode_intel.c
>>> +++ b/xen/arch/x86/microcode_intel.c
>>> @@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>>> return 0;
>>> }
>>>
>>> -static inline int microcode_update_match(
>>> - unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>>> - int sig, int pf)
>>> +static enum microcode_match_result microcode_update_match(
>>> + const struct microcode_header_intel *mc_header, unsigned int sig,
>>> + unsigned int pf, unsigned int rev)
>>> {
>>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>>> -
>>> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>>> - (mc_header->rev > uci->cpu_sig.rev));
>>> + const struct extended_sigtable *ext_header;
>>> + const struct extended_signature *ext_sig;
>>> + unsigned long data_size = get_datasize(mc_header);
>>> + unsigned int i;
>>> + const void *end = (const void *)mc_header + get_totalsize(mc_header);
>>> +
>>> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>
>> Both here and ...
>>
>>> + ext_header = (const void *)(mc_header + 1) + data_size;
>>> + ext_sig = (const void *)(ext_header + 1);
>>> +
>>> + /*
>>> + * Make sure there is enough space to hold an extended header and enough
>>> + * array elements.
>>> + */
>>> + if ( (end < (const void *)ext_sig) ||
>>> + (end < (const void *)(ext_sig + ext_header->count)) )
>>> + return MIS_UCODE;
>>> +
>>> + for ( i = 0; i < ext_header->count; i++ )
>>> + if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>
>> ... here there's again an assumption that there's strict ordering
>> between blobs, but as mentioned in reply to a later patch of an
>> earlier version this isn't the case. Therefore the function can't
>> be used to compare two arbitrary blobs, it may only be used to
>> compare a blob with what is already loaded into a CPU. I think it
>> is quite important to mention this restriction in a comment ahead
>> of the function.
>>
>> The code itself looks fine to me, and a comment could perhaps be
>> added while committing; with such a comment
>
> Agree. Because there will be a version 9, I can add a comment.
Having seen the uses in later patches, I think a comment is not the
way to go. Instead I think you want to first match _both_
signatures against the local CPU (unless e.g. for either side this
is logically guaranteed), and return DIS_UCODE upon mismatch. Only
then should you actually compare the two signatures. While from a
pure, abstract patch ordering perspective this isn't correct, we
only care about patches applicable to the local CPU anyway, and for
that case the extra restriction is fine. This way you'll be able to
avoid taking extra precautions in vendor-independent code just to
accommodate an Intel specific requirement.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-05 9:27 ` Jan Beulich
@ 2019-08-05 11:51 ` Chao Gao
2019-08-05 11:50 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-05 11:51 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On Mon, Aug 05, 2019 at 09:27:58AM +0000, Jan Beulich wrote:
>On 05.08.2019 07:58, Chao Gao wrote:
>> On Fri, Aug 02, 2019 at 01:29:14PM +0000, Jan Beulich wrote:
>>> On 01.08.2019 12:22, Chao Gao wrote:
>>>> --- a/xen/arch/x86/microcode_intel.c
>>>> +++ b/xen/arch/x86/microcode_intel.c
>>>> @@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>>>> return 0;
>>>> }
>>>>
>>>> -static inline int microcode_update_match(
>>>> - unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>>>> - int sig, int pf)
>>>> +static enum microcode_match_result microcode_update_match(
>>>> + const struct microcode_header_intel *mc_header, unsigned int sig,
>>>> + unsigned int pf, unsigned int rev)
>>>> {
>>>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>>>> -
>>>> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>>>> - (mc_header->rev > uci->cpu_sig.rev));
>>>> + const struct extended_sigtable *ext_header;
>>>> + const struct extended_signature *ext_sig;
>>>> + unsigned long data_size = get_datasize(mc_header);
>>>> + unsigned int i;
>>>> + const void *end = (const void *)mc_header + get_totalsize(mc_header);
>>>> +
>>>> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>>
>>> Both here and ...
>>>
>>>> + ext_header = (const void *)(mc_header + 1) + data_size;
>>>> + ext_sig = (const void *)(ext_header + 1);
>>>> +
>>>> + /*
>>>> + * Make sure there is enough space to hold an extended header and enough
>>>> + * array elements.
>>>> + */
>>>> + if ( (end < (const void *)ext_sig) ||
>>>> + (end < (const void *)(ext_sig + ext_header->count)) )
>>>> + return MIS_UCODE;
>>>> +
>>>> + for ( i = 0; i < ext_header->count; i++ )
>>>> + if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>>
>>> ... here there's again an assumption that there's strict ordering
>>> between blobs, but as mentioned in reply to a later patch of an
>>> earlier version this isn't the case. Therefore the function can't
>>> be used to compare two arbitrary blobs, it may only be used to
>>> compare a blob with what is already loaded into a CPU. I think it
>>> is quite important to mention this restriction in a comment ahead
>>> of the function.
>>>
>>> The code itself looks fine to me, and a comment could perhaps be
>>> added while committing; with such a comment
>>
>> Agree. Because there will be a version 9, I can add a comment.
>
>Having seen the uses in later patches, I think a comment is not the
>way to go. Instead I think you want to first match _both_
>signatures against the local CPU (unless e.g. for either side this
>is logically guaranteed),
Yes. It is guaranteed at the first place: we ignore any patch that
doesn't match with the CPU signature when parsing the ucode blob.
>and return DIS_UCODE upon mismatch. Only
>then should you actually compare the two signatures. While from a
>pure, abstract patch ordering perspective this isn't correct, we
>only care about patches applicable to the local CPU anyway, and for
>that case the extra restriction is fine. This way you'll be able to
>avoid taking extra precautions in vendor-independent code just to
>accommodate an Intel specific requirement.
Yes. I agree and it seems that no further change is needed except
the implementation of ->compare_patch. Please correct me if I am wrong.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match()
2019-08-05 11:51 ` Chao Gao
@ 2019-08-05 11:50 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 11:50 UTC (permalink / raw)
To: Chao Gao
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 05.08.2019 13:51, Chao Gao wrote:
> On Mon, Aug 05, 2019 at 09:27:58AM +0000, Jan Beulich wrote:
>> On 05.08.2019 07:58, Chao Gao wrote:
>>> On Fri, Aug 02, 2019 at 01:29:14PM +0000, Jan Beulich wrote:
>>>> On 01.08.2019 12:22, Chao Gao wrote:
>>>>> --- a/xen/arch/x86/microcode_intel.c
>>>>> +++ b/xen/arch/x86/microcode_intel.c
>>>>> @@ -134,14 +134,35 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>>>>> return 0;
>>>>> }
>>>>>
>>>>> -static inline int microcode_update_match(
>>>>> - unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>>>>> - int sig, int pf)
>>>>> +static enum microcode_match_result microcode_update_match(
>>>>> + const struct microcode_header_intel *mc_header, unsigned int sig,
>>>>> + unsigned int pf, unsigned int rev)
>>>>> {
>>>>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>>>>> -
>>>>> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>>>>> - (mc_header->rev > uci->cpu_sig.rev));
>>>>> + const struct extended_sigtable *ext_header;
>>>>> + const struct extended_signature *ext_sig;
>>>>> + unsigned long data_size = get_datasize(mc_header);
>>>>> + unsigned int i;
>>>>> + const void *end = (const void *)mc_header + get_totalsize(mc_header);
>>>>> +
>>>>> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>>>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>>>
>>>> Both here and ...
>>>>
>>>>> + ext_header = (const void *)(mc_header + 1) + data_size;
>>>>> + ext_sig = (const void *)(ext_header + 1);
>>>>> +
>>>>> + /*
>>>>> + * Make sure there is enough space to hold an extended header and enough
>>>>> + * array elements.
>>>>> + */
>>>>> + if ( (end < (const void *)ext_sig) ||
>>>>> + (end < (const void *)(ext_sig + ext_header->count)) )
>>>>> + return MIS_UCODE;
>>>>> +
>>>>> + for ( i = 0; i < ext_header->count; i++ )
>>>>> + if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>>>>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>>>
>>>> ... here there's again an assumption that there's strict ordering
>>>> between blobs, but as mentioned in reply to a later patch of an
>>>> earlier version this isn't the case. Therefore the function can't
>>>> be used to compare two arbitrary blobs, it may only be used to
>>>> compare a blob with what is already loaded into a CPU. I think it
>>>> is quite important to mention this restriction in a comment ahead
>>>> of the function.
>>>>
>>>> The code itself looks fine to me, and a comment could perhaps be
>>>> added while committing; with such a comment
>>>
>>> Agree. Because there will be a version 9, I can add a comment.
>>
>> Having seen the uses in later patches, I think a comment is not the
>> way to go. Instead I think you want to first match _both_
>> signatures against the local CPU (unless e.g. for either side this
>> is logically guaranteed),
>
> Yes. It is guaranteed at the first place: we ignore any patch that
> doesn't match with the CPU signature when parsing the ucode blob.
Well, if that's the case, then perhaps a comment is really all
that's needed, i.e. ...
>> and return DIS_UCODE upon mismatch. Only
>> then should you actually compare the two signatures. While from a
>> pure, abstract patch ordering perspective this isn't correct, we
>> only care about patches applicable to the local CPU anyway, and for
>> that case the extra restriction is fine. This way you'll be able to
>> avoid taking extra precautions in vendor-independent code just to
>> accommodate an Intel specific requirement.
>
> Yes. I agree and it seems that no further change is needed except
> the implementation of ->compare_patch. Please correct me if I am wrong.
... maybe not even the change here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 04/16] microcode/amd: fix memory leak
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (2 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 03/16] microcode/intel: extend microcode_update_match() Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 13:39 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 05/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
` (13 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Two buffers, '->equiv_cpu_table' and '->mpb', inside 'mc_amd' might be
allocated and in the error-handing path they are not freed properly.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
changes in v8:
- new
- it is found by reading code. No test is done.
---
xen/arch/x86/microcode_amd.c | 14 +++++++++++---
1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 7a854c0..afca51f 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -433,6 +433,9 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
goto out;
}
+ mc_amd->equiv_cpu_table_size = 0;
+ mc_amd->equiv_cpu_table = NULL;
+
/*
* Multiple container file support:
* 1. check if this container file has equiv_cpu_id match
@@ -479,6 +482,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
if ( error )
{
+ if ( mc_amd->equiv_cpu_table_size )
+ xfree(mc_amd->equiv_cpu_table);
xfree(mc_amd);
goto out;
}
@@ -549,11 +554,14 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
if ( save_error )
{
- xfree(mc_amd);
uci->mc.mc_amd = mc_old;
+ mc_old = mc_amd;
}
- else
- xfree(mc_old);
+
+ if ( mc_old->mpb_size )
+ xfree(mc_old->mpb);
+ xfree(mc_old->equiv_cpu_table);
+ xfree(mc_old);
out:
#if CONFIG_HVM
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 04/16] microcode/amd: fix memory leak
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 04/16] microcode/amd: fix memory leak Chao Gao
@ 2019-08-02 13:39 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 13:39 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -433,6 +433,9 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
> goto out;
> }
>
> + mc_amd->equiv_cpu_table_size = 0;
> + mc_amd->equiv_cpu_table = NULL;
Instead of adding these, you could as well use xzalloc()
further up and drop the explicit initialization of ->mpb and
->mpb_size to NULL/0 a few lines down.
> @@ -479,6 +482,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>
> if ( error )
> {
> + if ( mc_amd->equiv_cpu_table_size )
> + xfree(mc_amd->equiv_cpu_table);
Why the if()? There's no problem calling xfree() with a NULL
argument.
> @@ -549,11 +554,14 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>
> if ( save_error )
> {
> - xfree(mc_amd);
> uci->mc.mc_amd = mc_old;
> + mc_old = mc_amd;
> }
> - else
> - xfree(mc_old);
> +
> + if ( mc_old->mpb_size )
> + xfree(mc_old->mpb);
> + xfree(mc_old->equiv_cpu_table);
Same here. With the adjustments made (could possibly be done
again while committing)
Reviewed-by: Jan Beulich <jbeulich@suse.com>
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 05/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits()
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (3 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 04/16] microcode/amd: fix memory leak Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 13:41 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch Chao Gao
` (12 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Sometimes, an ucode with a level lower than or equal to current CPU's
patch level is useful. For example, to work around a broken bios which
only loads ucode for BSP, when BSP parses an ucode blob during bootup,
it is better to save an ucode with lower or equal level for APs
No functional change is made in this patch. But following patch would
handle "old ucode" and "mismatched ucode" separately.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- new
---
xen/arch/x86/microcode_amd.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index afca51f..e9a567f 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -152,8 +152,8 @@ static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
return 0;
}
-static bool_t microcode_fits(const struct microcode_amd *mc_amd,
- unsigned int cpu)
+static enum microcode_match_result microcode_fits(
+ const struct microcode_amd *mc_amd, unsigned int cpu)
{
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
@@ -167,27 +167,27 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd,
current_cpu_id = cpuid_eax(0x00000001);
if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) )
- return 0;
+ return MIS_UCODE;
if ( (mc_header->processor_rev_id) != equiv_cpu_id )
- return 0;
+ return MIS_UCODE;
if ( !verify_patch_size(mc_amd->mpb_size) )
{
pr_debug("microcode: patch size mismatch\n");
- return 0;
+ return MIS_UCODE;
}
if ( mc_header->patch_id <= uci->cpu_sig.rev )
{
pr_debug("microcode: patch is already at required level or greater.\n");
- return 0;
+ return OLD_UCODE;
}
pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n",
cpu, mc_header->patch_id, uci->cpu_sig.rev);
- return 1;
+ return NEW_UCODE;
}
static int apply_microcode(unsigned int cpu)
@@ -502,7 +502,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
&offset)) == 0 )
{
- if ( microcode_fits(mc_amd, cpu) )
+ if ( microcode_fits(mc_amd, cpu) == NEW_UCODE )
{
error = apply_microcode(cpu);
if ( error )
@@ -583,7 +583,7 @@ static int microcode_resume_match(unsigned int cpu, const void *mc)
struct microcode_amd *mc_amd = uci->mc.mc_amd;
const struct microcode_amd *src = mc;
- if ( !microcode_fits(src, cpu) )
+ if ( microcode_fits(src, cpu) != NEW_UCODE )
return 0;
if ( src != mc_amd )
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 05/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits()
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 05/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
@ 2019-08-02 13:41 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 13:41 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> Sometimes, an ucode with a level lower than or equal to current CPU's
> patch level is useful. For example, to work around a broken bios which
> only loads ucode for BSP, when BSP parses an ucode blob during bootup,
> it is better to save an ucode with lower or equal level for APs
>
> No functional change is made in this patch. But following patch would
> handle "old ucode" and "mismatched ucode" separately.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (4 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 05/16] microcode/amd: distinguish old and mismatched ucode in microcode_fits() Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 14:46 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 07/16] microcode: clean up microcode_resume_cpu Chao Gao
` (11 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
to replace the current per-cpu cache 'uci->mc'.
With the assumption that all CPUs in the system have the same signature
(family, model, stepping and 'pf'), one microcode update matches with
one cpu should match with others. Having multiple microcode revisions
on different cpus would cause system unstable and should be avoided.
Hence, caching only one microcode update is good enough for all cases.
Introduce a global variable, microcode_cache, to store the newest
matching microcode update. Whenever we get a new valid microcode update,
its revision id is compared against that of the microcode update to
determine whether the "microcode_cache" needs to be replaced. And
this global cache is loaded to cpu in apply_microcode().
All operations on the cache is protected by 'microcode_mutex'.
Note that I deliberately avoid touching 'uci->mc' as I am going to
remove it completely in the next patch.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- Free generic wrapper struct in general code
- Try to update cache as long as a patch covers current cpu. Previsouly,
cache is updated only if the patch is newer than current update revision in
the CPU. The small difference can work around a broken bios which only
applies microcode update to BSP and software has to apply the same
update to other CPUs.
Changes in v7:
- reworked to cache only one microcode patch rather than a list of
microcode patches.
---
xen/arch/x86/microcode.c | 39 ++++++++++++++++++
xen/arch/x86/microcode_amd.c | 90 +++++++++++++++++++++++++++++++++++++----
xen/arch/x86/microcode_intel.c | 75 ++++++++++++++++++++++++++--------
xen/include/asm-x86/microcode.h | 17 ++++++++
4 files changed, 198 insertions(+), 23 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 421d57e..a8425b8 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -61,6 +61,9 @@ static struct ucode_mod_blob __initdata ucode_blob;
*/
static bool_t __initdata ucode_scan;
+/* Protected by microcode_mutex */
+static struct microcode_patch *microcode_cache;
+
void __init microcode_set_module(unsigned int idx)
{
ucode_mod_idx = idx;
@@ -262,6 +265,42 @@ int microcode_resume_cpu(unsigned int cpu)
return err;
}
+void microcode_free_patch(struct microcode_patch *microcode_patch)
+{
+ microcode_ops->free_patch(microcode_patch->mc);
+ 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)
+{
+
+ ASSERT(spin_is_locked(µcode_mutex));
+
+ if ( !microcode_cache )
+ microcode_cache = patch;
+ else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
+ NEW_UCODE )
+ {
+ microcode_free_patch(microcode_cache);
+ microcode_cache = patch;
+ }
+ else
+ {
+ microcode_free_patch(patch);
+ return false;
+ }
+
+ return true;
+}
+
static int microcode_update_cpu(const void *buf, size_t size)
{
int err;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e9a567f..bb07e1e 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -190,24 +190,83 @@ static enum microcode_match_result microcode_fits(
return NEW_UCODE;
}
+static bool match_cpu(const struct microcode_patch *patch)
+{
+ if ( !patch )
+ return false;
+ return microcode_fits(patch->mc_amd, smp_processor_id()) == 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;
+
+ xfree(mc_amd->equiv_cpu_table);
+ xfree(mc_amd->mpb);
+ xfree(mc_amd);
+}
+
+static enum microcode_match_result compare_patch(
+ const struct microcode_patch *new, const struct microcode_patch *old)
+{
+ const struct microcode_amd *new_mc = new->mc_amd;
+ const struct microcode_header_amd *new_header = new_mc->mpb;
+ const struct microcode_amd *old_mc = old->mc_amd;
+ const struct microcode_header_amd *old_header = old_mc->mpb;
+
+ 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 int apply_microcode(unsigned int cpu)
{
unsigned long flags;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
uint32_t rev;
- struct microcode_amd *mc_amd = uci->mc.mc_amd;
- struct microcode_header_amd *hdr;
int hw_err;
+ const struct microcode_header_amd *hdr;
+ const struct microcode_patch *patch = microcode_get_cache();
/* We should bind the task to the CPU */
BUG_ON(raw_smp_processor_id() != cpu);
- if ( mc_amd == NULL )
+ if ( !match_cpu(patch) )
return -EINVAL;
- hdr = mc_amd->mpb;
- if ( hdr == NULL )
- return -EINVAL;
+ hdr = patch->mc_amd->mpb;
spin_lock_irqsave(µcode_update_lock, flags);
@@ -502,7 +561,21 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
&offset)) == 0 )
{
- if ( microcode_fits(mc_amd, cpu) == NEW_UCODE )
+ struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd);
+
+ if ( IS_ERR(new_patch) )
+ {
+ error = PTR_ERR(new_patch);
+ break;
+ }
+
+ /* Update cache if this patch covers current CPU */
+ if ( microcode_fits(new_patch->mc_amd, cpu) != MIS_UCODE )
+ microcode_update_cache(new_patch);
+ else
+ microcode_free_patch(new_patch);
+
+ if ( match_cpu(microcode_get_cache()) )
{
error = apply_microcode(cpu);
if ( error )
@@ -647,6 +720,9 @@ static const struct microcode_ops microcode_amd_ops = {
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
.start_update = start_update,
+ .free_patch = free_patch,
+ .compare_patch = compare_patch,
+ .match_cpu = match_cpu,
};
int __init microcode_init_amd(void)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 644660d..811421e 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -255,6 +255,31 @@ static int microcode_sanity_check(void *mc)
return 0;
}
+static bool match_cpu(const struct microcode_patch *patch)
+{
+ const struct ucode_cpu_info *uci = &this_cpu(ucode_cpu_info);
+
+ if ( !patch )
+ return false;
+
+ return microcode_update_match(&patch->mc_intel->hdr, uci->cpu_sig.sig,
+ uci->cpu_sig.pf, uci->cpu_sig.rev) == NEW_UCODE;
+}
+
+static void free_patch(void *mc)
+{
+ xfree(mc);
+}
+
+static enum microcode_match_result compare_patch(
+ const struct microcode_patch *new, const struct microcode_patch *old)
+{
+ const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
+
+ return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
+ old_header->pf, old_header->rev);
+}
+
/*
* return 0 - no update found
* return 1 - found update
@@ -265,11 +290,27 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
const struct microcode_header_intel *mc_header = mc;
unsigned long total_size = get_totalsize(mc_header);
- void *new_mc;
+ void *new_mc = xmalloc_bytes(total_size);
+ struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
- if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf,
- uci->cpu_sig.rev) != NEW_UCODE )
+ if ( !new_patch || !new_mc )
+ {
+ xfree(new_patch);
+ xfree(new_mc);
+ return -ENOMEM;
+ }
+ memcpy(new_mc, mc, total_size);
+ new_patch->mc_intel = new_mc;
+
+ /* Make sure that this patch covers current CPU */
+ if ( microcode_update_match(&new_patch->mc_intel->hdr, uci->cpu_sig.sig,
+ uci->cpu_sig.pf, uci->cpu_sig.rev) == 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",
@@ -294,18 +335,22 @@ static int apply_microcode(unsigned int cpu)
unsigned int val[2];
unsigned int cpu_num = raw_smp_processor_id();
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
+ const struct microcode_intel *mc_intel;
+ const struct microcode_patch *patch = microcode_get_cache();
/* We should bind the task to the CPU */
BUG_ON(cpu_num != cpu);
- if ( uci->mc.mc_intel == NULL )
+ if ( !match_cpu(patch) )
return -EINVAL;
+ mc_intel = patch->mc_intel;
+
/* serialize access to the physical write to MSR 0x79 */
spin_lock_irqsave(µcode_update_lock, flags);
/* write microcode via MSR 0x79 */
- wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)uci->mc.mc_intel->bits);
+ wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
/* As documented in the SDM: Do a CPUID 1 here */
@@ -316,19 +361,17 @@ static int apply_microcode(unsigned int cpu)
val[1] = (uint32_t)(msr_content >> 32);
spin_unlock_irqrestore(µcode_update_lock, flags);
- if ( val[1] != uci->mc.mc_intel->hdr.rev )
+ if ( val[1] != mc_intel->hdr.rev )
{
printk(KERN_ERR "microcode: CPU%d update from revision "
"%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
- uci->cpu_sig.rev, uci->mc.mc_intel->hdr.rev, val[1]);
+ uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]);
return -EIO;
}
printk(KERN_INFO "microcode: CPU%d updated from revision "
"%#x to %#x, date = %04x-%02x-%02x \n",
- cpu_num, uci->cpu_sig.rev, val[1],
- uci->mc.mc_intel->hdr.year,
- uci->mc.mc_intel->hdr.month,
- uci->mc.mc_intel->hdr.day);
+ cpu_num, uci->cpu_sig.rev, val[1], mc_intel->hdr.year,
+ mc_intel->hdr.month, mc_intel->hdr.day);
uci->cpu_sig.rev = val[1];
return 0;
@@ -368,7 +411,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
long offset = 0;
int error = 0;
void *mc;
- unsigned int matching_count = 0;
/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
@@ -386,10 +428,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
* lets keep searching till the latest version
*/
if ( error == 1 )
- {
- matching_count++;
error = 0;
- }
+
xfree(mc);
}
if ( offset > 0 )
@@ -397,7 +437,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
if ( offset < 0 )
error = offset;
- if ( !error && matching_count )
+ if ( !error && match_cpu(microcode_get_cache()) )
error = apply_microcode(cpu);
return error;
@@ -413,6 +453,9 @@ static const struct microcode_ops microcode_intel_ops = {
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
+ .free_patch = free_patch,
+ .compare_patch = compare_patch,
+ .match_cpu = match_cpu,
};
int __init microcode_init_intel(void)
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 882f560..42949b1 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -12,6 +12,14 @@ enum microcode_match_result {
struct cpu_signature;
struct ucode_cpu_info;
+struct microcode_patch {
+ union {
+ struct microcode_intel *mc_intel;
+ struct microcode_amd *mc_amd;
+ void *mc;
+ };
+};
+
struct microcode_ops {
int (*microcode_resume_match)(unsigned int cpu, const void *mc);
int (*cpu_request_microcode)(unsigned int cpu, const void *buf,
@@ -19,6 +27,11 @@ struct microcode_ops {
int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig);
int (*apply_microcode)(unsigned int cpu);
int (*start_update)(void);
+ void (*free_patch)(void *mc);
+ bool (*match_cpu)(const struct microcode_patch *patch);
+ enum microcode_match_result (*compare_patch)(
+ const struct microcode_patch *new,
+ const struct microcode_patch *old);
};
struct cpu_signature {
@@ -39,4 +52,8 @@ struct ucode_cpu_info {
DECLARE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
extern const struct microcode_ops *microcode_ops;
+const struct microcode_patch *microcode_get_cache(void);
+bool microcode_update_cache(struct microcode_patch *patch);
+void microcode_free_patch(struct microcode_patch *patch);
+
#endif /* ASM_X86__MICROCODE_H */
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch Chao Gao
@ 2019-08-02 14:46 ` Jan Beulich
2019-08-05 7:02 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 14:46 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> +bool microcode_update_cache(struct microcode_patch *patch)
> +{
> +
> + ASSERT(spin_is_locked(µcode_mutex));
> +
> + if ( !microcode_cache )
> + microcode_cache = patch;
> + else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
> + NEW_UCODE )
Indentation is wrong here.
> +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;
> +}
Why is it that everything needs to be copied here, rather than
simply shuffling one (or a few) pointer(s)? Can't the caller
simply install the argument it passes here as the new cache blob?
> +static enum microcode_match_result compare_patch(
> + const struct microcode_patch *new, const struct microcode_patch *old)
> +{
> + const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
> +
> + return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
> + old_header->pf, old_header->rev);
So this is exactly what I said on the earlier patch the function
cannot be used for. The way "pf" works precludes this, as said in
reply to an earlier version, and no-one corrected me (i.e. I'm in
no way excluding I'm misunderstanding something here).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch
2019-08-02 14:46 ` Jan Beulich
@ 2019-08-05 7:02 ` Chao Gao
2019-08-05 9:31 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-05 7:02 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On Fri, Aug 02, 2019 at 02:46:58PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> +bool microcode_update_cache(struct microcode_patch *patch)
>> +{
>> +
>> + ASSERT(spin_is_locked(µcode_mutex));
>> +
>> + if ( !microcode_cache )
>> + microcode_cache = patch;
>> + else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
>> + NEW_UCODE )
>
>Indentation is wrong here.
>
>> +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;
>> +}
>
>Why is it that everything needs to be copied here, rather than
>simply shuffling one (or a few) pointer(s)? Can't the caller
>simply install the argument it passes here as the new cache blob?
The old per-cpu cache would use the pointers. And the old cache struct
is removed in "microcode: remove struct ucode_cpu_info". I can add a
patch after that one to reuse the pointers. Otherwise, I may have to
merge following two patches into this one.
>
>> +static enum microcode_match_result compare_patch(
>> + const struct microcode_patch *new, const struct microcode_patch *old)
>> +{
>> + const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
>> +
>> + return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
>> + old_header->pf, old_header->rev);
>
>So this is exactly what I said on the earlier patch the function
>cannot be used for. The way "pf" works precludes this, as said in
>reply to an earlier version, and no-one corrected me (i.e. I'm in
>no way excluding I'm misunderstanding something here).
How about just check 'rev' here and leave a comment here to explain
why?
We drops all mismatched patches (including that only has 'pf'
mismatched) compared with current CPU signature. Given that, it is
fine to only check the revision number.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch
2019-08-05 7:02 ` Chao Gao
@ 2019-08-05 9:31 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 9:31 UTC (permalink / raw)
To: Chao Gao
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 05.08.2019 09:02, Chao Gao wrote:
> On Fri, Aug 02, 2019 at 02:46:58PM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> +bool microcode_update_cache(struct microcode_patch *patch)
>>> +{
>>> +
>>> + ASSERT(spin_is_locked(µcode_mutex));
>>> +
>>> + if ( !microcode_cache )
>>> + microcode_cache = patch;
>>> + else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
>>> + NEW_UCODE )
>>
>> Indentation is wrong here.
>>
>>> +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;
>>> +}
>>
>> Why is it that everything needs to be copied here, rather than
>> simply shuffling one (or a few) pointer(s)? Can't the caller
>> simply install the argument it passes here as the new cache blob?
>
> The old per-cpu cache would use the pointers. And the old cache struct
> is removed in "microcode: remove struct ucode_cpu_info". I can add a
> patch after that one to reuse the pointers. Otherwise, I may have to
> merge following two patches into this one.
If this is just a transitory step, then it's fine, but you should
say so in the description (and the patch to re-use the already
allocated space would then be nice to be part of this series).
>>> +static enum microcode_match_result compare_patch(
>>> + const struct microcode_patch *new, const struct microcode_patch *old)
>>> +{
>>> + const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
>>> +
>>> + return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
>>> + old_header->pf, old_header->rev);
>>
>> So this is exactly what I said on the earlier patch the function
>> cannot be used for. The way "pf" works precludes this, as said in
>> reply to an earlier version, and no-one corrected me (i.e. I'm in
>> no way excluding I'm misunderstanding something here).
>
> How about just check 'rev' here and leave a comment here to explain
> why?
>
> We drops all mismatched patches (including that only has 'pf'
> mismatched) compared with current CPU signature. Given that, it is
> fine to only check the revision number.
Ah, yes, that's apparently another option. But it'll require re-working
microcode_update_match(), or not using it here, afaics.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 07/16] microcode: clean up microcode_resume_cpu
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (5 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 14:57 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 08/16] microcode: remove struct ucode_cpu_info Chao Gao
` (10 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Previously, a per-cpu ucode cache is maintained. Then each CPU had one
per-cpu update cache and there might be multiple versions of microcode.
Thus microcode_resume_cpu tried best to update microcode by loading
every update cache until a successful load.
But now the cache struct is simplified a lot and only a single ucode is
cached. a single invocation of ->apply_microcode() would load the cache
and make microcode updated.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
changes in v8:
- new
- separated from the following patch
---
xen/arch/x86/microcode.c | 40 ++---------------------------------
xen/arch/x86/microcode_amd.c | 47 -----------------------------------------
xen/arch/x86/microcode_intel.c | 6 ------
xen/include/asm-x86/microcode.h | 1 -
4 files changed, 2 insertions(+), 92 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index a8425b8..3a5ddb7 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -215,8 +215,6 @@ int microcode_resume_cpu(unsigned int cpu)
{
int err;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
- struct cpu_signature nsig;
- unsigned int cpu2;
if ( !microcode_ops )
return 0;
@@ -224,42 +222,8 @@ int microcode_resume_cpu(unsigned int cpu)
spin_lock(µcode_mutex);
err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
- if ( err )
- {
- __microcode_fini_cpu(cpu);
- spin_unlock(µcode_mutex);
- return err;
- }
-
- if ( uci->mc.mc_valid )
- {
- err = microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid);
- if ( err >= 0 )
- {
- if ( err )
- err = microcode_ops->apply_microcode(cpu);
- spin_unlock(µcode_mutex);
- return err;
- }
- }
-
- nsig = uci->cpu_sig;
- __microcode_fini_cpu(cpu);
- uci->cpu_sig = nsig;
-
- err = -EIO;
- for_each_online_cpu ( cpu2 )
- {
- uci = &per_cpu(ucode_cpu_info, cpu2);
- if ( uci->mc.mc_valid &&
- microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid) > 0 )
- {
- err = microcode_ops->apply_microcode(cpu);
- break;
- }
- }
-
- __microcode_fini_cpu(cpu);
+ if ( likely(!err) )
+ err = microcode_ops->apply_microcode(cpu);
spin_unlock(µcode_mutex);
return err;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index bb07e1e..796988b 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -650,52 +650,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
return error;
}
-static int microcode_resume_match(unsigned int cpu, const void *mc)
-{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
- struct microcode_amd *mc_amd = uci->mc.mc_amd;
- const struct microcode_amd *src = mc;
-
- if ( microcode_fits(src, cpu) != NEW_UCODE )
- return 0;
-
- if ( src != mc_amd )
- {
- if ( mc_amd )
- {
- xfree(mc_amd->equiv_cpu_table);
- xfree(mc_amd->mpb);
- xfree(mc_amd);
- }
-
- mc_amd = xmalloc(struct microcode_amd);
- uci->mc.mc_amd = mc_amd;
- if ( !mc_amd )
- return -ENOMEM;
- mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
- if ( !mc_amd->equiv_cpu_table )
- goto err1;
- mc_amd->mpb = xmalloc_bytes(src->mpb_size);
- if ( !mc_amd->mpb )
- goto err2;
-
- mc_amd->equiv_cpu_table_size = src->equiv_cpu_table_size;
- mc_amd->mpb_size = src->mpb_size;
- memcpy(mc_amd->mpb, src->mpb, src->mpb_size);
- memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
- src->equiv_cpu_table_size);
- }
-
- return 1;
-
-err2:
- xfree(mc_amd->equiv_cpu_table);
-err1:
- xfree(mc_amd);
- uci->mc.mc_amd = NULL;
- return -ENOMEM;
-}
-
static int start_update(void)
{
#if CONFIG_HVM
@@ -715,7 +669,6 @@ static int start_update(void)
}
static const struct microcode_ops microcode_amd_ops = {
- .microcode_resume_match = microcode_resume_match,
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 811421e..55bfdac 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -443,13 +443,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
return error;
}
-static int microcode_resume_match(unsigned int cpu, const void *mc)
-{
- return get_matching_microcode(mc, cpu);
-}
-
static const struct microcode_ops microcode_intel_ops = {
- .microcode_resume_match = microcode_resume_match,
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 42949b1..3238743 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -21,7 +21,6 @@ struct microcode_patch {
};
struct microcode_ops {
- int (*microcode_resume_match)(unsigned int cpu, const void *mc);
int (*cpu_request_microcode)(unsigned int cpu, const void *buf,
size_t size);
int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 07/16] microcode: clean up microcode_resume_cpu
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 07/16] microcode: clean up microcode_resume_cpu Chao Gao
@ 2019-08-02 14:57 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 14:57 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> Previously, a per-cpu ucode cache is maintained. Then each CPU had one
> per-cpu update cache and there might be multiple versions of microcode.
> Thus microcode_resume_cpu tried best to update microcode by loading
> every update cache until a successful load.
>
> But now the cache struct is simplified a lot and only a single ucode is
> cached. a single invocation of ->apply_microcode() would load the cache
> and make microcode updated.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 08/16] microcode: remove struct ucode_cpu_info
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (6 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 07/16] microcode: clean up microcode_resume_cpu Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 15:03 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 09/16] microcode: remove pointless 'cpu' parameter Chao Gao
` (9 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Remove the per-cpu cache field in struct ucode_cpu_info since it has
been replaced by a global cache. It would leads to only one field
remaining in ucode_cpu_info. Then, this struct is removed and the
remaining field (cpu signature) is stored in per-cpu area.
The cpu status notifier is also removed. It was used to free the "mc"
field to avoid memory leak.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- split microcode_resume_cpu() cleanup to a separate patch.
Changes in v6:
- remove the whole struct ucode_cpu_info instead of the per-cpu cache
in it.
---
xen/arch/x86/apic.c | 2 +-
xen/arch/x86/microcode.c | 57 +++++++--------------------------------
xen/arch/x86/microcode_amd.c | 60 +++++++++++------------------------------
xen/arch/x86/microcode_intel.c | 31 ++++++++-------------
xen/arch/x86/spec_ctrl.c | 2 +-
xen/include/asm-x86/microcode.h | 12 +--------
6 files changed, 39 insertions(+), 125 deletions(-)
diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 9c3c998..ae1f1e9 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1193,7 +1193,7 @@ static void __init check_deadline_errata(void)
else
rev = (unsigned long)m->driver_data;
- if ( this_cpu(ucode_cpu_info).cpu_sig.rev >= rev )
+ if ( this_cpu(cpu_sig).rev >= rev )
return;
setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE);
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 3a5ddb7..9231dfb 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -187,7 +187,7 @@ const struct microcode_ops *microcode_ops;
static DEFINE_SPINLOCK(microcode_mutex);
-DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
+DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
struct microcode_info {
unsigned int cpu;
@@ -196,32 +196,17 @@ struct microcode_info {
char buffer[1];
};
-static void __microcode_fini_cpu(unsigned int cpu)
-{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-
- xfree(uci->mc.mc_valid);
- memset(uci, 0, sizeof(*uci));
-}
-
-static void microcode_fini_cpu(unsigned int cpu)
-{
- spin_lock(µcode_mutex);
- __microcode_fini_cpu(cpu);
- spin_unlock(µcode_mutex);
-}
-
int microcode_resume_cpu(unsigned int cpu)
{
int err;
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
if ( !microcode_ops )
return 0;
spin_lock(µcode_mutex);
- err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ err = microcode_ops->collect_cpu_info(cpu, sig);
if ( likely(!err) )
err = microcode_ops->apply_microcode(cpu);
spin_unlock(µcode_mutex);
@@ -269,16 +254,13 @@ static int microcode_update_cpu(const void *buf, size_t size)
{
int err;
unsigned int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
spin_lock(µcode_mutex);
- err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ err = microcode_ops->collect_cpu_info(cpu, sig);
if ( likely(!err) )
err = microcode_ops->cpu_request_microcode(cpu, buf, size);
- else
- __microcode_fini_cpu(cpu);
-
spin_unlock(µcode_mutex);
return err;
@@ -365,29 +347,10 @@ static int __init microcode_init(void)
}
__initcall(microcode_init);
-static int microcode_percpu_callback(
- struct notifier_block *nfb, unsigned long action, void *hcpu)
-{
- unsigned int cpu = (unsigned long)hcpu;
-
- switch ( action )
- {
- case CPU_DEAD:
- microcode_fini_cpu(cpu);
- break;
- }
-
- return NOTIFY_DONE;
-}
-
-static struct notifier_block microcode_percpu_nfb = {
- .notifier_call = microcode_percpu_callback,
-};
-
int __init early_microcode_update_cpu(bool start_update)
{
unsigned int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
int rc = 0;
void *data = NULL;
size_t len;
@@ -406,7 +369,7 @@ int __init early_microcode_update_cpu(bool start_update)
data = bootstrap_map(&ucode_mod);
}
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ microcode_ops->collect_cpu_info(cpu, sig);
if ( data )
{
@@ -425,7 +388,7 @@ int __init early_microcode_update_cpu(bool start_update)
int __init early_microcode_init(void)
{
unsigned int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
int rc;
rc = microcode_init_intel();
@@ -438,12 +401,10 @@ int __init early_microcode_init(void)
if ( microcode_ops )
{
- microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ microcode_ops->collect_cpu_info(cpu, sig);
if ( ucode_mod.mod_end || ucode_blob.size )
rc = early_microcode_update_cpu(true);
-
- register_cpu_notifier(µcode_percpu_nfb);
}
return rc;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 796988b..3f42781 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -155,7 +155,7 @@ static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
static enum microcode_match_result microcode_fits(
const struct microcode_amd *mc_amd, unsigned int cpu)
{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
unsigned int current_cpu_id;
@@ -178,14 +178,14 @@ static enum microcode_match_result microcode_fits(
return MIS_UCODE;
}
- if ( mc_header->patch_id <= uci->cpu_sig.rev )
+ if ( mc_header->patch_id <= sig->rev )
{
pr_debug("microcode: patch is already at required level or greater.\n");
return OLD_UCODE;
}
pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n",
- cpu, mc_header->patch_id, uci->cpu_sig.rev);
+ cpu, mc_header->patch_id, sig->rev);
return NEW_UCODE;
}
@@ -254,9 +254,9 @@ static enum microcode_match_result compare_patch(
static int apply_microcode(unsigned int cpu)
{
unsigned long flags;
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
uint32_t rev;
int hw_err;
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *hdr;
const struct microcode_patch *patch = microcode_get_cache();
@@ -292,9 +292,9 @@ static int apply_microcode(unsigned int cpu)
}
printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
- cpu, uci->cpu_sig.rev, hdr->patch_id);
+ cpu, sig->rev, hdr->patch_id);
- uci->cpu_sig.rev = rev;
+ sig->rev = rev;
return 0;
}
@@ -440,14 +440,14 @@ static bool_t check_final_patch_levels(unsigned int cpu)
* any of the 'final_levels', then we should not update the microcode
* patch on the cpu as system will hang otherwise.
*/
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
unsigned int i;
if ( boot_cpu_data.x86 != 0x10 )
return 0;
for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
- if ( uci->cpu_sig.rev == final_levels[i] )
+ if ( sig->rev == final_levels[i] )
return 1;
return 0;
@@ -456,13 +456,12 @@ static bool_t check_final_patch_levels(unsigned int cpu)
static int cpu_request_microcode(unsigned int cpu, const void *buf,
size_t bufsize)
{
- struct microcode_amd *mc_amd, *mc_old;
+ struct microcode_amd *mc_amd;
size_t offset = 0;
- size_t last_offset, applied_offset = 0;
- int error = 0, save_error = 1;
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ int error = 0;
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
+ const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
@@ -534,7 +533,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
{
printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
"microcode: Failed to update patch level. "
- "Current lvl:%#x\n", cpu, uci->cpu_sig.rev);
+ "Current lvl:%#x\n", cpu, sig->rev);
break;
}
}
@@ -547,17 +546,12 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
goto out;
}
- mc_old = uci->mc.mc_amd;
- /* implicitely validates uci->mc.mc_valid */
- uci->mc.mc_amd = mc_amd;
-
/*
* It's possible the data file has multiple matching ucode,
* lets keep searching till the latest version
*/
mc_amd->mpb = NULL;
mc_amd->mpb_size = 0;
- last_offset = offset;
while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
&offset)) == 0 )
{
@@ -580,11 +574,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
error = apply_microcode(cpu);
if ( error )
break;
- applied_offset = last_offset;
}
- last_offset = offset;
-
if ( offset >= bufsize )
break;
@@ -612,29 +603,10 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
*(const uint32_t *)(buf + offset) == UCODE_MAGIC )
break;
}
-
- /* On success keep the microcode patch for
- * re-apply on resume.
- */
- if ( applied_offset )
- {
- save_error = get_ucode_from_buffer_amd(
- mc_amd, buf, bufsize, &applied_offset);
-
- if ( save_error )
- error = save_error;
- }
-
- if ( save_error )
- {
- uci->mc.mc_amd = mc_old;
- mc_old = mc_amd;
- }
-
- if ( mc_old->mpb_size )
- xfree(mc_old->mpb);
- xfree(mc_old->equiv_cpu_table);
- xfree(mc_old);
+ if ( mc_amd->mpb_size )
+ xfree(mc_amd->mpb);
+ xfree(mc_amd->equiv_cpu_table);
+ xfree(mc_amd);
out:
#if CONFIG_HVM
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 55bfdac..9140eba 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -257,13 +257,13 @@ static int microcode_sanity_check(void *mc)
static bool match_cpu(const struct microcode_patch *patch)
{
- const struct ucode_cpu_info *uci = &this_cpu(ucode_cpu_info);
+ const struct cpu_signature *sig = &this_cpu(cpu_sig);
if ( !patch )
return false;
- return microcode_update_match(&patch->mc_intel->hdr, uci->cpu_sig.sig,
- uci->cpu_sig.pf, uci->cpu_sig.rev) == NEW_UCODE;
+ return microcode_update_match(&patch->mc_intel->hdr,
+ sig->sig, sig->pf, sig->rev) == NEW_UCODE;
}
static void free_patch(void *mc)
@@ -287,7 +287,7 @@ static enum microcode_match_result compare_patch(
*/
static int get_matching_microcode(const void *mc, unsigned int cpu)
{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_intel *mc_header = mc;
unsigned long total_size = get_totalsize(mc_header);
void *new_mc = xmalloc_bytes(total_size);
@@ -303,8 +303,8 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
new_patch->mc_intel = new_mc;
/* Make sure that this patch covers current CPU */
- if ( microcode_update_match(&new_patch->mc_intel->hdr, uci->cpu_sig.sig,
- uci->cpu_sig.pf, uci->cpu_sig.rev) == MIS_UCODE )
+ if ( microcode_update_match(&new_patch->mc_intel->hdr, sig->sig,
+ sig->pf, sig->rev) == MIS_UCODE )
{
microcode_free_patch(new_patch);
return 0;
@@ -314,17 +314,8 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
pr_debug("microcode: CPU%d found a matching microcode update with"
" version %#x (current=%#x)\n",
- cpu, mc_header->rev, uci->cpu_sig.rev);
- new_mc = xmalloc_bytes(total_size);
- if ( new_mc == NULL )
- {
- printk(KERN_ERR "microcode: error! Can not allocate memory\n");
- return -ENOMEM;
- }
+ cpu, mc_header->rev, sig->rev);
- memcpy(new_mc, mc, total_size);
- xfree(uci->mc.mc_intel);
- uci->mc.mc_intel = new_mc;
return 1;
}
@@ -334,7 +325,7 @@ static int apply_microcode(unsigned int cpu)
uint64_t msr_content;
unsigned int val[2];
unsigned int cpu_num = raw_smp_processor_id();
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
+ struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_intel *mc_intel;
const struct microcode_patch *patch = microcode_get_cache();
@@ -365,14 +356,14 @@ static int apply_microcode(unsigned int cpu)
{
printk(KERN_ERR "microcode: CPU%d update from revision "
"%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
- uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]);
+ sig->rev, mc_intel->hdr.rev, val[1]);
return -EIO;
}
printk(KERN_INFO "microcode: CPU%d updated from revision "
"%#x to %#x, date = %04x-%02x-%02x \n",
- cpu_num, uci->cpu_sig.rev, val[1], mc_intel->hdr.year,
+ cpu_num, sig->rev, val[1], mc_intel->hdr.year,
mc_intel->hdr.month, mc_intel->hdr.day);
- uci->cpu_sig.rev = val[1];
+ sig->rev = val[1];
return 0;
}
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 468a847..4761be8 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -438,7 +438,7 @@ static bool __init check_smt_enabled(void)
/* Calculate whether Retpoline is known-safe on this CPU. */
static bool __init retpoline_safe(uint64_t caps)
{
- unsigned int ucode_rev = this_cpu(ucode_cpu_info).cpu_sig.rev;
+ unsigned int ucode_rev = this_cpu(cpu_sig).rev;
if ( boot_cpu_data.x86_vendor & (X86_VENDOR_AMD | X86_VENDOR_HYGON) )
return true;
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 3238743..5b8289f 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -10,7 +10,6 @@ enum microcode_match_result {
};
struct cpu_signature;
-struct ucode_cpu_info;
struct microcode_patch {
union {
@@ -39,16 +38,7 @@ struct cpu_signature {
unsigned int rev;
};
-struct ucode_cpu_info {
- struct cpu_signature cpu_sig;
- union {
- struct microcode_intel *mc_intel;
- struct microcode_amd *mc_amd;
- void *mc_valid;
- } mc;
-};
-
-DECLARE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
+DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
extern const struct microcode_ops *microcode_ops;
const struct microcode_patch *microcode_get_cache(void);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Xen-devel] [PATCH v8 09/16] microcode: remove pointless 'cpu' parameter
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (7 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 08/16] microcode: remove struct ucode_cpu_info Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 15:15 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
` (8 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Some callbacks in microcode_ops or related functions take a cpu
id parameter. But at current call sites, the cpu id parameter is
always equal to current cpu id. Some of them even use an assertion
to guarantee this. Remove this redundent 'cpu' parameter.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- Use current_cpu_data in collect_cpu_info()
- keep the cpu parameter of check_final_patch_levels()
- use smp_processor_id() in get_matching_microcode() rather than
define a local variable and label it "__maybe_unused"
---
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/microcode.c | 20 ++++++++------------
xen/arch/x86/microcode_amd.c | 35 +++++++++++++----------------------
xen/arch/x86/microcode_intel.c | 28 ++++++++++------------------
xen/arch/x86/smpboot.c | 2 +-
xen/include/asm-x86/microcode.h | 7 +++----
xen/include/asm-x86/processor.h | 2 +-
7 files changed, 37 insertions(+), 59 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index aecc754..4f21903 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -253,7 +253,7 @@ static int enter_state(u32 state)
console_end_sync();
- microcode_resume_cpu(0);
+ microcode_resume_cpu();
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 9231dfb..bfb0afb 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -196,19 +196,19 @@ struct microcode_info {
char buffer[1];
};
-int microcode_resume_cpu(unsigned int cpu)
+int microcode_resume_cpu(void)
{
int err;
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+ struct cpu_signature *sig = &this_cpu(cpu_sig);
if ( !microcode_ops )
return 0;
spin_lock(µcode_mutex);
- err = microcode_ops->collect_cpu_info(cpu, sig);
+ err = microcode_ops->collect_cpu_info(sig);
if ( likely(!err) )
- err = microcode_ops->apply_microcode(cpu);
+ err = microcode_ops->apply_microcode();
spin_unlock(µcode_mutex);
return err;
@@ -258,9 +258,9 @@ static int microcode_update_cpu(const void *buf, size_t size)
spin_lock(µcode_mutex);
- err = microcode_ops->collect_cpu_info(cpu, sig);
+ err = microcode_ops->collect_cpu_info(sig);
if ( likely(!err) )
- err = microcode_ops->cpu_request_microcode(cpu, buf, size);
+ err = microcode_ops->cpu_request_microcode(buf, size);
spin_unlock(µcode_mutex);
return err;
@@ -349,8 +349,6 @@ __initcall(microcode_init);
int __init early_microcode_update_cpu(bool start_update)
{
- unsigned int cpu = smp_processor_id();
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
int rc = 0;
void *data = NULL;
size_t len;
@@ -369,7 +367,7 @@ int __init early_microcode_update_cpu(bool start_update)
data = bootstrap_map(&ucode_mod);
}
- microcode_ops->collect_cpu_info(cpu, sig);
+ microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
if ( data )
{
@@ -387,8 +385,6 @@ int __init early_microcode_update_cpu(bool start_update)
int __init early_microcode_init(void)
{
- unsigned int cpu = smp_processor_id();
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
int rc;
rc = microcode_init_intel();
@@ -401,7 +397,7 @@ int __init early_microcode_init(void)
if ( microcode_ops )
{
- microcode_ops->collect_cpu_info(cpu, sig);
+ microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
if ( ucode_mod.mod_end || ucode_blob.size )
rc = early_microcode_update_cpu(true);
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 3f42781..83ed8f9 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -78,23 +78,23 @@ struct mpbhdr {
static DEFINE_SPINLOCK(microcode_update_lock);
/* See comment in start_update() for cases when this routine fails */
-static int collect_cpu_info(unsigned int cpu, struct cpu_signature *csig)
+static int collect_cpu_info(struct cpu_signature *csig)
{
- struct cpuinfo_x86 *c = &cpu_data[cpu];
+ struct cpuinfo_x86 *c = ¤t_cpu_data;
memset(csig, 0, sizeof(*csig));
if ( (c->x86_vendor != X86_VENDOR_AMD) || (c->x86 < 0x10) )
{
printk(KERN_ERR "microcode: CPU%d not a capable AMD processor\n",
- cpu);
+ smp_processor_id());
return -EINVAL;
}
rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n",
- cpu, csig->rev);
+ smp_processor_id(), csig->rev);
return 0;
}
@@ -153,17 +153,15 @@ static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
}
static enum microcode_match_result microcode_fits(
- const struct microcode_amd *mc_amd, unsigned int cpu)
+ const struct microcode_amd *mc_amd)
{
+ unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *mc_header = mc_amd->mpb;
const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
current_cpu_id = cpuid_eax(0x00000001);
if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) )
@@ -192,9 +190,7 @@ static enum microcode_match_result microcode_fits(
static bool match_cpu(const struct microcode_patch *patch)
{
- if ( !patch )
- return false;
- return microcode_fits(patch->mc_amd, smp_processor_id()) == NEW_UCODE;
+ return patch && (microcode_fits(patch->mc_amd) == NEW_UCODE);
}
static struct microcode_patch *alloc_microcode_patch(
@@ -251,18 +247,16 @@ static enum microcode_match_result compare_patch(
return MIS_UCODE;
}
-static int apply_microcode(unsigned int cpu)
+static int apply_microcode(void)
{
unsigned long flags;
uint32_t rev;
int hw_err;
+ unsigned int cpu = smp_processor_id();
struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *hdr;
const struct microcode_patch *patch = microcode_get_cache();
- /* We should bind the task to the CPU */
- BUG_ON(raw_smp_processor_id() != cpu);
-
if ( !match_cpu(patch) )
return -EINVAL;
@@ -453,19 +447,16 @@ static bool_t check_final_patch_levels(unsigned int cpu)
return 0;
}
-static int cpu_request_microcode(unsigned int cpu, const void *buf,
- size_t bufsize)
+static int cpu_request_microcode(const void *buf, size_t bufsize)
{
struct microcode_amd *mc_amd;
size_t offset = 0;
int error = 0;
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
+ unsigned int cpu = smp_processor_id();
const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
current_cpu_id = cpuid_eax(0x00000001);
if ( *(const uint32_t *)buf != UCODE_MAGIC )
@@ -564,14 +555,14 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
}
/* Update cache if this patch covers current CPU */
- if ( microcode_fits(new_patch->mc_amd, cpu) != MIS_UCODE )
+ 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()) )
{
- error = apply_microcode(cpu);
+ error = apply_microcode();
if ( error )
break;
}
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 9140eba..867e4f2 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -96,13 +96,12 @@ struct extended_sigtable {
/* serialize access to the physical write to MSR 0x79 */
static DEFINE_SPINLOCK(microcode_update_lock);
-static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
+static int collect_cpu_info(struct cpu_signature *csig)
{
+ unsigned int cpu_num = smp_processor_id();
struct cpuinfo_x86 *c = &cpu_data[cpu_num];
uint64_t msr_content;
- BUG_ON(cpu_num != smp_processor_id());
-
memset(csig, 0, sizeof(*csig));
if ( (c->x86_vendor != X86_VENDOR_INTEL) || (c->x86 < 6) )
@@ -285,9 +284,9 @@ static enum microcode_match_result compare_patch(
* return 1 - found update
* return < 0 - error
*/
-static int get_matching_microcode(const void *mc, unsigned int cpu)
+static int get_matching_microcode(const void *mc)
{
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+ struct cpu_signature *sig = &this_cpu(cpu_sig);
const struct microcode_header_intel *mc_header = mc;
unsigned long total_size = get_totalsize(mc_header);
void *new_mc = xmalloc_bytes(total_size);
@@ -314,24 +313,21 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
pr_debug("microcode: CPU%d found a matching microcode update with"
" version %#x (current=%#x)\n",
- cpu, mc_header->rev, sig->rev);
+ smp_processord_id(), mc_header->rev, sig->rev);
return 1;
}
-static int apply_microcode(unsigned int cpu)
+static int apply_microcode(void)
{
unsigned long flags;
uint64_t msr_content;
unsigned int val[2];
unsigned int cpu_num = raw_smp_processor_id();
- struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+ struct cpu_signature *sig = &this_cpu(cpu_sig);
const struct microcode_intel *mc_intel;
const struct microcode_patch *patch = microcode_get_cache();
- /* We should bind the task to the CPU */
- BUG_ON(cpu_num != cpu);
-
if ( !match_cpu(patch) )
return -EINVAL;
@@ -396,22 +392,18 @@ static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
return offset + total_size;
}
-static int cpu_request_microcode(unsigned int cpu, const void *buf,
- size_t size)
+static int cpu_request_microcode(const void *buf, size_t size)
{
long offset = 0;
int error = 0;
void *mc;
- /* We should bind the task to the CPU */
- BUG_ON(cpu != raw_smp_processor_id());
-
while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
{
error = microcode_sanity_check(mc);
if ( error )
break;
- error = get_matching_microcode(mc, cpu);
+ error = get_matching_microcode(mc);
if ( error < 0 )
break;
/*
@@ -429,7 +421,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
error = offset;
if ( !error && match_cpu(microcode_get_cache()) )
- error = apply_microcode(cpu);
+ error = apply_microcode();
return error;
}
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 65e9cee..c818cfc 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -364,7 +364,7 @@ void start_secondary(void *unused)
if ( system_state <= SYS_STATE_smp_boot )
early_microcode_update_cpu(false);
else
- microcode_resume_cpu(cpu);
+ microcode_resume_cpu();
/*
* If MSR_SPEC_CTRL is available, apply Xen's default setting and discard
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 5b8289f..35223eb 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -20,10 +20,9 @@ struct microcode_patch {
};
struct microcode_ops {
- int (*cpu_request_microcode)(unsigned int cpu, const void *buf,
- size_t size);
- int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig);
- int (*apply_microcode)(unsigned int cpu);
+ int (*cpu_request_microcode)(const void *buf, size_t size);
+ int (*collect_cpu_info)(struct cpu_signature *csig);
+ int (*apply_microcode)(void);
int (*start_update)(void);
void (*free_patch)(void *mc);
bool (*match_cpu)(const struct microcode_patch *patch);
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 2862321..104faa9 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -568,7 +568,7 @@ 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(unsigned int cpu);
+int microcode_resume_cpu(void);
int early_microcode_update_cpu(bool start_update);
int early_microcode_init(void);
int microcode_init_intel(void);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 09/16] microcode: remove pointless 'cpu' parameter
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 09/16] microcode: remove pointless 'cpu' parameter Chao Gao
@ 2019-08-02 15:15 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 15:15 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -78,23 +78,23 @@ struct mpbhdr {
> static DEFINE_SPINLOCK(microcode_update_lock);
>
> /* See comment in start_update() for cases when this routine fails */
> -static int collect_cpu_info(unsigned int cpu, struct cpu_signature *csig)
> +static int collect_cpu_info(struct cpu_signature *csig)
> {
> - struct cpuinfo_x86 *c = &cpu_data[cpu];
> + struct cpuinfo_x86 *c = ¤t_cpu_data;
>
> memset(csig, 0, sizeof(*csig));
>
> if ( (c->x86_vendor != X86_VENDOR_AMD) || (c->x86 < 0x10) )
> {
> printk(KERN_ERR "microcode: CPU%d not a capable AMD processor\n",
> - cpu);
> + smp_processor_id());
> return -EINVAL;
> }
>
> rdmsrl(MSR_AMD_PATCHLEVEL, csig->rev);
>
> pr_debug("microcode: CPU%d collect_cpu_info: patch_id=%#x\n",
> - cpu, csig->rev);
> + smp_processor_id(), csig->rev);
>
> return 0;
> }
Argh - I'd been wrong saying "The only other use of "cpu" is in a
pr_debug()" in a reply to v7. I had managed to overlook the use in
the printk(). This suggests that the earlier solution was better,
as now we have at least two smp_processor_id() in the function, in
a debug build three of them. I'm sorry.
Reviewed-by: Jan Beulich <jbeulich@suse.com>
preferably with the change above moved back to its v7 shape, but
given this was my mistake I won't insist. If there was no need for
v9, then this could also be done while committing.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (8 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 09/16] microcode: remove pointless 'cpu' parameter Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 15:21 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 11/16] microcode: pass a patch pointer to apply_microcode() Chao Gao
` (7 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Introduce a vendor hook, .end_update, for svm_host_osvw_init().
The hook function is called on each cpu after loading an update.
It is a preparation for spliting out apply_microcode() from
cpu_request_microcode().
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- new
---
xen/arch/x86/microcode.c | 3 +++
xen/arch/x86/microcode_amd.c | 21 ++++++++++-----------
xen/include/asm-x86/microcode.h | 1 +
3 files changed, 14 insertions(+), 11 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index bfb0afb..082b29c 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -277,6 +277,9 @@ static long do_microcode_update(void *_info)
if ( error )
info->error = error;
+ if ( microcode_ops->end_update )
+ microcode_ops->end_update();
+
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);
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 83ed8f9..3d1505d 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -600,10 +600,6 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
xfree(mc_amd);
out:
-#if CONFIG_HVM
- svm_host_osvw_init();
-#endif
-
/*
* 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
@@ -617,13 +613,8 @@ static int start_update(void)
{
#if CONFIG_HVM
/*
- * We assume here that svm_host_osvw_init() will be called on each cpu (from
- * cpu_request_microcode()).
- *
- * Note that if collect_cpu_info() returns an error then
- * cpu_request_microcode() will not invoked thus leaving OSVW bits not
- * updated. Currently though collect_cpu_info() will not fail on processors
- * supporting OSVW so we will not deal with this possibility.
+ * svm_host_osvw_init() will be called on each cpu by calling '.end_update'
+ * in common code.
*/
svm_host_osvw_reset();
#endif
@@ -631,11 +622,19 @@ static int start_update(void)
return 0;
}
+static void end_update(void)
+{
+#if CONFIG_HVM
+ svm_host_osvw_init();
+#endif
+}
+
static const struct microcode_ops microcode_amd_ops = {
.cpu_request_microcode = cpu_request_microcode,
.collect_cpu_info = collect_cpu_info,
.apply_microcode = apply_microcode,
.start_update = start_update,
+ .end_update = end_update,
.free_patch = free_patch,
.compare_patch = compare_patch,
.match_cpu = match_cpu,
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 35223eb..c8d2c4f 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -24,6 +24,7 @@ struct microcode_ops {
int (*collect_cpu_info)(struct cpu_signature *csig);
int (*apply_microcode)(void);
int (*start_update)(void);
+ void (*end_update)(void);
void (*free_patch)(void *mc);
bool (*match_cpu)(const struct microcode_patch *patch);
enum microcode_match_result (*compare_patch)(
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
@ 2019-08-02 15:21 ` Jan Beulich
2019-08-05 7:05 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 15:21 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -277,6 +277,9 @@ static long do_microcode_update(void *_info)
> if ( error )
> info->error = error;
>
> + if ( microcode_ops->end_update )
> + microcode_ops->end_update();
> +
> 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);
This being the only change in this file - don't you also need to
alter the early ucode loading path?
> @@ -631,11 +622,19 @@ static int start_update(void)
> return 0;
> }
>
> +static void end_update(void)
> +{
> +#if CONFIG_HVM
> + svm_host_osvw_init();
> +#endif
> +}
Instead of leaving an empty function in the !HVM case, ...
> static const struct microcode_ops microcode_amd_ops = {
> .cpu_request_microcode = cpu_request_microcode,
> .collect_cpu_info = collect_cpu_info,
> .apply_microcode = apply_microcode,
> .start_update = start_update,
> + .end_update = end_update,
... could you please leave this pointer uninitialized (i.e.
NULL) in that case?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code
2019-08-02 15:21 ` Jan Beulich
@ 2019-08-05 7:05 ` Chao Gao
0 siblings, 0 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-05 7:05 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On Fri, Aug 02, 2019 at 03:21:55PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -277,6 +277,9 @@ static long do_microcode_update(void *_info)
>> if ( error )
>> info->error = error;
>>
>> + if ( microcode_ops->end_update )
>> + microcode_ops->end_update();
>> +
>> 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);
>
>This being the only change in this file - don't you also need to
>alter the early ucode loading path?
Yes. I should have.
>
>> @@ -631,11 +622,19 @@ static int start_update(void)
>> return 0;
>> }
>>
>> +static void end_update(void)
>> +{
>> +#if CONFIG_HVM
>> + svm_host_osvw_init();
>> +#endif
>> +}
>
>Instead of leaving an empty function in the !HVM case, ...
>
>> static const struct microcode_ops microcode_amd_ops = {
>> .cpu_request_microcode = cpu_request_microcode,
>> .collect_cpu_info = collect_cpu_info,
>> .apply_microcode = apply_microcode,
>> .start_update = start_update,
>> + .end_update = end_update,
>
>... could you please leave this pointer uninitialized (i.e.
>NULL) in that case?
Will do.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 11/16] microcode: pass a patch pointer to apply_microcode()
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (9 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 10/16] microcode/amd: call svm_host_osvw_init() in common code Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 15:25 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
` (6 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
apply_microcode()'s always loading the cached ucode patch forces
a patch to be stored before being loading. Make apply_microcode()
accept a patch pointer to remove the limitation so that a patch
can be stored after a successful loading.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- new
---
xen/arch/x86/microcode.c | 2 +-
xen/arch/x86/microcode_amd.c | 5 ++---
xen/arch/x86/microcode_intel.c | 5 ++---
xen/include/asm-x86/microcode.h | 2 +-
4 files changed, 6 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 082b29c..03bc0aa 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -208,7 +208,7 @@ int microcode_resume_cpu(void)
err = microcode_ops->collect_cpu_info(sig);
if ( likely(!err) )
- err = microcode_ops->apply_microcode();
+ err = microcode_ops->apply_microcode(microcode_cache);
spin_unlock(µcode_mutex);
return err;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 3d1505d..fed044a 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -247,7 +247,7 @@ static enum microcode_match_result compare_patch(
return MIS_UCODE;
}
-static int apply_microcode(void)
+static int apply_microcode(const struct microcode_patch *patch)
{
unsigned long flags;
uint32_t rev;
@@ -255,7 +255,6 @@ static int apply_microcode(void)
unsigned int cpu = smp_processor_id();
struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
const struct microcode_header_amd *hdr;
- const struct microcode_patch *patch = microcode_get_cache();
if ( !match_cpu(patch) )
return -EINVAL;
@@ -562,7 +561,7 @@ static int cpu_request_microcode(const void *buf, size_t bufsize)
if ( match_cpu(microcode_get_cache()) )
{
- error = apply_microcode();
+ error = apply_microcode(microcode_get_cache());
if ( error )
break;
}
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 867e4f2..bcb48bc 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -318,7 +318,7 @@ static int get_matching_microcode(const void *mc)
return 1;
}
-static int apply_microcode(void)
+static int apply_microcode(const struct microcode_patch *patch)
{
unsigned long flags;
uint64_t msr_content;
@@ -326,7 +326,6 @@ static int apply_microcode(void)
unsigned int cpu_num = raw_smp_processor_id();
struct cpu_signature *sig = &this_cpu(cpu_sig);
const struct microcode_intel *mc_intel;
- const struct microcode_patch *patch = microcode_get_cache();
if ( !match_cpu(patch) )
return -EINVAL;
@@ -421,7 +420,7 @@ static int cpu_request_microcode(const void *buf, size_t size)
error = offset;
if ( !error && match_cpu(microcode_get_cache()) )
- error = apply_microcode();
+ error = apply_microcode(microcode_get_cache());
return error;
}
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index c8d2c4f..8c7de9d 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -22,7 +22,7 @@ struct microcode_patch {
struct microcode_ops {
int (*cpu_request_microcode)(const void *buf, size_t size);
int (*collect_cpu_info)(struct cpu_signature *csig);
- int (*apply_microcode)(void);
+ int (*apply_microcode)(const struct microcode_patch *patch);
int (*start_update)(void);
void (*end_update)(void);
void (*free_patch)(void *mc);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (10 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 11/16] microcode: pass a patch pointer to apply_microcode() Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-02 15:40 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup Chao Gao
` (5 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
During late microcode loading, apply_microcode() is invoked in
cpu_request_microcode(). To make late microcode update more reliable,
we want to put the apply_microcode() into stop_machine context. So
we split out it from cpu_request_microcode(). In general, for both
early loading on BSP and late loading, cpu_request_microcode() is
called first to get the matching microcode update contained by
the blob and then apply_microcode() is invoked explicitly on each
cpu in common code.
Given that all CPUs are supposed to have the same signature, parsing
microcode only needs to be done once. So cpu_request_microcode() is
also moved out of microcode_update_cpu().
In some cases (e.g. a broken bios), the system may have multiple
revisions of microcode update. So we would try to load a microcode
update as long as it covers current cpu. And if a cpu loads this patch
successfully, the patch would be stored into the patch cache.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
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 | 181 ++++++++++++++++++++++++++++------------
xen/arch/x86/microcode_amd.c | 38 +++++----
xen/arch/x86/microcode_intel.c | 70 ++++++++--------
xen/include/asm-x86/microcode.h | 5 +-
4 files changed, 186 insertions(+), 108 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 03bc0aa..f0b1e39 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -189,12 +189,20 @@ 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 *microcode_parse_blob(const char *buf,
+ uint32_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,13 +228,6 @@ 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)
{
@@ -250,49 +251,88 @@ 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));
+
+ if ( unlikely(err) )
+ return err;
spin_lock(µcode_mutex);
- err = microcode_ops->collect_cpu_info(sig);
- if ( likely(!err) )
- err = microcode_ops->cpu_request_microcode(buf, size);
+ if ( patch )
+ {
+ /*
+ * If a patch is specified, it should has newer revision than
+ * that of the patch cached.
+ */
+ if ( microcode_cache &&
+ microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
+ {
+ spin_unlock(µcode_mutex);
+ return -EINVAL;
+ }
+ }
+ else if ( microcode_cache )
+ patch = microcode_cache;
+ else
+ /* No patch to update */
+ err = -ENOENT;
+
+ if ( patch )
+ {
+ err = microcode_ops->apply_microcode(patch);
+ /* clean up patch cache if we failed to load the cached patch */
+ if ( patch == microcode_cache && err == -EIO )
+ {
+ microcode_free_patch(microcode_cache);
+ microcode_cache = NULL;
+ }
+ }
+
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;
- error = microcode_update_cpu(info->buffer, info->buffer_size);
- if ( error )
- info->error = error;
+ /* store the patch after a successful loading */
+ if ( !microcode_update_cpu(patch) && patch )
+ {
+ spin_lock(µcode_mutex);
+ microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+ patch = NULL;
+ }
if ( microcode_ops->end_update )
microcode_ops->end_update();
- 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);
+ cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
+ if ( cpu < nr_cpu_ids )
+ return continue_hypercall_on_cpu(cpu, do_microcode_update, patch);
+
+ /* Free the patch if no CPU has loaded it successfully. */
+ if ( patch )
+ microcode_free_patch(patch);
- error = info->error;
- xfree(info);
- return error;
+ return 0;
}
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;
@@ -300,32 +340,44 @@ 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 )
+ if ( copy_from_guest(buffer, buf, len) )
{
- xfree(info);
- return ret;
+ ret = -EFAULT;
+ goto free;
}
- info->buffer_size = len;
- info->error = 0;
- info->cpu = cpumask_first(&cpu_online_map);
-
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
if ( ret != 0 )
- {
- xfree(info);
- return ret;
- }
+ goto free;
+ }
+
+ patch = microcode_parse_blob(buffer, len);
+ if ( IS_ERR(patch) )
+ {
+ ret = PTR_ERR(patch);
+ printk(XENLOG_INFO "Parsing microcode blob error %d\n", ret);
+ goto free;
}
- return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+ if ( !patch )
+ {
+ printk(XENLOG_INFO "No ucode found. Update aborted!\n");
+ ret = -EINVAL;
+ goto free;
+ }
+
+ ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
+ do_microcode_update, patch);
+
+ free:
+ xfree(buffer);
+ return ret;
}
static int __init microcode_init(void)
@@ -372,15 +424,40 @@ int __init early_microcode_update_cpu(bool start_update)
microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
+ if ( !start_update )
+ return microcode_update_cpu(NULL);
+
if ( data )
{
- if ( start_update && microcode_ops->start_update )
+ struct microcode_patch *patch;
+
+ if ( microcode_ops->start_update )
rc = microcode_ops->start_update();
if ( rc )
return rc;
- return microcode_update_cpu(data, len);
+ patch = microcode_parse_blob(data, len);
+ if ( IS_ERR(patch) )
+ {
+ printk(XENLOG_INFO "Parsing microcode blob error %ld\n",
+ PTR_ERR(patch));
+ return PTR_ERR(patch);
+ }
+
+ if ( !patch )
+ {
+ printk(XENLOG_INFO "No ucode found. Update aborted!\n");
+ return -EINVAL;
+ }
+
+ spin_lock(µcode_mutex);
+ rc = microcode_update_cache(patch);
+ spin_unlock(µcode_mutex);
+
+ ASSERT(rc);
+
+ return microcode_update_cpu(NULL);
}
else
return -ENOMEM;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index fed044a..93d2a98 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -446,9 +446,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;
@@ -553,19 +555,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)
xfree(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;
}
static int start_update(void)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index bcb48bc..8780be0 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -279,15 +279,9 @@ static enum microcode_match_result compare_patch(
old_header->pf, old_header->rev);
}
-/*
- * 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)
{
- struct cpu_signature *sig = &this_cpu(cpu_sig);
- 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);
@@ -296,26 +290,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(&new_patch->mc_intel->hdr, sig->sig,
- sig->pf, sig->rev) == 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_processord_id(), mc_header->rev, sig->rev);
-
- return 1;
+ return new_patch;
}
static int apply_microcode(const struct microcode_patch *patch)
@@ -391,26 +371,46 @@ 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;
+ const struct cpu_signature *sig = &this_cpu(cpu_sig);
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, sig->sig,
+ sig->pf, sig->rev) != 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);
}
@@ -419,10 +419,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 8c7de9d..8e71615 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);
@@ -41,8 +42,6 @@ struct cpu_signature {
DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
extern const struct microcode_ops *microcode_ops;
-const struct microcode_patch *microcode_get_cache(void);
-bool microcode_update_cache(struct microcode_patch *patch);
void microcode_free_patch(struct microcode_patch *patch);
#endif /* ASM_X86__MICROCODE_H */
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
@ 2019-08-02 15:40 ` Jan Beulich
2019-08-05 7:36 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-02 15:40 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -189,12 +189,20 @@ 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 *microcode_parse_blob(const char *buf,
> + uint32_t len)
Btw - you'd have less issues with line length if you omitted the
"microcode_" prefix from static functions.
> @@ -250,49 +251,88 @@ 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));
> +
> + if ( unlikely(err) )
> + return err;
>
> spin_lock(µcode_mutex);
>
> - err = microcode_ops->collect_cpu_info(sig);
> - if ( likely(!err) )
> - err = microcode_ops->cpu_request_microcode(buf, size);
> + if ( patch )
> + {
> + /*
> + * If a patch is specified, it should has newer revision than
> + * that of the patch cached.
> + */
> + if ( microcode_cache &&
> + microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
While I see that you've taken care of the one case in Intel specific
code, this is again a case where I don't think you can validly call
this hook in the Intel case. Albeit maybe it is okay, provided that
the caller has already verified it against the CPU signature. Then
again I wonder why this check gets done here rather than in the
caller (next to that other check) anyway. Afaics this way you'd
also avoid re-checking on every CPU a CPU-independent property.
> -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;
>
> - error = microcode_update_cpu(info->buffer, info->buffer_size);
> - if ( error )
> - info->error = error;
> + /* store the patch after a successful loading */
Nit: Comments should start with an uppercase letter.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()
2019-08-02 15:40 ` Jan Beulich
@ 2019-08-05 7:36 ` Chao Gao
2019-08-05 9:38 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-05 7:36 UTC (permalink / raw)
To: Jan Beulich
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On Fri, Aug 02, 2019 at 03:40:55PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -189,12 +189,20 @@ 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 *microcode_parse_blob(const char *buf,
>> + uint32_t len)
>
>Btw - you'd have less issues with line length if you omitted the
>"microcode_" prefix from static functions.
>
>> @@ -250,49 +251,88 @@ 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));
>> +
>> + if ( unlikely(err) )
>> + return err;
>>
>> spin_lock(µcode_mutex);
>>
>> - err = microcode_ops->collect_cpu_info(sig);
>> - if ( likely(!err) )
>> - err = microcode_ops->cpu_request_microcode(buf, size);
>> + if ( patch )
>> + {
>> + /*
>> + * If a patch is specified, it should has newer revision than
>> + * that of the patch cached.
>> + */
>> + if ( microcode_cache &&
>> + microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
>
>While I see that you've taken care of the one case in Intel specific
>code, this is again a case where I don't think you can validly call
>this hook in the Intel case. Albeit maybe it is okay, provided that
>the caller has already verified it against the CPU signature. Then
>again I wonder why this check gets done here rather than in the
>caller (next to that other check) anyway. Afaics this way you'd
>also avoid re-checking on every CPU a CPU-independent property.
As said in an earlier reply to patch 6, ->compare_patch can
be simplified a lot. Do you think it is fine to call it here
with that change?
About avoiding re-checking, we should check it with "microcode_mutex"
held otherwise we cannot ensure nobody is updating the cache. If we want
to avoid re-checking, then this lock is held for a long time from loading
on the first core to the last core. And also for early loading and late
loading, we pass 'NULL' to this function on following CPUs after the
first successful loading. I am afraid that there is no redundant checking.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()
2019-08-05 7:36 ` Chao Gao
@ 2019-08-05 9:38 ` Jan Beulich
2019-08-05 12:09 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 9:38 UTC (permalink / raw)
To: Chao Gao
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 05.08.2019 09:36, Chao Gao wrote:
> On Fri, Aug 02, 2019 at 03:40:55PM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> --- a/xen/arch/x86/microcode.c
>>> +++ b/xen/arch/x86/microcode.c
>>> @@ -189,12 +189,20 @@ 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 *microcode_parse_blob(const char *buf,
>>> + uint32_t len)
>>
>> Btw - you'd have less issues with line length if you omitted the
>> "microcode_" prefix from static functions.
>>
>>> @@ -250,49 +251,88 @@ 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));
>>> +
>>> + if ( unlikely(err) )
>>> + return err;
>>>
>>> spin_lock(µcode_mutex);
>>>
>>> - err = microcode_ops->collect_cpu_info(sig);
>>> - if ( likely(!err) )
>>> - err = microcode_ops->cpu_request_microcode(buf, size);
>>> + if ( patch )
>>> + {
>>> + /*
>>> + * If a patch is specified, it should has newer revision than
>>> + * that of the patch cached.
>>> + */
>>> + if ( microcode_cache &&
>>> + microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
>>
>> While I see that you've taken care of the one case in Intel specific
>> code, this is again a case where I don't think you can validly call
>> this hook in the Intel case. Albeit maybe it is okay, provided that
>> the caller has already verified it against the CPU signature. Then
>> again I wonder why this check gets done here rather than in the
>> caller (next to that other check) anyway. Afaics this way you'd
>> also avoid re-checking on every CPU a CPU-independent property.
>
> As said in an earlier reply to patch 6, ->compare_patch can
> be simplified a lot. Do you think it is fine to call it here
> with that change?
As said there - yes, this looks to be an option.
> About avoiding re-checking, we should check it with "microcode_mutex"
> held otherwise we cannot ensure nobody is updating the cache. If we want
> to avoid re-checking, then this lock is held for a long time from loading
> on the first core to the last core. And also for early loading and late
> loading, we pass 'NULL' to this function on following CPUs after the
> first successful loading. I am afraid that there is no redundant checking.
There should not be any updating of the cache while one (system-wide)
ucode load operation is in progress, or else you risk leaving the
system in a "mixed ucode versions" state in the end. As said, also
from a logical flow-of-events perspective it seems to me that it should
be the caller to validate applicability of the patch before calling
here. But as also said, I may as well be missing some important aspect
here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()
2019-08-05 9:38 ` Jan Beulich
@ 2019-08-05 12:09 ` Chao Gao
0 siblings, 0 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-05 12:09 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On Mon, Aug 05, 2019 at 09:38:00AM +0000, Jan Beulich wrote:
>On 05.08.2019 09:36, Chao Gao wrote:
>> On Fri, Aug 02, 2019 at 03:40:55PM +0000, Jan Beulich wrote:
>>> On 01.08.2019 12:22, Chao Gao wrote:
>>>> --- a/xen/arch/x86/microcode.c
>>>> +++ b/xen/arch/x86/microcode.c
>>>> @@ -189,12 +189,20 @@ 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 *microcode_parse_blob(const char *buf,
>>>> + uint32_t len)
>>>
>>> Btw - you'd have less issues with line length if you omitted the
>>> "microcode_" prefix from static functions.
>>>
>>>> @@ -250,49 +251,88 @@ 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));
>>>> +
>>>> + if ( unlikely(err) )
>>>> + return err;
>>>>
>>>> spin_lock(µcode_mutex);
>>>>
>>>> - err = microcode_ops->collect_cpu_info(sig);
>>>> - if ( likely(!err) )
>>>> - err = microcode_ops->cpu_request_microcode(buf, size);
>>>> + if ( patch )
>>>> + {
>>>> + /*
>>>> + * If a patch is specified, it should has newer revision than
>>>> + * that of the patch cached.
>>>> + */
>>>> + if ( microcode_cache &&
>>>> + microcode_ops->compare_patch(patch, microcode_cache) != NEW_UCODE )
>>>
>>> While I see that you've taken care of the one case in Intel specific
>>> code, this is again a case where I don't think you can validly call
>>> this hook in the Intel case. Albeit maybe it is okay, provided that
>>> the caller has already verified it against the CPU signature. Then
>>> again I wonder why this check gets done here rather than in the
>>> caller (next to that other check) anyway. Afaics this way you'd
>>> also avoid re-checking on every CPU a CPU-independent property.
>>
>> As said in an earlier reply to patch 6, ->compare_patch can
>> be simplified a lot. Do you think it is fine to call it here
>> with that change?
>
>As said there - yes, this looks to be an option.
>
>> About avoiding re-checking, we should check it with "microcode_mutex"
>> held otherwise we cannot ensure nobody is updating the cache. If we want
>> to avoid re-checking, then this lock is held for a long time from loading
>> on the first core to the last core. And also for early loading and late
>> loading, we pass 'NULL' to this function on following CPUs after the
>> first successful loading. I am afraid that there is no redundant checking.
>
>There should not be any updating of the cache while one (system-wide)
>ucode load operation is in progress, or else you risk leaving the
>system in a "mixed ucode versions" state in the end. As said, also
Yes. I agree.
>from a logical flow-of-events perspective it seems to me that it should
>be the caller to validate applicability of the patch before calling
>here.
Maybe we can just remove this check. The check is to avoid loading an
older update than the cached one (an error is returned if it is the
case). But actually, ->apply_microcode() will ensure the provided
update's revision is newer than current update revision by calling
match_cpu(). So probably this check is redundant.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (11 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode() Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-05 10:19 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading Chao Gao
` (4 subsequent siblings)
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
Both are loading the cached patch. Since APs call the unified function,
microcode_update_one(), during wakeup, the 'start_update' parameter
which originally used to distinguish BSP and APs is redundant. So remove
this parameter.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- split out from the previous patch
---
xen/arch/x86/acpi/power.c | 2 +-
xen/arch/x86/microcode.c | 36 +++++++++++-------------------------
xen/arch/x86/smpboot.c | 5 +----
xen/include/asm-x86/processor.h | 4 ++--
4 files changed, 15 insertions(+), 32 deletions(-)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 4f21903..24798d5 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -253,7 +253,7 @@ static int enter_state(u32 state)
console_end_sync();
- microcode_resume_cpu();
+ microcode_update_one();
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 f0b1e39..cbaf13d 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -204,24 +204,6 @@ static struct microcode_patch *microcode_parse_blob(const char *buf,
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);
@@ -402,7 +384,16 @@ 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(void)
+{
+ return microcode_ops ? microcode_update_cpu(NULL) : 0;
+}
+
+/*
+ * 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;
@@ -422,11 +413,6 @@ int __init early_microcode_update_cpu(bool start_update)
data = bootstrap_map(&ucode_mod);
}
- microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
-
- if ( !start_update )
- return microcode_update_cpu(NULL);
-
if ( data )
{
struct microcode_patch *patch;
@@ -480,7 +466,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 c818cfc..e62a1ca 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -361,10 +361,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();
/*
* 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 104faa9..2a76d90 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -568,9 +568,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(void);
int microcode_init_intel(void);
int microcode_init_amd(void);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup Chao Gao
@ 2019-08-05 10:19 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 10:19 UTC (permalink / raw)
To: Chao Gao
Cc: xen-devel, Roger Pau Monné, Ashok Raj, Wei Liu, Andrew Cooper
On 01.08.2019 12:22, Chao Gao wrote:
> Both are loading the cached patch. Since APs call the unified function,
> microcode_update_one(), during wakeup, the 'start_update' parameter
> which originally used to distinguish BSP and APs is redundant. So remove
> this parameter.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> Changes in v8:
> - split out from the previous patch
> ---
> xen/arch/x86/acpi/power.c | 2 +-
> xen/arch/x86/microcode.c | 36 +++++++++++-------------------------
> xen/arch/x86/smpboot.c | 5 +----
> xen/include/asm-x86/processor.h | 4 ++--
> 4 files changed, 15 insertions(+), 32 deletions(-)
>
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 4f21903..24798d5 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -253,7 +253,7 @@ static int enter_state(u32 state)
>
> console_end_sync();
>
> - microcode_resume_cpu();
> + microcode_update_one();
>
> 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 f0b1e39..cbaf13d 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -204,24 +204,6 @@ static struct microcode_patch *microcode_parse_blob(const char *buf,
> 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);
> @@ -402,7 +384,16 @@ 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(void)
> +{
> + return microcode_ops ? microcode_update_cpu(NULL) : 0;
> +}
With both callers ignoring the return value, I wonder if the
function should return void. Else it might be better (but I'm
not entirely certain) for it to return -EOPNOTSUPP rather
than 0.
> +/*
> + * BSP calls this function to parse ucode blob and then apply an update.
> + */
This is a single line comment, and hence wants its style
changed accordingly.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (12 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-05 10:43 ` Jan Beulich
2019-08-05 12:07 ` Jan Beulich
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 15/16] microcode: remove microcode_update_lock Chao Gao
` (3 subsequent siblings)
17 siblings, 2 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Borislav Petkov, Ashok Raj, Wei Liu, Jun Nakajima,
Andrew Cooper, Jan Beulich, Thomas Gleixner, Chao Gao,
Roger Pau Monné
This patch ports microcode improvement patches from linux kernel.
Before you read any further: the early loading method is still the
preferred one and you should always do that. The following patch is
improving the late loading mechanism for long running jobs and cloud use
cases.
Gather all cores and serialize the microcode update on them by doing it
one-by-one to make the late update process as reliable as possible and
avoid potential issues caused by the microcode update.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Tested-by: Chao Gao <chao.gao@intel.com>
[linux commit: a5321aec6412b20b5ad15db2d6b916c05349dbff]
[linux commit: bb8c13d61a629276a162c1d2b1a20a815cbcfbb7]
Cc: Kevin Tian <kevin.tian@intel.com>
Cc: Jun Nakajima <jun.nakajima@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>
Cc: Borislav Petkov <bp@suse.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
---
Changes in 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 | 229 ++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 207 insertions(+), 22 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index cbaf13d..67549be 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -30,18 +30,41 @@
#include <xen/smp.h>
#include <xen/softirq.h>
#include <xen/spinlock.h>
+#include <xen/stop_machine.h>
#include <xen/tasklet.h>
#include <xen/guest_access.h>
#include <xen/earlycpio.h>
+#include <xen/watchdog.h>
+#include <asm/delay.h>
#include <asm/msr.h>
#include <asm/processor.h>
#include <asm/setup.h>
#include <asm/microcode.h>
+/*
+ * Before performing a late microcode update on any thread, we
+ * rendezvous all cpus in stop_machine context. The timeout for
+ * waiting for cpu rendezvous is 30ms. It is the timeout used by
+ * live patching
+ */
+#define MICROCODE_CALLIN_TIMEOUT_US 30000
+
+/*
+ * Timeout for each thread to complete update is set to 1s. It is a
+ * conservative choice considering all possible interference.
+ */
+#define MICROCODE_UPDATE_TIMEOUT_US 1000000
+
static module_t __initdata ucode_mod;
static signed int __initdata ucode_mod_idx;
static bool_t __initdata ucode_mod_forced;
+static unsigned int nr_cores;
+static enum {
+ LOADING_EXITED,
+ LOADING_ENTERED,
+ LOADING_ABORTED,
+} loading_state;
/*
* If we scan the initramfs.cpio for the early microcode code
@@ -190,6 +213,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
@@ -234,6 +267,35 @@ bool microcode_update_cache(struct microcode_patch *patch)
}
/*
+ * Wait for a condition to be met with a timeout (us).
+ */
+static int wait_for_condition(int (*func)(void *data), void *data,
+ unsigned int timeout)
+{
+ while ( !func(data) )
+ {
+ if ( !timeout-- )
+ {
+ printk("CPU%u: Timeout in %s\n", smp_processor_id(), __func__);
+ return -EBUSY;
+ }
+ udelay(1);
+ }
+
+ return 0;
+}
+
+static int wait_cpu_callin(void *nr)
+{
+ return cpumask_weight(&cpu_callin_map) >= (unsigned long)nr;
+}
+
+static int wait_cpu_callout(void *nr)
+{
+ return atomic_read(&cpu_out) >= (unsigned long)nr;
+}
+
+/*
* Load a microcode update to current CPU.
*
* If no patch is provided, the cached patch will be loaded. Microcode update
@@ -283,37 +345,105 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
return err;
}
-static long do_microcode_update(void *patch)
+static int do_microcode_update(void *patch)
{
- unsigned int cpu;
+ unsigned int cpu = smp_processor_id();
+ unsigned int cpu_nr = num_online_cpus();
+ int ret;
- /* store the patch after a successful loading */
- if ( !microcode_update_cpu(patch) && patch )
+ /* Mark loading an ucode is in progress */
+ cmpxchg(&loading_state, LOADING_EXITED, LOADING_ENTERED);
+ cpumask_set_cpu(cpu, &cpu_callin_map);
+ ret = wait_for_condition(wait_cpu_callin, (void *)(unsigned long)cpu_nr,
+ MICROCODE_CALLIN_TIMEOUT_US);
+ if ( ret )
{
- spin_lock(µcode_mutex);
- microcode_update_cache(patch);
- spin_unlock(µcode_mutex);
- patch = NULL;
+ cmpxchg(&loading_state, LOADING_ENTERED, LOADING_ABORTED);
+ return ret;
}
- if ( microcode_ops->end_update )
- microcode_ops->end_update();
+ /*
+ * Load microcode update on only one logical processor per core, or in
+ * AMD's term, one core per compute unit. The one with the lowest thread
+ * id among all siblings is chosen to perform the loading.
+ */
+ if ( (cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu))) )
+ {
+ static unsigned int panicked = 0;
+ bool monitor;
+ unsigned int done;
+ unsigned long tick = 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 = microcode_ops->apply_microcode(patch);
+ if ( !ret )
+ {
+ unsigned int cpu2;
- /* Free the patch if no CPU has loaded it successfully. */
- if ( patch )
- microcode_free_patch(patch);
+ atomic_inc(&cpu_updated);
+ /* Propagate revision number to all siblings */
+ for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))
+ per_cpu(cpu_sig, cpu2).rev = this_cpu(cpu_sig).rev;
+ }
- return 0;
+ /*
+ * The first CPU reaching here will monitor the progress and emit
+ * warning message if the duration is too long (e.g. >1 second).
+ */
+ monitor = !atomic_inc_return(&cpu_out);
+ if ( monitor )
+ tick = rdtsc_ordered();
+
+ /* Waiting for all cores or computing units finishing update */
+ done = atomic_read(&cpu_out);
+ while ( panicked && done != 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,
+ (void *)(unsigned long)(done + 1),
+ MICROCODE_UPDATE_TIMEOUT_US) &&
+ !cmpxchg(&panicked, 0, 1) )
+ panic("Timeout when finishing updating microcode (finished %u/%u)",
+ done, nr_cores);
+
+ /* Print warning message once if long time is spent here */
+ if ( monitor )
+ {
+ if ( rdtsc_ordered() - tick >= cpu_khz * 1000 )
+ {
+ printk(XENLOG_WARNING "WARNING: UPDATING MICROCODE HAS CONSUMED MORE THAN 1 SECOND!\n");
+ monitor = false;
+ }
+ }
+
+ done = atomic_read(&cpu_out);
+ }
+
+ /* Mark loading is done to unblock other threads */
+ loading_state = LOADING_EXITED;
+ }
+ else
+ {
+ while ( loading_state == LOADING_ENTERED )
+ rep_nop();
+ }
+
+ if ( microcode_ops->end_update )
+ microcode_ops->end_update();
+
+ return ret;
}
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 )
@@ -332,11 +462,18 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
goto free;
}
+ /* cpu_online_map must not change during update */
+ if ( !get_cpu_maps() )
+ {
+ ret = -EBUSY;
+ goto free;
+ }
+
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
if ( ret != 0 )
- goto free;
+ goto put;
}
patch = microcode_parse_blob(buffer, len);
@@ -344,19 +481,67 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
ret = PTR_ERR(patch);
printk(XENLOG_INFO "Parsing microcode blob error %d\n", ret);
- goto free;
+ goto put;
}
if ( !patch )
{
printk(XENLOG_INFO "No ucode found. Update aborted!\n");
ret = -EINVAL;
- goto free;
+ goto put;
+ }
+
+ cpumask_clear(&cpu_callin_map);
+ atomic_set(&cpu_out, 0);
+ atomic_set(&cpu_updated, 0);
+ loading_state = LOADING_EXITED;
+
+ /* 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);
+
+ /*
+ * We intend to disable interrupt for long time, which may lead to
+ * watchdog timeout.
+ */
+ watchdog_disable();
+ /*
+ * 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);
+ watchdog_enable();
+
+ 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);
- ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
- do_microcode_update, patch);
+ if ( updated && updated != nr_cores )
+ printk(XENLOG_ERR
+ "ERROR: Updating microcode succeeded on %u cores and failed on\n"
+ "other %u cores. A system with differing microcode revisions is\n"
+ "considered unstable. Please reboot and do not load the microcode\n"
+ "that triggers this warning!\n", updated, nr_cores - updated);
+ put:
+ put_cpu_maps();
free:
xfree(buffer);
return ret;
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading Chao Gao
@ 2019-08-05 10:43 ` Jan Beulich
2019-08-05 13:16 ` Chao Gao
2019-08-05 12:07 ` Jan Beulich
1 sibling, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 10:43 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Ashok Raj, Wei Liu, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Roger Pau Monné
On 01.08.2019 12:22, Chao Gao wrote:
> @@ -234,6 +267,35 @@ bool microcode_update_cache(struct microcode_patch *patch)
> }
>
> /*
> + * Wait for a condition to be met with a timeout (us).
> + */
> +static int wait_for_condition(int (*func)(void *data), void *data,
> + unsigned int timeout)
> +{
> + while ( !func(data) )
> + {
> + if ( !timeout-- )
> + {
> + printk("CPU%u: Timeout in %s\n", smp_processor_id(), __func__);
I don't think __func__ is helpful here for problem analysis. Instead
I think you would want to log either func or __builtin_return_address(0).
> @@ -283,37 +345,105 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
> return err;
> }
>
> -static long do_microcode_update(void *patch)
> +static int do_microcode_update(void *patch)
> {
> - unsigned int cpu;
> + unsigned int cpu = smp_processor_id();
> + unsigned int cpu_nr = num_online_cpus();
> + int ret;
>
> - /* store the patch after a successful loading */
> - if ( !microcode_update_cpu(patch) && patch )
> + /* Mark loading an ucode is in progress */
> + cmpxchg(&loading_state, LOADING_EXITED, LOADING_ENTERED);
Why cmpxchg(), especially when you don't check whether the store
has actually happened? (Same further down then.)
> + cpumask_set_cpu(cpu, &cpu_callin_map);
> + ret = wait_for_condition(wait_cpu_callin, (void *)(unsigned long)cpu_nr,
> + MICROCODE_CALLIN_TIMEOUT_US);
> + if ( ret )
> {
> - spin_lock(µcode_mutex);
> - microcode_update_cache(patch);
> - spin_unlock(µcode_mutex);
> - patch = NULL;
> + cmpxchg(&loading_state, LOADING_ENTERED, LOADING_ABORTED);
> + return ret;
> }
>
> - if ( microcode_ops->end_update )
> - microcode_ops->end_update();
> + /*
> + * Load microcode update on only one logical processor per core, or in
> + * AMD's term, one core per compute unit. The one with the lowest thread
> + * id among all siblings is chosen to perform the loading.
> + */
> + if ( (cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu))) )
> + {
> + static unsigned int panicked = 0;
> + bool monitor;
> + unsigned int done;
> + unsigned long tick = 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 = microcode_ops->apply_microcode(patch);
> + if ( !ret )
> + {
> + unsigned int cpu2;
>
> - /* Free the patch if no CPU has loaded it successfully. */
> - if ( patch )
> - microcode_free_patch(patch);
> + atomic_inc(&cpu_updated);
> + /* Propagate revision number to all siblings */
> + for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))
> + per_cpu(cpu_sig, cpu2).rev = this_cpu(cpu_sig).rev;
> + }
>
> - return 0;
> + /*
> + * The first CPU reaching here will monitor the progress and emit
> + * warning message if the duration is too long (e.g. >1 second).
> + */
> + monitor = !atomic_inc_return(&cpu_out);
> + if ( monitor )
> + tick = rdtsc_ordered();
> +
> + /* Waiting for all cores or computing units finishing update */
> + done = atomic_read(&cpu_out);
> + while ( panicked && done != nr_cores )
Don't you mean "!panicked" here, or || instead of && ? Otherwise I don't
see how the loop would ever be entered.
> + {
> + /*
> + * 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,
> + (void *)(unsigned long)(done + 1),
> + MICROCODE_UPDATE_TIMEOUT_US) &&
> + !cmpxchg(&panicked, 0, 1) )
> + panic("Timeout when finishing updating microcode (finished %u/%u)",
> + done, nr_cores);
> +
> + /* Print warning message once if long time is spent here */
> + if ( monitor )
> + {
> + if ( rdtsc_ordered() - tick >= cpu_khz * 1000 )
> + {
> + printk(XENLOG_WARNING "WARNING: UPDATING MICROCODE HAS CONSUMED MORE THAN 1 SECOND!\n");
Please save a little bit on line length by wrapping lines before the
initial quote.
> + monitor = false;
> + }
> + }
Please combine both if()-s. And please also consider dropping the
"monitor" local variable - "tick" being non-zero can easily replace
it afaics.
> + done = atomic_read(&cpu_out);
> + }
> +
> + /* Mark loading is done to unblock other threads */
> + loading_state = LOADING_EXITED;
> + }
> + else
> + {
> + while ( loading_state == LOADING_ENTERED )
> + rep_nop();
Instead of the "for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))"
above, wouldn't it be more logical to have each thread update its
signature here?
Also - do you perhaps mean cpu_relax() instead of rep_nop()?
> @@ -344,19 +481,67 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> {
> ret = PTR_ERR(patch);
> printk(XENLOG_INFO "Parsing microcode blob error %d\n", ret);
> - goto free;
> + goto put;
> }
>
> if ( !patch )
> {
> printk(XENLOG_INFO "No ucode found. Update aborted!\n");
> ret = -EINVAL;
> - goto free;
> + goto put;
> + }
> +
> + cpumask_clear(&cpu_callin_map);
> + atomic_set(&cpu_out, 0);
> + atomic_set(&cpu_updated, 0);
> + loading_state = LOADING_EXITED;
> +
> + /* 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);
> +
> + /*
> + * We intend to disable interrupt for long time, which may lead to
> + * watchdog timeout.
> + */
> + watchdog_disable();
> + /*
> + * 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);
> + watchdog_enable();
> +
> + 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);
>
> - ret = continue_hypercall_on_cpu(cpumask_first(&cpu_online_map),
> - do_microcode_update, patch);
> + if ( updated && updated != nr_cores )
> + printk(XENLOG_ERR
> + "ERROR: Updating microcode succeeded on %u cores and failed on\n"
> + "other %u cores. A system with differing microcode revisions is\n"
> + "considered unstable. Please reboot and do not load the microcode\n"
> + "that triggers this warning!\n", updated, nr_cores - updated);
Note that for such multi-line log messages you need to prefix XENLOG_*
to every one of them.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading
2019-08-05 10:43 ` Jan Beulich
@ 2019-08-05 13:16 ` Chao Gao
2019-08-05 13:28 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-05 13:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Ashok Raj, Wei Liu, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Roger Pau Monné
On Mon, Aug 05, 2019 at 10:43:16AM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> @@ -234,6 +267,35 @@ bool microcode_update_cache(struct microcode_patch *patch)
>> }
>>
>> /*
>> + * Wait for a condition to be met with a timeout (us).
>> + */
>> +static int wait_for_condition(int (*func)(void *data), void *data,
>> + unsigned int timeout)
>> +{
>> + while ( !func(data) )
>> + {
>> + if ( !timeout-- )
>> + {
>> + printk("CPU%u: Timeout in %s\n", smp_processor_id(), __func__);
>
>I don't think __func__ is helpful here for problem analysis. Instead
>I think you would want to log either func or __builtin_return_address(0).
Will do.
>
>> @@ -283,37 +345,105 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
>> return err;
>> }
>>
>> -static long do_microcode_update(void *patch)
>> +static int do_microcode_update(void *patch)
>> {
>> - unsigned int cpu;
>> + unsigned int cpu = smp_processor_id();
>> + unsigned int cpu_nr = num_online_cpus();
>> + int ret;
>>
>> - /* store the patch after a successful loading */
>> - if ( !microcode_update_cpu(patch) && patch )
>> + /* Mark loading an ucode is in progress */
>> + cmpxchg(&loading_state, LOADING_EXITED, LOADING_ENTERED);
>
>Why cmpxchg(), especially when you don't check whether the store
>has actually happened? (Same further down then.)
loading_state is set to "LOADING_EXITED" by the caller. So the first CPU
coming here would store "LOADING_ENTERED" to it. And we don't need
special handling for the CPU that sets the state, so the return value
isn't checked.
Here are my considerations about how to set the state:
1) We cannot set the state in the caller. Because we would use this
state to block #NMI handling. Setting the state in the caller may
break stop_machine_run mechanism with the patch 16.
2) The first CPU reaching here should set the state (it means we cannot
choose one CPU, e.g. BSP, to do it). With this, #NMI handling is
disabled system-wise before any CPU calls in. Otherwise, if there is
an exception for a CPU, it may be still in #NMI handling, when its
sibling thread starts loading an ucode.
3) A simple assignment on every CPU is problematic in some cases.
For example, if one CPU comes in after other CPUs timed out and left,
it might set the state to "LOADING_ENTERED" and be blocked in #NMI
handling forever with patch 16.
That's why I choose to use a cmpxhg().
Regarding the cmpxchg() in error-handling below, it can be replaced by
a simple assignment. But I am not sure whether it is better to use
cmpxchg() considering cache line bouncing.
>
>> + cpumask_set_cpu(cpu, &cpu_callin_map);
>> + ret = wait_for_condition(wait_cpu_callin, (void *)(unsigned long)cpu_nr,
>> + MICROCODE_CALLIN_TIMEOUT_US);
>> + if ( ret )
>> {
>> - spin_lock(µcode_mutex);
>> - microcode_update_cache(patch);
>> - spin_unlock(µcode_mutex);
>> - patch = NULL;
>> + cmpxchg(&loading_state, LOADING_ENTERED, LOADING_ABORTED);
>> + return ret;
>> }
>>
>> - if ( microcode_ops->end_update )
>> - microcode_ops->end_update();
>> + /*
>> + * Load microcode update on only one logical processor per core, or in
>> + * AMD's term, one core per compute unit. The one with the lowest thread
>> + * id among all siblings is chosen to perform the loading.
>> + */
>> + if ( (cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu))) )
>> + {
>> + static unsigned int panicked = 0;
>> + bool monitor;
>> + unsigned int done;
>> + unsigned long tick = 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 = microcode_ops->apply_microcode(patch);
>> + if ( !ret )
>> + {
>> + unsigned int cpu2;
>>
>> - /* Free the patch if no CPU has loaded it successfully. */
>> - if ( patch )
>> - microcode_free_patch(patch);
>> + atomic_inc(&cpu_updated);
>> + /* Propagate revision number to all siblings */
>> + for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))
>> + per_cpu(cpu_sig, cpu2).rev = this_cpu(cpu_sig).rev;
>> + }
>>
>> - return 0;
>> + /*
>> + * The first CPU reaching here will monitor the progress and emit
>> + * warning message if the duration is too long (e.g. >1 second).
>> + */
>> + monitor = !atomic_inc_return(&cpu_out);
>> + if ( monitor )
>> + tick = rdtsc_ordered();
>> +
>> + /* Waiting for all cores or computing units finishing update */
>> + done = atomic_read(&cpu_out);
>> + while ( panicked && done != nr_cores )
>
>Don't you mean "!panicked" here, or || instead of && ? Otherwise I don't
>see how the loop would ever be entered.
Sorry. It should be !panicked.
Other comments are reasonable and I will follow your suggestions.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading
2019-08-05 13:16 ` Chao Gao
@ 2019-08-05 13:28 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 13:28 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Ashok Raj, Wei Liu, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Roger Pau Monné
On 05.08.2019 15:16, Chao Gao wrote:
> On Mon, Aug 05, 2019 at 10:43:16AM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> @@ -283,37 +345,105 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
>>> return err;
>>> }
>>>
>>> -static long do_microcode_update(void *patch)
>>> +static int do_microcode_update(void *patch)
>>> {
>>> - unsigned int cpu;
>>> + unsigned int cpu = smp_processor_id();
>>> + unsigned int cpu_nr = num_online_cpus();
>>> + int ret;
>>>
>>> - /* store the patch after a successful loading */
>>> - if ( !microcode_update_cpu(patch) && patch )
>>> + /* Mark loading an ucode is in progress */
>>> + cmpxchg(&loading_state, LOADING_EXITED, LOADING_ENTERED);
>>
>> Why cmpxchg(), especially when you don't check whether the store
>> has actually happened? (Same further down then.)
>
> loading_state is set to "LOADING_EXITED" by the caller. So the first CPU
> coming here would store "LOADING_ENTERED" to it. And we don't need
> special handling for the CPU that sets the state, so the return value
> isn't checked.
>
> Here are my considerations about how to set the state:
> 1) We cannot set the state in the caller. Because we would use this
> state to block #NMI handling. Setting the state in the caller may
> break stop_machine_run mechanism with the patch 16.
>
> 2) The first CPU reaching here should set the state (it means we cannot
> choose one CPU, e.g. BSP, to do it). With this, #NMI handling is
> disabled system-wise before any CPU calls in. Otherwise, if there is
> an exception for a CPU, it may be still in #NMI handling, when its
> sibling thread starts loading an ucode.
>
> 3) A simple assignment on every CPU is problematic in some cases.
> For example, if one CPU comes in after other CPUs timed out and left,
> it might set the state to "LOADING_ENTERED" and be blocked in #NMI
> handling forever with patch 16.
>
> That's why I choose to use a cmpxhg().
But if the expected incoming state is _not_ the one actually found
in memory, then the cmpxchg() will silently degenerate to a no-op,
without anyone noticing other than the system misbehaving in an
unpredictable way.
On the whole, seeing your explanation above, there is merit to
using cmpxchg(). But at the very least this background needs to be
put in the description; perhaps better in a code comment next to
the variable definition.
> Regarding the cmpxchg() in error-handling below, it can be replaced by
> a simple assignment. But I am not sure whether it is better to use
> cmpxchg() considering cache line bouncing.
I didn't think MOV would cause more heavy cacheline bouncing compared
to CMPXCHG.
>>> + cpumask_set_cpu(cpu, &cpu_callin_map);
>>> + ret = wait_for_condition(wait_cpu_callin, (void *)(unsigned long)cpu_nr,
>>> + MICROCODE_CALLIN_TIMEOUT_US);
>>> + if ( ret )
>>> {
>>> - spin_lock(µcode_mutex);
>>> - microcode_update_cache(patch);
>>> - spin_unlock(µcode_mutex);
>>> - patch = NULL;
>>> + cmpxchg(&loading_state, LOADING_ENTERED, LOADING_ABORTED);
>>> + return ret;
>>> }
>>>
>>> - if ( microcode_ops->end_update )
>>> - microcode_ops->end_update();
>>> + /*
>>> + * Load microcode update on only one logical processor per core, or in
>>> + * AMD's term, one core per compute unit. The one with the lowest thread
>>> + * id among all siblings is chosen to perform the loading.
>>> + */
>>> + if ( (cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu))) )
>>> + {
>>> + static unsigned int panicked = 0;
>>> + bool monitor;
>>> + unsigned int done;
>>> + unsigned long tick = 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 = microcode_ops->apply_microcode(patch);
>>> + if ( !ret )
>>> + {
>>> + unsigned int cpu2;
>>>
>>> - /* Free the patch if no CPU has loaded it successfully. */
>>> - if ( patch )
>>> - microcode_free_patch(patch);
>>> + atomic_inc(&cpu_updated);
>>> + /* Propagate revision number to all siblings */
>>> + for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))
>>> + per_cpu(cpu_sig, cpu2).rev = this_cpu(cpu_sig).rev;
>>> + }
>>>
>>> - return 0;
>>> + /*
>>> + * The first CPU reaching here will monitor the progress and emit
>>> + * warning message if the duration is too long (e.g. >1 second).
>>> + */
>>> + monitor = !atomic_inc_return(&cpu_out);
>>> + if ( monitor )
>>> + tick = rdtsc_ordered();
>>> +
>>> + /* Waiting for all cores or computing units finishing update */
>>> + done = atomic_read(&cpu_out);
>>> + while ( panicked && done != nr_cores )
>>
>> Don't you mean "!panicked" here, or || instead of && ? Otherwise I don't
>> see how the loop would ever be entered.
>
> Sorry. It should be !panicked.
I.e. you mean to exit the loop on all CPUs once one of them has
invoked panic()? I did append the alternative for a reason - it
seems to me that keeping CPUs in this loop after a panic() might
be better.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading Chao Gao
2019-08-05 10:43 ` Jan Beulich
@ 2019-08-05 12:07 ` Jan Beulich
1 sibling, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 12:07 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Ashok Raj, Wei Liu, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Roger Pau Monné
On 01.08.2019 12:22, Chao Gao wrote:
> @@ -283,37 +345,105 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
> return err;
> }
>
> -static long do_microcode_update(void *patch)
> +static int do_microcode_update(void *patch)
> {
> - unsigned int cpu;
> + unsigned int cpu = smp_processor_id();
> + unsigned int cpu_nr = num_online_cpus();
> + int ret;
>
> - /* store the patch after a successful loading */
> - if ( !microcode_update_cpu(patch) && patch )
> + /* Mark loading an ucode is in progress */
> + cmpxchg(&loading_state, LOADING_EXITED, LOADING_ENTERED);
> + cpumask_set_cpu(cpu, &cpu_callin_map);
> + ret = wait_for_condition(wait_cpu_callin, (void *)(unsigned long)cpu_nr,
> + MICROCODE_CALLIN_TIMEOUT_US);
One more minor request - just like you do here, ...
> + if ( ret )
> {
> - spin_lock(µcode_mutex);
> - microcode_update_cache(patch);
> - spin_unlock(µcode_mutex);
> - patch = NULL;
> + cmpxchg(&loading_state, LOADING_ENTERED, LOADING_ABORTED);
> + return ret;
> }
>
> - if ( microcode_ops->end_update )
> - microcode_ops->end_update();
> + /*
> + * Load microcode update on only one logical processor per core, or in
> + * AMD's term, one core per compute unit. The one with the lowest thread
> + * id among all siblings is chosen to perform the loading.
> + */
> + if ( (cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu))) )
> + {
> + static unsigned int panicked = 0;
> + bool monitor;
> + unsigned int done;
> + unsigned long tick = 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 = microcode_ops->apply_microcode(patch);
> + if ( !ret )
> + {
> + unsigned int cpu2;
>
> - /* Free the patch if no CPU has loaded it successfully. */
> - if ( patch )
> - microcode_free_patch(patch);
> + atomic_inc(&cpu_updated);
> + /* Propagate revision number to all siblings */
> + for_each_cpu(cpu2, per_cpu(cpu_sibling_mask, cpu))
> + per_cpu(cpu_sig, cpu2).rev = this_cpu(cpu_sig).rev;
> + }
>
> - return 0;
> + /*
> + * The first CPU reaching here will monitor the progress and emit
> + * warning message if the duration is too long (e.g. >1 second).
> + */
> + monitor = !atomic_inc_return(&cpu_out);
> + if ( monitor )
> + tick = rdtsc_ordered();
> +
> + /* Waiting for all cores or computing units finishing update */
> + done = atomic_read(&cpu_out);
> + while ( panicked && done != 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,
> + (void *)(unsigned long)(done + 1),
> + MICROCODE_UPDATE_TIMEOUT_US) &&
... could you please omit the redundant & on the first argument here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* [Xen-devel] [PATCH v8 15/16] microcode: remove microcode_update_lock
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (13 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 14/16] x86/microcode: Synchronize late microcode loading Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode Chao Gao
` (2 subsequent siblings)
17 siblings, 0 replies; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
microcode_update_lock is to prevent logic threads of a same core from
updating microcode at the same time. But due to using a global lock, it
also prevented parallel microcode updating on different cores.
Remove this lock in order to update microcode in parallel. It is safe
because we have already ensured serialization of sibling threads at the
caller side.
1.For late microcode update, do_microcode_update() ensures that only one
sibiling thread of a core can update microcode.
2.For microcode update during system startup or CPU-hotplug,
microcode_mutex() guarantees update serialization of logical threads.
3.get/put_cpu_bitmaps() prevents the concurrency of CPU-hotplug and
late microcode update.
Note that printk in apply_microcode() and svm_host_osvm_init() (for AMD
only) are still processed sequentially.
Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
Changes in v7:
- reworked. Remove complex lock logics introduced in v5 and v6. The microcode
patch to be applied is passed as an argument without any global variable. Thus
no lock is added to serialize potential readers/writers. Callers of
apply_microcode() will guarantee the correctness: the patch poninted by the
arguments won't be changed by others.
Changes in v6:
- introduce early_ucode_update_lock to serialize early ucode update.
Changes in v5:
- newly add
---
xen/arch/x86/microcode_amd.c | 8 +-------
xen/arch/x86/microcode_intel.c | 8 +-------
2 files changed, 2 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 93d2a98..3a58dba 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)
{
@@ -249,7 +246,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();
@@ -261,15 +257,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 8780be0..016e381 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();
@@ -300,7 +297,6 @@ static struct microcode_patch *alloc_microcode_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();
@@ -312,8 +308,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);
@@ -326,7 +321,6 @@ static int apply_microcode(const struct microcode_patch *patch)
rdmsrl(MSR_IA32_UCODE_REV, msr_content);
val[1] = (uint32_t)(msr_content >> 32);
- spin_unlock_irqrestore(µcode_update_lock, flags);
if ( val[1] != mc_intel->hdr.rev )
{
printk(KERN_ERR "microcode: CPU%d update from revision "
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (14 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 15/16] microcode: remove microcode_update_lock Chao Gao
@ 2019-08-01 10:22 ` Chao Gao
2019-08-05 12:11 ` Jan Beulich
2019-08-01 18:15 ` [Xen-devel] [PATCH v8 00/16] improve late microcode loading Andrew Cooper
2019-08-02 10:59 ` Roger Pau Monné
17 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-01 10:22 UTC (permalink / raw)
To: xen-devel
Cc: Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich, Chao Gao,
Roger Pau Monné
register an nmi callback. And this callback does busy-loop on threads
which are waiting for loading completion if 'loading_ucode' is true.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v8:
- new
---
xen/arch/x86/microcode.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 67549be..4ac7e93 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -38,6 +38,7 @@
#include <asm/delay.h>
#include <asm/msr.h>
+#include <asm/nmi.h>
#include <asm/processor.h>
#include <asm/setup.h>
#include <asm/microcode.h>
@@ -439,12 +440,37 @@ static int do_microcode_update(void *patch)
return ret;
}
+static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
+{
+ bool print = false;
+
+ /* The first thread of a core is to load an update. Don't block it. */
+ if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
+ return 0;
+
+ if ( loading_state == LOADING_ENTERED )
+ {
+ cpumask_set_cpu(cpu, &cpu_callin_map);
+ printk(XENLOG_DEBUG "CPU%u enters %s\n", smp_processor_id(), __func__);
+ print = true;
+ }
+
+ while ( loading_state == LOADING_ENTERED )
+ rep_nop();
+
+ if ( print )
+ printk(XENLOG_DEBUG "CPU%u exits %s\n", smp_processor_id(), __func__);
+
+ return 0;
+}
+
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;
+ nmi_callback_t *saved_nmi_callback;
if ( len != (uint32_t)len )
return -E2BIG;
@@ -509,6 +535,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
* watchdog timeout.
*/
watchdog_disable();
+
+ saved_nmi_callback = set_nmi_callback(microcode_nmi_callback);
/*
* Late loading dance. Why the heavy-handed stop_machine effort?
*
@@ -521,6 +549,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
* conservative and good.
*/
ret = stop_machine_run(do_microcode_update, patch, NR_CPUS);
+ set_nmi_callback(saved_nmi_callback);
watchdog_enable();
updated = atomic_read(&cpu_updated);
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode Chao Gao
@ 2019-08-05 12:11 ` Jan Beulich
2019-08-07 7:59 ` Chao Gao
0 siblings, 1 reply; 49+ messages in thread
From: Jan Beulich @ 2019-08-05 12:11 UTC (permalink / raw)
To: Chao Gao, Andrew Cooper
Cc: xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 01.08.2019 12:22, Chao Gao wrote:
> @@ -439,12 +440,37 @@ static int do_microcode_update(void *patch)
> return ret;
> }
>
> +static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
> +{
> + bool print = false;
> +
> + /* The first thread of a core is to load an update. Don't block it. */
> + if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
> + return 0;
> +
> + if ( loading_state == LOADING_ENTERED )
> + {
> + cpumask_set_cpu(cpu, &cpu_callin_map);
> + printk(XENLOG_DEBUG "CPU%u enters %s\n", smp_processor_id(), __func__);
Here and ...
> + print = true;
> + }
> +
> + while ( loading_state == LOADING_ENTERED )
> + rep_nop();
> +
> + if ( print )
> + printk(XENLOG_DEBUG "CPU%u exits %s\n", smp_processor_id(), __func__);
... here - why smp_processor_id() when you can use "cpu"? And what
use is __func__ here?
The rep_nop() above also presumably wants to be cpu_relax() again.
But on the whole I was really hoping for more aggressive disabling
of NMI handling, more like (but of course not quite as heavy as)
the crash path wiring the IDT entry to trap_nop(). Andrew, I'm
curious to learn what you're thinking would be best here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode
2019-08-05 12:11 ` Jan Beulich
@ 2019-08-07 7:59 ` Chao Gao
2019-08-07 8:44 ` Jan Beulich
0 siblings, 1 reply; 49+ messages in thread
From: Chao Gao @ 2019-08-07 7:59 UTC (permalink / raw)
To: Jan Beulich
Cc: Andrew Cooper, Roger Pau Monné, Ashok Raj, Wei Liu, xen-devel
On Mon, Aug 05, 2019 at 12:11:01PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> @@ -439,12 +440,37 @@ static int do_microcode_update(void *patch)
>> return ret;
>> }
>>
>> +static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
>> +{
>> + bool print = false;
>> +
>> + /* The first thread of a core is to load an update. Don't block it. */
>> + if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>> + return 0;
>> +
>> + if ( loading_state == LOADING_ENTERED )
>> + {
>> + cpumask_set_cpu(cpu, &cpu_callin_map);
>> + printk(XENLOG_DEBUG "CPU%u enters %s\n", smp_processor_id(), __func__);
>
>Here and ...
>
>> + print = true;
>> + }
>> +
>> + while ( loading_state == LOADING_ENTERED )
>> + rep_nop();
>> +
>> + if ( print )
>> + printk(XENLOG_DEBUG "CPU%u exits %s\n", smp_processor_id(), __func__);
>
>... here - why smp_processor_id() when you can use "cpu"? And what
>use is __func__ here?
>
>The rep_nop() above also presumably wants to be cpu_relax() again.
>
>But on the whole I was really hoping for more aggressive disabling
>of NMI handling, more like (but of course not quite as heavy as)
>the crash path wiring the IDT entry to trap_nop().
Hi Jan,
I agree with you that it should be more aggressive. This patch is
problematic considering there is a lot of code before reaching this
callback (especially, SPEC_CTRL_ENTRY_FROM_INTR_IST which may write
MSR_SPEC_CTRL).
In my mind, we have two options to solve the issue:
1. Wire the IDT entry to trap_nop() like the crash path.
2. Enhance this patch: A thread which is not going to load an update
is forced to send an #NMI to itself to enter the callback, do
busy-loop until completion of loading ucode on all cores.
With this method, no #NMI delivery, or MSR write would happen on this
threads during microcode update.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode
2019-08-07 7:59 ` Chao Gao
@ 2019-08-07 8:44 ` Jan Beulich
0 siblings, 0 replies; 49+ messages in thread
From: Jan Beulich @ 2019-08-07 8:44 UTC (permalink / raw)
To: Chao Gao
Cc: Andrew Cooper, xen-devel, Ashok Raj, Wei Liu, Roger Pau Monné
On 07.08.2019 09:59, Chao Gao wrote:
> On Mon, Aug 05, 2019 at 12:11:01PM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> @@ -439,12 +440,37 @@ static int do_microcode_update(void *patch)
>>> return ret;
>>> }
>>>
>>> +static int microcode_nmi_callback(const struct cpu_user_regs *regs, int cpu)
>>> +{
>>> + bool print = false;
>>> +
>>> + /* The first thread of a core is to load an update. Don't block it. */
>>> + if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>>> + return 0;
>>> +
>>> + if ( loading_state == LOADING_ENTERED )
>>> + {
>>> + cpumask_set_cpu(cpu, &cpu_callin_map);
>>> + printk(XENLOG_DEBUG "CPU%u enters %s\n", smp_processor_id(), __func__);
>>
>> Here and ...
>>
>>> + print = true;
>>> + }
>>> +
>>> + while ( loading_state == LOADING_ENTERED )
>>> + rep_nop();
>>> +
>>> + if ( print )
>>> + printk(XENLOG_DEBUG "CPU%u exits %s\n", smp_processor_id(), __func__);
>>
>> ... here - why smp_processor_id() when you can use "cpu"? And what
>> use is __func__ here?
>>
>> The rep_nop() above also presumably wants to be cpu_relax() again.
>>
>> But on the whole I was really hoping for more aggressive disabling
>> of NMI handling, more like (but of course not quite as heavy as)
>> the crash path wiring the IDT entry to trap_nop().
>
> I agree with you that it should be more aggressive. This patch is
> problematic considering there is a lot of code before reaching this
> callback (especially, SPEC_CTRL_ENTRY_FROM_INTR_IST which may write
> MSR_SPEC_CTRL).
>
> In my mind, we have two options to solve the issue:
> 1. Wire the IDT entry to trap_nop() like the crash path.
As said, this is not directly an option - at the very least a thread
should record the fact that there was an NMI, such that it can
handle it after the ucode update has completed.
> 2. Enhance this patch: A thread which is not going to load an update
> is forced to send an #NMI to itself to enter the callback, do
> busy-loop until completion of loading ucode on all cores.
> With this method, no #NMI delivery, or MSR write would happen on this
> threads during microcode update.
This sounds doable at the first glance; iirc the CPU would latch
another NMI while NMIs are blocked due to there not having been an
IRET yet after the last one was raised. There would still be some
ambiguity in case the self-NMI and an actual one arrived at about
the same time, but I guess we need to live with this small window.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread
* Re: [Xen-devel] [PATCH v8 00/16] improve late microcode loading
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (15 preceding siblings ...)
2019-08-01 10:22 ` [Xen-devel] [PATCH v8 16/16] microcode: block #NMI handling when loading an ucode Chao Gao
@ 2019-08-01 18:15 ` Andrew Cooper
2019-08-02 10:59 ` Roger Pau Monné
17 siblings, 0 replies; 49+ messages in thread
From: Andrew Cooper @ 2019-08-01 18:15 UTC (permalink / raw)
To: Chao Gao, xen-devel
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Jan Beulich, Boris Ostrovsky,
Brian Woods, Suravee Suthikulpanit, Roger Pau Monné
On 01/08/2019 11:22, Chao Gao wrote:
> This series includes below changes:
> 1. Patch 1: always read microcode revision at boot time
> 2. Patch 2: an userspace tool for late microcode update
To get things started, I've committed these two changes. I'm afraid I
don't have time immediately to review the rest of the series, but will
try to find time.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread* Re: [Xen-devel] [PATCH v8 00/16] improve late microcode loading
2019-08-01 10:22 [Xen-devel] [PATCH v8 00/16] improve late microcode loading Chao Gao
` (16 preceding siblings ...)
2019-08-01 18:15 ` [Xen-devel] [PATCH v8 00/16] improve late microcode loading Andrew Cooper
@ 2019-08-02 10:59 ` Roger Pau Monné
17 siblings, 0 replies; 49+ messages in thread
From: Roger Pau Monné @ 2019-08-02 10:59 UTC (permalink / raw)
To: Chao Gao
Cc: Sergey Dyasli, Ashok Raj, Wei Liu, Andrew Cooper, Jan Beulich,
xen-devel, Boris Ostrovsky, Brian Woods, Suravee Suthikulpanit
On Thu, Aug 01, 2019 at 06:22:35PM +0800, Chao Gao wrote:
> Changes in version 8:
> - block #NMI handling during microcode loading (Patch 16)
> - Don't assume that all CPUs in the system have loaded a same ucode.
> So when parsing a blob, we attempt to save a patch as long as it matches
> with current cpu signature regardless of the revision of the patch.
> And also for loading, we only require the patch to be loaded isn't old
> than the cached one.
> - store an update after the first successful loading on a CPU
> - remove the patch that calls wbinvd() unconditionally before microcode
> loading. It is under internal discussion.
> - divide two big patches into several patches to improve readability.
Thanks for following up on this!
I will try to review it between Monday and Tuesday.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 49+ messages in thread