* [PATCH v4 1/6] microcode/intel: extend microcode_update_match()
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 10:58 ` Roger Pau Monné
2018-11-28 5:34 ` [PATCH v4 2/6] microcode: save all microcodes which pass sanity check Chao Gao
` (4 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Chao Gao
to a more generic function. The benefit is that this function can be
used to check whether a microcode is newer than another as well. We
rely on this function to decide to perform a replacement or an add when
updating the global microcode cache (introduced by later patches in
this series).
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
xen/arch/x86/microcode_intel.c | 57 +++++++++++++++++++++++-------------------
1 file changed, 31 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 9657575..8d9a3b2 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -127,14 +127,37 @@ 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)
+enum {
+ OLD_UCODE, /* signature matched, but revision id isn't newer */
+ NEW_UCODE, /* signature matched, but revision id is newer */
+ MIS_UCODE, /* signature mismatched */
+};
+
+static int microcode_update_match(const void *mc,
+ unsigned int sig, unsigned int pf, unsigned int rev)
{
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
+ 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;
- return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
- (mc_header->rev > uci->cpu_sig.rev));
+ if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
+ return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+ if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) )
+ return MIS_UCODE;
+
+ 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 ( sigmatch(sig, ext_sig->sig, pf, ext_sig->pf) )
+ return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+ ext_sig++;
+ }
+ return MIS_UCODE;
}
static int microcode_sanity_check(void *mc)
@@ -236,31 +259,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);
--
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] 41+ messages in thread* Re: [PATCH v4 1/6] microcode/intel: extend microcode_update_match()
2018-11-28 5:34 ` [PATCH v4 1/6] microcode/intel: extend microcode_update_match() Chao Gao
@ 2018-11-28 10:58 ` Roger Pau Monné
2018-11-29 2:00 ` Chao Gao
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 10:58 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:34:11PM +0800, Chao Gao wrote:
> to a more generic function. The benefit is that this function can be
> used to check whether a microcode is newer than another as well. We
> rely on this function to decide to perform a replacement or an add when
> updating the global microcode cache (introduced by later patches in
> this series).
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> xen/arch/x86/microcode_intel.c | 57 +++++++++++++++++++++++-------------------
> 1 file changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index 9657575..8d9a3b2 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -127,14 +127,37 @@ 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)
> +enum {
> + OLD_UCODE, /* signature matched, but revision id isn't newer */
> + NEW_UCODE, /* signature matched, but revision id is newer */
> + MIS_UCODE, /* signature mismatched */
> +};
Shouldn't you give a name to this type ...
> +static int microcode_update_match(const void *mc,
... so that this function can return it instead of int?
> + unsigned int sig, unsigned int pf, unsigned int rev)
> {
> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
> + const struct microcode_header_intel *mc_header = mc;
> + const struct extended_sigtable *ext_header;
> + unsigned long total_size = get_totalsize(mc_header);
size_t might be more appropriate here.
> + int ext_sigcount, i;
unsigned int.
> + struct extended_signature *ext_sig;
const?
>
> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
> - (mc_header->rev > uci->cpu_sig.rev));
> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
> +
> + if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) )
> + return MIS_UCODE;
Shouldn't you perform this check before the signature check?
> +
> + ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE;
> + ext_sigcount = ext_header->count;
> + ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
You are dropping the const here AFAICT by casting to void *.
> + for ( i = 0; i < ext_sigcount; i++ )
> + {
> + if ( sigmatch(sig, ext_sig->sig, pf, ext_sig->pf) )
> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
> + ext_sig++;
> + }
I would add a newline here for readability.
> + return MIS_UCODE;
> }
>
> static int microcode_sanity_check(void *mc)
> @@ -236,31 +259,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;
Shouldn't you differentiate between the function returning OLD_UCODE
or MIS_UCODE? I would expect that trying to load a mismatched UCODE
would trigger some kind of message from Xen.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 1/6] microcode/intel: extend microcode_update_match()
2018-11-28 10:58 ` Roger Pau Monné
@ 2018-11-29 2:00 ` Chao Gao
2018-11-29 9:14 ` Roger Pau Monné
0 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-29 2:00 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 11:58:06AM +0100, Roger Pau Monné wrote:
>On Wed, Nov 28, 2018 at 01:34:11PM +0800, Chao Gao wrote:
>> to a more generic function. The benefit is that this function can be
>> used to check whether a microcode is newer than another as well. We
>> rely on this function to decide to perform a replacement or an add when
>> updating the global microcode cache (introduced by later patches in
>> this series).
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> xen/arch/x86/microcode_intel.c | 57 +++++++++++++++++++++++-------------------
>> 1 file changed, 31 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
>> index 9657575..8d9a3b2 100644
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -127,14 +127,37 @@ 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)
>> +enum {
>> + OLD_UCODE, /* signature matched, but revision id isn't newer */
>> + NEW_UCODE, /* signature matched, but revision id is newer */
>> + MIS_UCODE, /* signature mismatched */
>> +};
>
>Shouldn't you give a name to this type ...
>
>> +static int microcode_update_match(const void *mc,
>
>... so that this function can return it instead of int?
>
>> + unsigned int sig, unsigned int pf, unsigned int rev)
>> {
>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> + const struct microcode_header_intel *mc_header = mc;
>> + const struct extended_sigtable *ext_header;
>> + unsigned long total_size = get_totalsize(mc_header);
>
>size_t might be more appropriate here.
>
>> + int ext_sigcount, i;
>
>unsigned int.
>
>> + struct extended_signature *ext_sig;
>
>const?
>
>>
>> - return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>> - (mc_header->rev > uci->cpu_sig.rev));
>> + if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>> +
>> + if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) )
>> + return MIS_UCODE;
>
>Shouldn't you perform this check before the signature check?
I think this check shouldn't be here. Given that this check is also
done in microcode_sanity_check(), I will remove this check here.
>
>> +
>> + ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE;
>> + ext_sigcount = ext_header->count;
>> + ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
>
>You are dropping the const here AFAICT by casting to void *.
>
>> + for ( i = 0; i < ext_sigcount; i++ )
>> + {
>> + if ( sigmatch(sig, ext_sig->sig, pf, ext_sig->pf) )
>> + return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>> + ext_sig++;
>> + }
>
>I would add a newline here for readability.
>
>> + return MIS_UCODE;
>> }
>>
>> static int microcode_sanity_check(void *mc)
>> @@ -236,31 +259,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;
>
>Shouldn't you differentiate between the function returning OLD_UCODE
>or MIS_UCODE? I would expect that trying to load a mismatched UCODE
>would trigger some kind of message from Xen.
I don't differentiate these two cases. For both of them, we do nothing.
Actually, I add a message "No newer or matched microcode found" in patch 4
for them (Currently each cpu parses the file locally, if we add
an error message, it will show up many times). However, if you are to load
a corrupted file, another error will be prompted.
Other comments are fine with me.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 1/6] microcode/intel: extend microcode_update_match()
2018-11-29 2:00 ` Chao Gao
@ 2018-11-29 9:14 ` Roger Pau Monné
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-29 9:14 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Thu, Nov 29, 2018 at 10:00:34AM +0800, Chao Gao wrote:
> On Wed, Nov 28, 2018 at 11:58:06AM +0100, Roger Pau Monné wrote:
> >On Wed, Nov 28, 2018 at 01:34:11PM +0800, Chao Gao wrote:
> >> static int microcode_sanity_check(void *mc)
> >> @@ -236,31 +259,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;
> >
> >Shouldn't you differentiate between the function returning OLD_UCODE
> >or MIS_UCODE? I would expect that trying to load a mismatched UCODE
> >would trigger some kind of message from Xen.
>
> I don't differentiate these two cases. For both of them, we do nothing.
> Actually, I add a message "No newer or matched microcode found" in patch 4
> for them (Currently each cpu parses the file locally, if we add
> an error message, it will show up many times). However, if you are to load
> a corrupted file, another error will be prompted.
What I wanted to point out is that you don't need 3 different return
values if you only differentiate between two of them. Ie: a boolean
will achieve the same result here.
If those 3 different return values are indeed used by patch 4 then
that's fine, I didn't realize it.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
2018-11-28 5:34 ` [PATCH v4 1/6] microcode/intel: extend microcode_update_match() Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 12:00 ` Roger Pau Monné
2018-11-28 5:34 ` [PATCH v4 3/6] microcode: delete 'mc' field from struct ucode_cpu_info Chao Gao
` (3 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Chao Gao
... and search caches to find a suitable one when loading.
With this cache, the existing 'uci->mc' structure is redundent.
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>
---
xen/arch/x86/microcode.c | 2 +
xen/arch/x86/microcode_amd.c | 93 +++++++++++++++++++++++++++++++++++---
xen/arch/x86/microcode_intel.c | 99 ++++++++++++++++++++++++++++++++++++++---
xen/include/asm-x86/microcode.h | 11 +++++
4 files changed, 193 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 4163f50..4f2db88 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -61,6 +61,8 @@ static struct ucode_mod_blob __initdata ucode_blob;
*/
static bool_t __initdata ucode_scan;
+LIST_HEAD(microcode_cache);
+
void __init microcode_set_module(unsigned int idx)
{
ucode_mod_idx = idx;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index fba44cc..a686a87 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -190,22 +190,90 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd,
return 1;
}
+static struct ucode_patch *alloc_ucode_patch(struct microcode_amd *mc_amd)
+{
+ struct ucode_patch *ucode_patch = xmalloc(struct ucode_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 ( !ucode_patch || !cache || !mpb || !equiv_cpu_table )
+ return ERR_PTR(-ENOMEM);
+
+ memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table,
+ mc_amd->equiv_cpu_table_size);
+ memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size);
+ cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size;
+ cache->mpb_size = mc_amd->mpb_size;
+ ucode_patch->data = cache;
+ return ucode_patch;
+}
+
+static void free_ucode_patch(struct ucode_patch *ucode_patch)
+{
+ struct microcode_amd *mc_amd = ucode_patch->data;
+
+ xfree(mc_amd->equiv_cpu_table);
+ xfree(mc_amd->mpb);
+ xfree(mc_amd);
+ xfree(ucode_patch);
+}
+
+/*
+ * save a micrcode to the cache list
+ * return 1: added successfully
+ * 0: replaced an existing entry
+ * -1: failed as a newer microcode was already cached
+ */
+static int save_patch(struct ucode_patch *new_patch)
+{
+ struct ucode_patch *ucode_patch;
+ struct microcode_amd *new_mc = new_patch->data;
+ struct microcode_header_amd *new_header = new_mc->mpb;
+
+ list_for_each_entry(ucode_patch, µcode_cache, list)
+ {
+ struct microcode_amd *old_mc = ucode_patch->data;
+ struct microcode_header_amd *old_header = old_mc->mpb;
+
+ if ( new_header->processor_rev_id == old_header->processor_rev_id )
+ {
+ if ( new_header->patch_id <= old_header->patch_id )
+ return -1;
+ list_replace(&ucode_patch->list, &new_patch->list);
+ free_ucode_patch(ucode_patch);
+ return 0;
+ }
+ }
+ list_add_tail(&new_patch->list, µcode_cache);
+ return 1;
+}
+
+static struct microcode_header_amd *find_patch(unsigned int cpu)
+{
+ struct ucode_patch *ucode_patch;
+
+ list_for_each_entry(ucode_patch, µcode_cache, list)
+ {
+ if ( microcode_fits(ucode_patch->data, cpu) )
+ return ((struct microcode_amd *)ucode_patch->data)->mpb;
+ }
+ return NULL;
+}
+
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;
/* We should bind the task to the CPU */
BUG_ON(raw_smp_processor_id() != cpu);
- if ( mc_amd == NULL )
- return -EINVAL;
-
- hdr = mc_amd->mpb;
+ hdr = find_patch(cpu);
if ( hdr == NULL )
return -EINVAL;
@@ -491,6 +559,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 )
{
+ struct ucode_patch *ucode_patch;
+
+ /*
+ * Save this microcode before checking the signature. It is to
+ * optimize microcode update on a mixed family system. Parsing
+ * microcode file is only done once on one of the CPUs, and
+ * during this process microcode cache is created. Other CPUs
+ * needn't parse the same micrcode file again and again.
+ * Instead, they just load the matched and latest microcode in
+ * the caches.
+ */
+ ucode_patch = alloc_ucode_patch(mc_amd);
+ if ( !IS_ERR_OR_NULL(ucode_patch) && (save_patch(ucode_patch) < 0) )
+ free_ucode_patch(ucode_patch);
+
if ( microcode_fits(mc_amd, cpu) )
{
error = apply_microcode(cpu);
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 8d9a3b2..c4f812f 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -251,6 +251,42 @@ static int microcode_sanity_check(void *mc)
}
/*
+ * save a micrcode to the cache list
+ * return 1: added successfully
+ * 0: replaced an existing entry
+ * -1: failed as a newer microcode was already cached
+ */
+static int save_patch(struct ucode_patch *new_patch)
+{
+ void *mc;
+ struct ucode_patch *ucode_patch;
+
+ ASSERT(new_patch);
+
+ mc = new_patch->data;
+ list_for_each_entry(ucode_patch, µcode_cache, list)
+ {
+ struct microcode_header_intel *saved_header = ucode_patch->data;
+ int ret;
+
+ ret = microcode_update_match(mc, saved_header->sig, saved_header->pf,
+ saved_header->rev);
+ if ( ret == OLD_UCODE )
+ return -1;
+ if ( ret == MIS_UCODE )
+ continue;
+
+ list_replace(&ucode_patch->list, &new_patch->list);
+ xfree(ucode_patch->data);
+ xfree(ucode_patch);
+ return 0;
+ }
+
+ list_add_tail(&new_patch->list, µcode_cache);
+ return 1;
+}
+
+/*
* return 0 - no update found
* return 1 - found update
* return < 0 - error
@@ -261,6 +297,30 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
const struct microcode_header_intel *mc_header = mc;
unsigned long total_size = get_totalsize(mc_header);
void *new_mc;
+ struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch);
+ void *new_mc2 = xmalloc_bytes(total_size);
+
+ /*
+ * Save this microcode before checking the signature. It is to
+ * optimize microcode update on a mixed family system. Parsing
+ * microcode file is only done once on one of the CPUs, and
+ * during this process microcode cache is created. Other CPUs
+ * needn't parse the same micrcode file again and again.
+ * Instead, they just load the matched and latest microcode in
+ * the caches.
+ */
+ if ( !ucode_patch || !new_mc2 )
+ {
+ printk(KERN_ERR "microcode: error! Can not allocate memory\n");
+ return -ENOMEM;
+ }
+ memcpy(new_mc2, mc, total_size);
+ ucode_patch->data = new_mc2;
+ if ( save_patch(ucode_patch) < 0 )
+ {
+ xfree(new_mc2);
+ xfree(ucode_patch);
+ }
if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf,
uci->cpu_sig.rev) != NEW_UCODE )
@@ -282,6 +342,29 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
return 1;
}
+static struct microcode_intel *find_patch(unsigned int cpu)
+{
+ int err;
+ struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ struct ucode_patch *ucode_patch;
+
+ err = collect_cpu_info(cpu, &uci->cpu_sig);
+ if ( unlikely(err) )
+ {
+ memset(uci, 0, sizeof(*uci));
+ return ERR_PTR(err);
+ }
+
+ list_for_each_entry(ucode_patch, µcode_cache, list)
+ {
+ int ret = microcode_update_match(ucode_patch->data, uci->cpu_sig.sig,
+ uci->cpu_sig.pf, uci->cpu_sig.rev);
+ if (ret == NEW_UCODE)
+ return ucode_patch->data;
+ }
+ return NULL;
+}
+
static int apply_microcode(unsigned int cpu)
{
unsigned long flags;
@@ -289,18 +372,20 @@ 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);
+ struct microcode_intel *mc_intel;
/* We should bind the task to the CPU */
BUG_ON(cpu_num != cpu);
- if ( uci->mc.mc_intel == NULL )
+ mc_intel = find_patch(cpu);
+ if ( mc_intel == NULL )
return -EINVAL;
/* 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 */
@@ -311,19 +396,19 @@ 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.date & 0xffff,
- uci->mc.mc_intel->hdr.date >> 24,
- (uci->mc.mc_intel->hdr.date >> 16) & 0xff);
+ mc_intel->hdr.date & 0xffff,
+ mc_intel->hdr.date >> 24,
+ (mc_intel->hdr.date >> 16) & 0xff);
uci->cpu_sig.rev = val[1];
return 0;
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 23ea954..0236425 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -1,6 +1,7 @@
#ifndef ASM_X86__MICROCODE_H
#define ASM_X86__MICROCODE_H
+#include <xen/list.h>
#include <xen/percpu.h>
struct cpu_signature;
@@ -30,7 +31,17 @@ struct ucode_cpu_info {
} mc;
};
+struct ucode_patch {
+ struct list_head list;
+ void *data;
+ uint32_t patch_id;
+ uint16_t equiv_cpu;
+ void * equiv_cpu_table;
+ size_t equiv_cpu_table_size;
+};
+
DECLARE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
extern const struct microcode_ops *microcode_ops;
+extern struct list_head microcode_cache;
#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] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-28 5:34 ` [PATCH v4 2/6] microcode: save all microcodes which pass sanity check Chao Gao
@ 2018-11-28 12:00 ` Roger Pau Monné
2018-11-29 2:40 ` Chao Gao
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 12:00 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
> ... and search caches to find a suitable one when loading.
Why do you need to save all of them? You are only going to load a
single microcode, so I don't understand the need to cache them all.
> With this cache, the existing 'uci->mc' structure is redundent.
> 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>
> ---
> xen/arch/x86/microcode.c | 2 +
> xen/arch/x86/microcode_amd.c | 93 +++++++++++++++++++++++++++++++++++---
> xen/arch/x86/microcode_intel.c | 99 ++++++++++++++++++++++++++++++++++++++---
> xen/include/asm-x86/microcode.h | 11 +++++
> 4 files changed, 193 insertions(+), 12 deletions(-)
>
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 4163f50..4f2db88 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -61,6 +61,8 @@ static struct ucode_mod_blob __initdata ucode_blob;
> */
> static bool_t __initdata ucode_scan;
>
> +LIST_HEAD(microcode_cache);
> +
> void __init microcode_set_module(unsigned int idx)
> {
> ucode_mod_idx = idx;
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index fba44cc..a686a87 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -190,22 +190,90 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd,
> return 1;
> }
>
> +static struct ucode_patch *alloc_ucode_patch(struct microcode_amd *mc_amd)
> +{
> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_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 ( !ucode_patch || !cache || !mpb || !equiv_cpu_table )
> + return ERR_PTR(-ENOMEM);
This can leak memory. For example failing to allocate only
equiv_cpu_table would leak all the other allocations. Ie: you need to
xfree all of them before returning.
> +
> + memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table,
> + mc_amd->equiv_cpu_table_size);
> + memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size);
Don't you need to do:
cache->equiv_cpu_table = equiv_cpu_table;
cache->mpb = mpb;
Before attempting to memcpy to it? Or else you will memcpy to random
locations because the contents of cache are not zeroed.
IMO making such modifications to the AMD code without testing it is
very dangerous. Could you get an AMD system or ask an AMD dev to test
it? I would try with the AMD SVM maintainers.
> + cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size;
> + cache->mpb_size = mc_amd->mpb_size;
> + ucode_patch->data = cache;
Newline.
> + return ucode_patch;
> +}
> +
> +static void free_ucode_patch(struct ucode_patch *ucode_patch)
> +{
> + struct microcode_amd *mc_amd = ucode_patch->data;
> +
> + xfree(mc_amd->equiv_cpu_table);
> + xfree(mc_amd->mpb);
> + xfree(mc_amd);
> + xfree(ucode_patch);
> +}
> +
> +/*
> + * save a micrcode to the cache list
> + * return 1: added successfully
> + * 0: replaced an existing entry
> + * -1: failed as a newer microcode was already cached
Using an enum (like you do in patch #1) would be better and less
cryptic IMO.
> + */
> +static int save_patch(struct ucode_patch *new_patch)
> +{
> + struct ucode_patch *ucode_patch;
> + struct microcode_amd *new_mc = new_patch->data;
> + struct microcode_header_amd *new_header = new_mc->mpb;
> +
> + list_for_each_entry(ucode_patch, µcode_cache, list)
> + {
> + struct microcode_amd *old_mc = ucode_patch->data;
> + struct microcode_header_amd *old_header = old_mc->mpb;
> +
> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
> + {
> + if ( new_header->patch_id <= old_header->patch_id )
> + return -1;
> + list_replace(&ucode_patch->list, &new_patch->list);
> + free_ucode_patch(ucode_patch);
> + return 0;
> + }
> + }
This could be made common code with a specific hook for AMD and Intel
in order to do the comparison, so that at least the loop over the
list of ucode entries could be shared.
> + list_add_tail(&new_patch->list, µcode_cache);
> + return 1;
> +}
> +
> +static struct microcode_header_amd *find_patch(unsigned int cpu)
> +{
> + struct ucode_patch *ucode_patch;
> +
> + list_for_each_entry(ucode_patch, µcode_cache, list)
> + {
> + if ( microcode_fits(ucode_patch->data, cpu) )
> + return ((struct microcode_amd *)ucode_patch->data)->mpb;
> + }
> + return NULL;
This also looks suitable to be moved to common code with dedicated AMD
and Intel hooks.
> +}
> +
> 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;
>
> /* We should bind the task to the CPU */
> BUG_ON(raw_smp_processor_id() != cpu);
>
> - if ( mc_amd == NULL )
> - return -EINVAL;
> -
> - hdr = mc_amd->mpb;
> + hdr = find_patch(cpu);
> if ( hdr == NULL )
> return -EINVAL;
>
> @@ -491,6 +559,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 )
> {
> + struct ucode_patch *ucode_patch;
> +
> + /*
> + * Save this microcode before checking the signature. It is to
> + * optimize microcode update on a mixed family system. Parsing
Er, is it possible to have a system with CPUs of different family?
What's going to happen with CPUs having different features?
> + * microcode file is only done once on one of the CPUs, and
> + * during this process microcode cache is created. Other CPUs
> + * needn't parse the same micrcode file again and again.
> + * Instead, they just load the matched and latest microcode in
> + * the caches.
> + */
> + ucode_patch = alloc_ucode_patch(mc_amd);
> + if ( !IS_ERR_OR_NULL(ucode_patch) && (save_patch(ucode_patch) < 0) )
> + free_ucode_patch(ucode_patch);
> +
> if ( microcode_fits(mc_amd, cpu) )
> {
> error = apply_microcode(cpu);
> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index 8d9a3b2..c4f812f 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -251,6 +251,42 @@ static int microcode_sanity_check(void *mc)
> }
>
> /*
> + * save a micrcode to the cache list
> + * return 1: added successfully
> + * 0: replaced an existing entry
> + * -1: failed as a newer microcode was already cached
> + */
> +static int save_patch(struct ucode_patch *new_patch)
> +{
> + void *mc;
> + struct ucode_patch *ucode_patch;
> +
> + ASSERT(new_patch);
> +
> + mc = new_patch->data;
> + list_for_each_entry(ucode_patch, µcode_cache, list)
> + {
> + struct microcode_header_intel *saved_header = ucode_patch->data;
> + int ret;
> +
> + ret = microcode_update_match(mc, saved_header->sig, saved_header->pf,
> + saved_header->rev);
You can initialize ret at definition time.
> + if ( ret == OLD_UCODE )
> + return -1;
> + if ( ret == MIS_UCODE )
> + continue;
> +
> + list_replace(&ucode_patch->list, &new_patch->list);
> + xfree(ucode_patch->data);
> + xfree(ucode_patch);
> + return 0;
> + }
> +
> + list_add_tail(&new_patch->list, µcode_cache);
> + return 1;
> +}
> +
> +/*
> * return 0 - no update found
> * return 1 - found update
> * return < 0 - error
> @@ -261,6 +297,30 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
> const struct microcode_header_intel *mc_header = mc;
> unsigned long total_size = get_totalsize(mc_header);
> void *new_mc;
> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch);
> + void *new_mc2 = xmalloc_bytes(total_size);
> +
> + /*
> + * Save this microcode before checking the signature. It is to
> + * optimize microcode update on a mixed family system. Parsing
> + * microcode file is only done once on one of the CPUs, and
> + * during this process microcode cache is created. Other CPUs
> + * needn't parse the same micrcode file again and again.
> + * Instead, they just load the matched and latest microcode in
> + * the caches.
> + */
> + if ( !ucode_patch || !new_mc2 )
> + {
> + printk(KERN_ERR "microcode: error! Can not allocate memory\n");
Since this code is not imported from Linux please use XENLOG_ERR. You
also need to xfree both structs in order to avoid leaking memory.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-28 12:00 ` Roger Pau Monné
@ 2018-11-29 2:40 ` Chao Gao
2018-11-29 9:22 ` Roger Pau Monné
2018-11-29 10:19 ` Jan Beulich
0 siblings, 2 replies; 41+ messages in thread
From: Chao Gao @ 2018-11-29 2:40 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>> ... and search caches to find a suitable one when loading.
>
>Why do you need to save all of them? You are only going to load a
>single microcode, so I don't understand the need to cache them all.
>
>> With this cache, the existing 'uci->mc' structure is redundent.
>> 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>
>> ---
>> xen/arch/x86/microcode.c | 2 +
>> xen/arch/x86/microcode_amd.c | 93 +++++++++++++++++++++++++++++++++++---
>> xen/arch/x86/microcode_intel.c | 99 ++++++++++++++++++++++++++++++++++++++---
>> xen/include/asm-x86/microcode.h | 11 +++++
>> 4 files changed, 193 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 4163f50..4f2db88 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -61,6 +61,8 @@ static struct ucode_mod_blob __initdata ucode_blob;
>> */
>> static bool_t __initdata ucode_scan;
>>
>> +LIST_HEAD(microcode_cache);
>> +
>> void __init microcode_set_module(unsigned int idx)
>> {
>> ucode_mod_idx = idx;
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index fba44cc..a686a87 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -190,22 +190,90 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd,
>> return 1;
>> }
>>
>> +static struct ucode_patch *alloc_ucode_patch(struct microcode_amd *mc_amd)
>> +{
>> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_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 ( !ucode_patch || !cache || !mpb || !equiv_cpu_table )
>> + return ERR_PTR(-ENOMEM);
>
>This can leak memory. For example failing to allocate only
>equiv_cpu_table would leak all the other allocations. Ie: you need to
>xfree all of them before returning.
Yes. I fully agree :).
>
>> +
>> + memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table,
>> + mc_amd->equiv_cpu_table_size);
>> + memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size);
>
>Don't you need to do:
>
>cache->equiv_cpu_table = equiv_cpu_table;
>cache->mpb = mpb;
Yes. I should have done this.
>
>Before attempting to memcpy to it? Or else you will memcpy to random
>locations because the contents of cache are not zeroed.
>
>IMO making such modifications to the AMD code without testing it is
>very dangerous. Could you get an AMD system or ask an AMD dev to test
>it? I would try with the AMD SVM maintainers.
It is improbable for me to find an AMD machine in my team. I will copy AMD
SVM maintainers in the coming versions and ask them to help to test this
series.
>
>> + cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size;
>> + cache->mpb_size = mc_amd->mpb_size;
>> + ucode_patch->data = cache;
>
>Newline.
>
>> + return ucode_patch;
>> +}
>> +
>> +static void free_ucode_patch(struct ucode_patch *ucode_patch)
>> +{
>> + struct microcode_amd *mc_amd = ucode_patch->data;
>> +
>> + xfree(mc_amd->equiv_cpu_table);
>> + xfree(mc_amd->mpb);
>> + xfree(mc_amd);
>> + xfree(ucode_patch);
>> +}
>> +
>> +/*
>> + * save a micrcode to the cache list
>> + * return 1: added successfully
>> + * 0: replaced an existing entry
>> + * -1: failed as a newer microcode was already cached
>
>Using an enum (like you do in patch #1) would be better and less
>cryptic IMO.
>
>> + */
>> +static int save_patch(struct ucode_patch *new_patch)
>> +{
>> + struct ucode_patch *ucode_patch;
>> + struct microcode_amd *new_mc = new_patch->data;
>> + struct microcode_header_amd *new_header = new_mc->mpb;
>> +
>> + list_for_each_entry(ucode_patch, µcode_cache, list)
>> + {
>> + struct microcode_amd *old_mc = ucode_patch->data;
>> + struct microcode_header_amd *old_header = old_mc->mpb;
>> +
>> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
>> + {
>> + if ( new_header->patch_id <= old_header->patch_id )
>> + return -1;
>> + list_replace(&ucode_patch->list, &new_patch->list);
>> + free_ucode_patch(ucode_patch);
>> + return 0;
>> + }
>> + }
>
>This could be made common code with a specific hook for AMD and Intel
>in order to do the comparison, so that at least the loop over the
>list of ucode entries could be shared.
Something like pt_pirq_iterate()? Will give it a try.
>
>> + list_add_tail(&new_patch->list, µcode_cache);
>> + return 1;
>> +}
>> +
>> +static struct microcode_header_amd *find_patch(unsigned int cpu)
>> +{
>> + struct ucode_patch *ucode_patch;
>> +
>> + list_for_each_entry(ucode_patch, µcode_cache, list)
>> + {
>> + if ( microcode_fits(ucode_patch->data, cpu) )
>> + return ((struct microcode_amd *)ucode_patch->data)->mpb;
>> + }
>> + return NULL;
>
>This also looks suitable to be moved to common code with dedicated AMD
>and Intel hooks.
>
>> +}
>> +
>> 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;
>>
>> /* We should bind the task to the CPU */
>> BUG_ON(raw_smp_processor_id() != cpu);
>>
>> - if ( mc_amd == NULL )
>> - return -EINVAL;
>> -
>> - hdr = mc_amd->mpb;
>> + hdr = find_patch(cpu);
>> if ( hdr == NULL )
>> return -EINVAL;
>>
>> @@ -491,6 +559,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 )
>> {
>> + struct ucode_patch *ucode_patch;
>> +
>> + /*
>> + * Save this microcode before checking the signature. It is to
>> + * optimize microcode update on a mixed family system. Parsing
>
>Er, is it possible to have a system with CPUs of different family?
>What's going to happen with CPUs having different features?
I have no idea. That each cpu has a per-cpu variable to store the
microcode rather than a global one gives me a feeling that the current
implementation wants to make it work on a system with CPUs of different
family.
>
>> + * microcode file is only done once on one of the CPUs, and
>> + * during this process microcode cache is created. Other CPUs
>> + * needn't parse the same micrcode file again and again.
>> + * Instead, they just load the matched and latest microcode in
>> + * the caches.
>> + */
>> + ucode_patch = alloc_ucode_patch(mc_amd);
>> + if ( !IS_ERR_OR_NULL(ucode_patch) && (save_patch(ucode_patch) < 0) )
>> + free_ucode_patch(ucode_patch);
>> +
>> if ( microcode_fits(mc_amd, cpu) )
>> {
>> error = apply_microcode(cpu);
>> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
>> index 8d9a3b2..c4f812f 100644
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -251,6 +251,42 @@ static int microcode_sanity_check(void *mc)
>> }
>>
>> /*
>> + * save a micrcode to the cache list
>> + * return 1: added successfully
>> + * 0: replaced an existing entry
>> + * -1: failed as a newer microcode was already cached
>> + */
>> +static int save_patch(struct ucode_patch *new_patch)
>> +{
>> + void *mc;
>> + struct ucode_patch *ucode_patch;
>> +
>> + ASSERT(new_patch);
>> +
>> + mc = new_patch->data;
>> + list_for_each_entry(ucode_patch, µcode_cache, list)
>> + {
>> + struct microcode_header_intel *saved_header = ucode_patch->data;
>> + int ret;
>> +
>> + ret = microcode_update_match(mc, saved_header->sig, saved_header->pf,
>> + saved_header->rev);
>
>You can initialize ret at definition time.
>
>> + if ( ret == OLD_UCODE )
>> + return -1;
>> + if ( ret == MIS_UCODE )
>> + continue;
>> +
>> + list_replace(&ucode_patch->list, &new_patch->list);
>> + xfree(ucode_patch->data);
>> + xfree(ucode_patch);
>> + return 0;
>> + }
>> +
>> + list_add_tail(&new_patch->list, µcode_cache);
>> + return 1;
>> +}
>> +
>> +/*
>> * return 0 - no update found
>> * return 1 - found update
>> * return < 0 - error
>> @@ -261,6 +297,30 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
>> const struct microcode_header_intel *mc_header = mc;
>> unsigned long total_size = get_totalsize(mc_header);
>> void *new_mc;
>> + struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch);
>> + void *new_mc2 = xmalloc_bytes(total_size);
>> +
>> + /*
>> + * Save this microcode before checking the signature. It is to
>> + * optimize microcode update on a mixed family system. Parsing
>> + * microcode file is only done once on one of the CPUs, and
>> + * during this process microcode cache is created. Other CPUs
>> + * needn't parse the same micrcode file again and again.
>> + * Instead, they just load the matched and latest microcode in
>> + * the caches.
>> + */
>> + if ( !ucode_patch || !new_mc2 )
>> + {
>> + printk(KERN_ERR "microcode: error! Can not allocate memory\n");
>
>Since this code is not imported from Linux please use XENLOG_ERR. You
>also need to xfree both structs in order to avoid leaking memory.
Will fix this.
Other comments are fine with me 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] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-29 2:40 ` Chao Gao
@ 2018-11-29 9:22 ` Roger Pau Monné
2018-11-30 7:55 ` Chao Gao
2018-12-04 22:39 ` Woods, Brian
2018-11-29 10:19 ` Jan Beulich
1 sibling, 2 replies; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-29 9:22 UTC (permalink / raw)
To: Chao Gao
Cc: Wei Liu, Jan Beulich, Andrew Cooper, Suravee Suthikulpanit,
xen-devel, Boris Ostrovsky, Brian Woods
On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
> >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
> >> ... and search caches to find a suitable one when loading.
> >
> >Why do you need to save all of them? You are only going to load a
> >single microcode, so I don't understand the need to cache them all.
I think the above question needs an answer.
> >IMO making such modifications to the AMD code without testing it is
> >very dangerous. Could you get an AMD system or ask an AMD dev to test
> >it? I would try with the AMD SVM maintainers.
>
> It is improbable for me to find an AMD machine in my team. I will copy AMD
> SVM maintainers in the coming versions and ask them to help to test this
> series.
I'm Cc'ing them now in case they want to provide some feedback.
> >> +static int save_patch(struct ucode_patch *new_patch)
> >> +{
> >> + struct ucode_patch *ucode_patch;
> >> + struct microcode_amd *new_mc = new_patch->data;
> >> + struct microcode_header_amd *new_header = new_mc->mpb;
> >> +
> >> + list_for_each_entry(ucode_patch, µcode_cache, list)
> >> + {
> >> + struct microcode_amd *old_mc = ucode_patch->data;
> >> + struct microcode_header_amd *old_header = old_mc->mpb;
> >> +
> >> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
> >> + {
> >> + if ( new_header->patch_id <= old_header->patch_id )
> >> + return -1;
> >> + list_replace(&ucode_patch->list, &new_patch->list);
> >> + free_ucode_patch(ucode_patch);
> >> + return 0;
> >> + }
> >> + }
> >
> >This could be made common code with a specific hook for AMD and Intel
> >in order to do the comparison, so that at least the loop over the
> >list of ucode entries could be shared.
>
> Something like pt_pirq_iterate()? Will give it a try.
Yes, that might also be helpful. I was thinking of adding such a
comparison hook in microcode_ops, also having something like
pt_pirq_iterate will be helpful if you need to iterate over the cache
in other functions.
> >> @@ -491,6 +559,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 )
> >> {
> >> + struct ucode_patch *ucode_patch;
> >> +
> >> + /*
> >> + * Save this microcode before checking the signature. It is to
> >> + * optimize microcode update on a mixed family system. Parsing
> >
> >Er, is it possible to have a system with CPUs of different family?
> >What's going to happen with CPUs having different features?
>
> I have no idea. That each cpu has a per-cpu variable to store the
> microcode rather than a global one gives me a feeling that the current
> implementation wants to make it work on a system with CPUs of different
> family.
I think we need AMD maintainers input on this one. TBH I very much
doubt there are (working) systems out there with mixed family CPUs.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-29 9:22 ` Roger Pau Monné
@ 2018-11-30 7:55 ` Chao Gao
2018-11-30 9:32 ` Jan Beulich
2018-12-04 22:39 ` Woods, Brian
1 sibling, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-30 7:55 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Wei Liu, Jan Beulich, Andrew Cooper, Suravee Suthikulpanit,
xen-devel, Boris Ostrovsky, Brian Woods
On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
>On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
>> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>> >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>> >> ... and search caches to find a suitable one when loading.
>> >
>> >Why do you need to save all of them? You are only going to load a
>> >single microcode, so I don't understand the need to cache them all.
>
>I think the above question needs an answer.
Out of consideraton for a mixed-family system. Anyway, Since Jan commented
that we gave up support of a mixed-family system, we only need to save
a single microcode for offlined or hot-plugged cpus.
>
>> >IMO making such modifications to the AMD code without testing it is
>> >very dangerous. Could you get an AMD system or ask an AMD dev to test
>> >it? I would try with the AMD SVM maintainers.
>>
>> It is improbable for me to find an AMD machine in my team. I will copy AMD
>> SVM maintainers in the coming versions and ask them to help to test this
>> series.
>
>I'm Cc'ing them now in case they want to provide some feedback.
>
>> >> +static int save_patch(struct ucode_patch *new_patch)
>> >> +{
>> >> + struct ucode_patch *ucode_patch;
>> >> + struct microcode_amd *new_mc = new_patch->data;
>> >> + struct microcode_header_amd *new_header = new_mc->mpb;
>> >> +
>> >> + list_for_each_entry(ucode_patch, µcode_cache, list)
>> >> + {
>> >> + struct microcode_amd *old_mc = ucode_patch->data;
>> >> + struct microcode_header_amd *old_header = old_mc->mpb;
>> >> +
>> >> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
>> >> + {
>> >> + if ( new_header->patch_id <= old_header->patch_id )
>> >> + return -1;
>> >> + list_replace(&ucode_patch->list, &new_patch->list);
>> >> + free_ucode_patch(ucode_patch);
>> >> + return 0;
>> >> + }
>> >> + }
>> >
>> >This could be made common code with a specific hook for AMD and Intel
>> >in order to do the comparison, so that at least the loop over the
>> >list of ucode entries could be shared.
>>
>> Something like pt_pirq_iterate()? Will give it a try.
>
>Yes, that might also be helpful. I was thinking of adding such a
>comparison hook in microcode_ops, also having something like
>pt_pirq_iterate will be helpful if you need to iterate over the cache
>in other functions.
As I am going to remove the microcode cache list, I needn't to iterate
over a list.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-30 7:55 ` Chao Gao
@ 2018-11-30 9:32 ` Jan Beulich
2019-01-15 15:07 ` Andrew Cooper
0 siblings, 1 reply; 41+ messages in thread
From: Jan Beulich @ 2018-11-30 9:32 UTC (permalink / raw)
To: Andrew Cooper, Chao Gao
Cc: Wei Liu, Suravee Suthikulpanit, xen-devel, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
>>> On 30.11.18 at 08:55, <chao.gao@intel.com> wrote:
> On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
>>On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
>>> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>>> >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>>> >> ... and search caches to find a suitable one when loading.
>>> >
>>> >Why do you need to save all of them? You are only going to load a
>>> >single microcode, so I don't understand the need to cache them all.
>>
>>I think the above question needs an answer.
>
> Out of consideraton for a mixed-family system. Anyway, Since Jan commented
> that we gave up support of a mixed-family system, we only need to save
> a single microcode for offlined or hot-plugged cpus.
Well, there might be slightly more needed than just a single instance.
This depends on whether same family/model CPUs with different
stepping and/or different "pf" value can be mixed (and would have
identical feature flags in their CPUID output).
I may have oversimplified the current state of things: Hotplugging
a CPU with more capabilities than the boot CPU is generally fine
afaict. Both you (Intel) and AMD place restrictions on permitted
mixes iirc, so I don't think we need to support anything beyond
such restrictions. Being able to run on permitted mixes which are
not in conflict with our general assumption of there not being any
CPU in the system with less capabilities than the boot CPU would
seem desirable.
Andrew, do you have any view or opinion here?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-30 9:32 ` Jan Beulich
@ 2019-01-15 15:07 ` Andrew Cooper
0 siblings, 0 replies; 41+ messages in thread
From: Andrew Cooper @ 2019-01-15 15:07 UTC (permalink / raw)
To: Jan Beulich, Chao Gao
Cc: Wei Liu, Suravee Suthikulpanit, xen-devel, Boris Ostrovsky,
Brian Woods, Roger Pau Monne
On 30/11/2018 09:32, Jan Beulich wrote:
>>>> On 30.11.18 at 08:55, <chao.gao@intel.com> wrote:
>> On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
>>> On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
>>>> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>>>>> On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>>>>>> ... and search caches to find a suitable one when loading.
>>>>> Why do you need to save all of them? You are only going to load a
>>>>> single microcode, so I don't understand the need to cache them all.
>>> I think the above question needs an answer.
>> Out of consideraton for a mixed-family system. Anyway, Since Jan commented
>> that we gave up support of a mixed-family system, we only need to save
>> a single microcode for offlined or hot-plugged cpus.
> Well, there might be slightly more needed than just a single instance.
> This depends on whether same family/model CPUs with different
> stepping and/or different "pf" value can be mixed (and would have
> identical feature flags in their CPUID output).
>
> I may have oversimplified the current state of things: Hotplugging
> a CPU with more capabilities than the boot CPU is generally fine
> afaict. Both you (Intel) and AMD place restrictions on permitted
> mixes iirc, so I don't think we need to support anything beyond
> such restrictions. Being able to run on permitted mixes which are
> not in conflict with our general assumption of there not being any
> CPU in the system with less capabilities than the boot CPU would
> seem desirable.
>
> Andrew, do you have any view or opinion here?
I'm not aware of anything since the P4 which has tolerated a
heterogeneous setup of physical CPUs.
Our current microcode loading doesn't support more than a single patch
per system, and I expect plenty of other things would break if we did
genuinely have a heterogeneous setup, featurewise.
I'd simplify everything and assume that there is only a single valid up
to date patch, and I doubt we'll see any problems.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-29 9:22 ` Roger Pau Monné
2018-11-30 7:55 ` Chao Gao
@ 2018-12-04 22:39 ` Woods, Brian
2018-12-05 7:38 ` Chao Gao
1 sibling, 1 reply; 41+ messages in thread
From: Woods, Brian @ 2018-12-04 22:39 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Wei Liu, Suthikulpanit, Suravee, Andrew Cooper, Jan Beulich,
xen-devel, Boris Ostrovsky, Woods, Brian, Chao Gao
On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
> On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
> > On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
> > >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
> > >> ... and search caches to find a suitable one when loading.
> > >
> > >Why do you need to save all of them? You are only going to load a
> > >single microcode, so I don't understand the need to cache them all.
>
> I think the above question needs an answer.
>
> > >IMO making such modifications to the AMD code without testing it is
> > >very dangerous. Could you get an AMD system or ask an AMD dev to test
> > >it? I would try with the AMD SVM maintainers.
> >
> > It is improbable for me to find an AMD machine in my team. I will copy AMD
> > SVM maintainers in the coming versions and ask them to help to test this
> > series.
>
> I'm Cc'ing them now in case they want to provide some feedback.
>
> > >> +static int save_patch(struct ucode_patch *new_patch)
> > >> +{
> > >> + struct ucode_patch *ucode_patch;
> > >> + struct microcode_amd *new_mc = new_patch->data;
> > >> + struct microcode_header_amd *new_header = new_mc->mpb;
> > >> +
> > >> + list_for_each_entry(ucode_patch, µcode_cache, list)
> > >> + {
> > >> + struct microcode_amd *old_mc = ucode_patch->data;
> > >> + struct microcode_header_amd *old_header = old_mc->mpb;
> > >> +
> > >> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
> > >> + {
> > >> + if ( new_header->patch_id <= old_header->patch_id )
> > >> + return -1;
> > >> + list_replace(&ucode_patch->list, &new_patch->list);
> > >> + free_ucode_patch(ucode_patch);
> > >> + return 0;
> > >> + }
> > >> + }
> > >
> > >This could be made common code with a specific hook for AMD and Intel
> > >in order to do the comparison, so that at least the loop over the
> > >list of ucode entries could be shared.
> >
> > Something like pt_pirq_iterate()? Will give it a try.
>
> Yes, that might also be helpful. I was thinking of adding such a
> comparison hook in microcode_ops, also having something like
> pt_pirq_iterate will be helpful if you need to iterate over the cache
> in other functions.
>
> > >> @@ -491,6 +559,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 )
> > >> {
> > >> + struct ucode_patch *ucode_patch;
> > >> +
> > >> + /*
> > >> + * Save this microcode before checking the signature. It is to
> > >> + * optimize microcode update on a mixed family system. Parsing
> > >
> > >Er, is it possible to have a system with CPUs of different family?
> > >What's going to happen with CPUs having different features?
> >
> > I have no idea. That each cpu has a per-cpu variable to store the
> > microcode rather than a global one gives me a feeling that the current
> > implementation wants to make it work on a system with CPUs of different
> > family.
>
> I think we need AMD maintainers input on this one. TBH I very much
> doubt there are (working) systems out there with mixed family CPUs.
>
> Thanks, Roger.
Sorry about the delay. From the PPR for F17 M00-0FH
"AMD Family 17h processors with different OPNs or different revisions
cannot be mixed in a multiprocessor system. If an unsupported
configuration is detected, BIOS should configure the BSP as a single
processor system and signal an error."
Even mixing OPNs within a model is a no go for us. Mixing different
families is something I highly doubt will ever be supported.
--
Brian Woods
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-12-04 22:39 ` Woods, Brian
@ 2018-12-05 7:38 ` Chao Gao
0 siblings, 0 replies; 41+ messages in thread
From: Chao Gao @ 2018-12-05 7:38 UTC (permalink / raw)
To: Woods, Brian
Cc: Wei Liu, Suthikulpanit, Suravee, Andrew Cooper, Jan Beulich,
xen-devel, Boris Ostrovsky, Roger Pau Monné
On Tue, Dec 04, 2018 at 10:39:03PM +0000, Woods, Brian wrote:
>On Thu, Nov 29, 2018 at 10:22:10AM +0100, Roger Pau Monné wrote:
>> On Thu, Nov 29, 2018 at 10:40:32AM +0800, Chao Gao wrote:
>> > On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>> > >On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>> > >> ... and search caches to find a suitable one when loading.
>> > >
>> > >Why do you need to save all of them? You are only going to load a
>> > >single microcode, so I don't understand the need to cache them all.
>>
>> I think the above question needs an answer.
>>
>> > >IMO making such modifications to the AMD code without testing it is
>> > >very dangerous. Could you get an AMD system or ask an AMD dev to test
>> > >it? I would try with the AMD SVM maintainers.
>> >
>> > It is improbable for me to find an AMD machine in my team. I will copy AMD
>> > SVM maintainers in the coming versions and ask them to help to test this
>> > series.
>>
>> I'm Cc'ing them now in case they want to provide some feedback.
>>
>> > >> +static int save_patch(struct ucode_patch *new_patch)
>> > >> +{
>> > >> + struct ucode_patch *ucode_patch;
>> > >> + struct microcode_amd *new_mc = new_patch->data;
>> > >> + struct microcode_header_amd *new_header = new_mc->mpb;
>> > >> +
>> > >> + list_for_each_entry(ucode_patch, µcode_cache, list)
>> > >> + {
>> > >> + struct microcode_amd *old_mc = ucode_patch->data;
>> > >> + struct microcode_header_amd *old_header = old_mc->mpb;
>> > >> +
>> > >> + if ( new_header->processor_rev_id == old_header->processor_rev_id )
>> > >> + {
>> > >> + if ( new_header->patch_id <= old_header->patch_id )
>> > >> + return -1;
>> > >> + list_replace(&ucode_patch->list, &new_patch->list);
>> > >> + free_ucode_patch(ucode_patch);
>> > >> + return 0;
>> > >> + }
>> > >> + }
>> > >
>> > >This could be made common code with a specific hook for AMD and Intel
>> > >in order to do the comparison, so that at least the loop over the
>> > >list of ucode entries could be shared.
>> >
>> > Something like pt_pirq_iterate()? Will give it a try.
>>
>> Yes, that might also be helpful. I was thinking of adding such a
>> comparison hook in microcode_ops, also having something like
>> pt_pirq_iterate will be helpful if you need to iterate over the cache
>> in other functions.
>>
>> > >> @@ -491,6 +559,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 )
>> > >> {
>> > >> + struct ucode_patch *ucode_patch;
>> > >> +
>> > >> + /*
>> > >> + * Save this microcode before checking the signature. It is to
>> > >> + * optimize microcode update on a mixed family system. Parsing
>> > >
>> > >Er, is it possible to have a system with CPUs of different family?
>> > >What's going to happen with CPUs having different features?
>> >
>> > I have no idea. That each cpu has a per-cpu variable to store the
>> > microcode rather than a global one gives me a feeling that the current
>> > implementation wants to make it work on a system with CPUs of different
>> > family.
>>
>> I think we need AMD maintainers input on this one. TBH I very much
>> doubt there are (working) systems out there with mixed family CPUs.
>>
>> Thanks, Roger.
>
>Sorry about the delay. From the PPR for F17 M00-0FH
>"AMD Family 17h processors with different OPNs or different revisions
>cannot be mixed in a multiprocessor system. If an unsupported
>configuration is detected, BIOS should configure the BSP as a single
>processor system and signal an error."
>
>Even mixing OPNs within a model is a no go for us. Mixing different
>families is something I highly doubt will ever be supported.
>
Thanks for this information. It is my bad to use the word 'mixed-family'
here without thinking over its rationality or checking SDM carefully.
Will reword the description here.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-29 2:40 ` Chao Gao
2018-11-29 9:22 ` Roger Pau Monné
@ 2018-11-29 10:19 ` Jan Beulich
2019-01-15 15:15 ` Andrew Cooper
1 sibling, 1 reply; 41+ messages in thread
From: Jan Beulich @ 2018-11-29 10:19 UTC (permalink / raw)
To: Chao Gao; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 29.11.18 at 03:40, <chao.gao@intel.com> wrote:
> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>>On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>>> @@ -491,6 +559,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 )
>>> {
>>> + struct ucode_patch *ucode_patch;
>>> +
>>> + /*
>>> + * Save this microcode before checking the signature. It is to
>>> + * optimize microcode update on a mixed family system. Parsing
>>
>>Er, is it possible to have a system with CPUs of different family?
>>What's going to happen with CPUs having different features?
>
> I have no idea. That each cpu has a per-cpu variable to store the
> microcode rather than a global one gives me a feeling that the current
> implementation wants to make it work on a system with CPUs of different
> family.
I think we've long given up on supporting mixed-model or even
mixed-family systems. Therefore in this overhaul doing away with
per-CPU tracking beyond the present ucode level (which indeed
may differ, even if we may have to consider to refuse keeping the
system up in such a case) would perhaps be pretty reasonable.
One thing definitely needs to work: Updating of ucode when
firmware has loaded differing versions (which usually boils down
to firmware neglecting to load ucode on all cores).
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 2/6] microcode: save all microcodes which pass sanity check
2018-11-29 10:19 ` Jan Beulich
@ 2019-01-15 15:15 ` Andrew Cooper
0 siblings, 0 replies; 41+ messages in thread
From: Andrew Cooper @ 2019-01-15 15:15 UTC (permalink / raw)
To: Jan Beulich, Chao Gao; +Cc: xen-devel, Wei Liu, Roger Pau Monne
On 29/11/2018 10:19, Jan Beulich wrote:
>>>> On 29.11.18 at 03:40, <chao.gao@intel.com> wrote:
>> On Wed, Nov 28, 2018 at 01:00:14PM +0100, Roger Pau Monné wrote:
>>> On Wed, Nov 28, 2018 at 01:34:12PM +0800, Chao Gao wrote:
>>>> @@ -491,6 +559,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 )
>>>> {
>>>> + struct ucode_patch *ucode_patch;
>>>> +
>>>> + /*
>>>> + * Save this microcode before checking the signature. It is to
>>>> + * optimize microcode update on a mixed family system. Parsing
>>> Er, is it possible to have a system with CPUs of different family?
>>> What's going to happen with CPUs having different features?
>> I have no idea. That each cpu has a per-cpu variable to store the
>> microcode rather than a global one gives me a feeling that the current
>> implementation wants to make it work on a system with CPUs of different
>> family.
> I think we've long given up on supporting mixed-model or even
> mixed-family systems. Therefore in this overhaul doing away with
> per-CPU tracking beyond the present ucode level (which indeed
> may differ, even if we may have to consider to refuse keeping the
> system up in such a case) would perhaps be pretty reasonable.
>
> One thing definitely needs to work: Updating of ucode when
> firmware has loaded differing versions (which usually boils down
> to firmware neglecting to load ucode on all cores).
Right. It doesn't matter what ucode is installed in the cores currently.
We will have one single piece of early ucode provided in the
initrd/other which needs to be cached and optionally loaded on every CPU
bringup.
During late load load, if the proposed ucode is correct for the system
and newer than the cached version, it should be replace the exiting
cached version, and then loaded onto all CPUs.
The one case that we cannot deal cleanly with is if firmware has a newer
ucode than the initrd, and has only updated some cores, at which point
there is nothing Xen can do to cause every core to have the same
version. We can probably get away with spitting out a warning, and
trying to continue booting, because it is just possible the system will
be stable.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 3/6] microcode: delete 'mc' field from struct ucode_cpu_info
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
2018-11-28 5:34 ` [PATCH v4 1/6] microcode/intel: extend microcode_update_match() Chao Gao
2018-11-28 5:34 ` [PATCH v4 2/6] microcode: save all microcodes which pass sanity check Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 12:32 ` Roger Pau Monné
2018-11-28 5:34 ` [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode() Chao Gao
` (2 subsequent siblings)
5 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Chao Gao
apply_microcode() now uses the cached microcode rather than
the microcode stored in "mc" field of ucode_cpu_info. Also remove
'microcode_resume_match' from microcode_ops because the check is
done in find_patch() in apply_microcode() callback.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
xen/arch/x86/microcode.c | 33 +-----------------
xen/arch/x86/microcode_amd.c | 75 ++---------------------------------------
xen/arch/x86/microcode_intel.c | 27 +++------------
xen/include/asm-x86/microcode.h | 6 ----
4 files changed, 8 insertions(+), 133 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 4f2db88..8350d22 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -199,7 +199,6 @@ 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));
}
@@ -214,8 +213,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;
@@ -230,35 +227,7 @@ int microcode_resume_cpu(unsigned int cpu)
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);
+ 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 a686a87..6e6598a 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -459,10 +459,10 @@ 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;
+ int error = 0;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
@@ -545,10 +545,6 @@ 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
@@ -612,26 +608,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
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 )
- {
- xfree(mc_amd);
- uci->mc.mc_amd = mc_old;
- }
- else
- xfree(mc_old);
-
out:
#if CONFIG_HVM
svm_host_osvw_init();
@@ -646,52 +622,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) )
- 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
@@ -711,7 +641,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 c4f812f..1857332 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -296,9 +296,8 @@ 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;
struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch);
- void *new_mc2 = xmalloc_bytes(total_size);
+ void *new_mc = xmalloc_bytes(total_size);
/*
* Save this microcode before checking the signature. It is to
@@ -309,16 +308,16 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
* Instead, they just load the matched and latest microcode in
* the caches.
*/
- if ( !ucode_patch || !new_mc2 )
+ if ( !ucode_patch || !new_mc )
{
printk(KERN_ERR "microcode: error! Can not allocate memory\n");
return -ENOMEM;
}
- memcpy(new_mc2, mc, total_size);
- ucode_patch->data = new_mc2;
+ memcpy(new_mc, mc, total_size);
+ ucode_patch->data = new_mc;
if ( save_patch(ucode_patch) < 0 )
{
- xfree(new_mc2);
+ xfree(new_mc);
xfree(ucode_patch);
}
@@ -329,16 +328,6 @@ 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;
- }
-
- memcpy(new_mc, mc, total_size);
- xfree(uci->mc.mc_intel);
- uci->mc.mc_intel = new_mc;
return 1;
}
@@ -483,13 +472,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 0236425..e06401b 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -8,7 +8,6 @@ struct cpu_signature;
struct ucode_cpu_info;
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);
@@ -24,11 +23,6 @@ struct cpu_signature {
struct ucode_cpu_info {
struct cpu_signature cpu_sig;
- union {
- struct microcode_intel *mc_intel;
- struct microcode_amd *mc_amd;
- void *mc_valid;
- } mc;
};
struct ucode_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] 41+ messages in thread* Re: [PATCH v4 3/6] microcode: delete 'mc' field from struct ucode_cpu_info
2018-11-28 5:34 ` [PATCH v4 3/6] microcode: delete 'mc' field from struct ucode_cpu_info Chao Gao
@ 2018-11-28 12:32 ` Roger Pau Monné
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 12:32 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:34:13PM +0800, Chao Gao wrote:
> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index c4f812f..1857332 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -296,9 +296,8 @@ 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;
> struct ucode_patch *ucode_patch = xmalloc(struct ucode_patch);
> - void *new_mc2 = xmalloc_bytes(total_size);
> + void *new_mc = xmalloc_bytes(total_size);
>
> /*
> * Save this microcode before checking the signature. It is to
> @@ -309,16 +308,16 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
> * Instead, they just load the matched and latest microcode in
> * the caches.
> */
> - if ( !ucode_patch || !new_mc2 )
> + if ( !ucode_patch || !new_mc )
> {
> printk(KERN_ERR "microcode: error! Can not allocate memory\n");
> return -ENOMEM;
I know this is exiting code, but it's leaking memory...
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
` (2 preceding siblings ...)
2018-11-28 5:34 ` [PATCH v4 3/6] microcode: delete 'mc' field from struct ucode_cpu_info Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 15:02 ` Roger Pau Monné
2018-11-28 5:34 ` [PATCH v4 5/6] microcode: delete microcode pointer and size from microcode_info Chao Gao
2018-11-28 5:34 ` [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading Chao Gao
5 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Chao Gao
cpu_request_microcode() will only parse microcode file and save
suitable microcodes to microcode_cache. To update microcode,
apply_microcode() should be invoked explicitly.
On AMD side, svm_host_osvw_init() is supposed to be called after
microcode update. As apply_micrcode() won't be called by
cpu_request_microcode() now, svm_host_osvw_init() is also moved to the
end of apply_microcode().
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
xen/arch/x86/microcode.c | 58 ++++++++++++++++++++++++++++++------------
xen/arch/x86/microcode_amd.c | 15 +++++------
xen/arch/x86/microcode_intel.c | 5 +---
3 files changed, 50 insertions(+), 28 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 8350d22..cca7b2c 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
return err;
}
-static int microcode_update_cpu(const void *buf, size_t size)
+static int microcode_update_cpu(void)
{
int err;
- unsigned int cpu = smp_processor_id();
- struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
spin_lock(µcode_mutex);
-
- err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
- if ( likely(!err) )
- err = microcode_ops->cpu_request_microcode(cpu, buf, size);
- else
- __microcode_fini_cpu(cpu);
-
+ err = microcode_ops->apply_microcode(smp_processor_id());
spin_unlock(µcode_mutex);
return err;
@@ -259,7 +251,7 @@ static long do_microcode_update(void *_info)
BUG_ON(info->cpu != smp_processor_id());
- error = microcode_update_cpu(info->buffer, info->buffer_size);
+ error = microcode_update_cpu();
if ( error )
info->error = error;
@@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
int ret;
struct microcode_info *info;
+ unsigned int cpu = smp_processor_id();
+ struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
if ( len != (uint32_t)len )
return -E2BIG;
@@ -294,10 +288,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return ret;
}
- info->buffer_size = len;
- info->error = 0;
- info->cpu = cpumask_first(&cpu_online_map);
-
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
@@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
}
}
+ spin_lock(µcode_mutex);
+
+ ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ if ( likely(!ret) )
+ ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
+ else
+ __microcode_fini_cpu(cpu);
+
+ spin_unlock(µcode_mutex);
+
+ if ( ret <= 0 )
+ {
+ printk("No valid or newer microcode found. Update abort!\n");
+ return -EINVAL;
+ }
+
+ info->buffer_size = len;
+ info->error = 0;
+ info->cpu = cpumask_first(&cpu_online_map);
+
return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
}
@@ -370,13 +380,29 @@ int __init early_microcode_update_cpu(bool start_update)
}
if ( data )
{
+ unsigned int cpu = smp_processor_id();
+ struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+
if ( start_update && microcode_ops->start_update )
rc = microcode_ops->start_update();
if ( rc )
return rc;
- return microcode_update_cpu(data, len);
+ spin_lock(µcode_mutex);
+
+ rc = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+ if ( likely(!rc) )
+ rc = microcode_ops->cpu_request_microcode(cpu, data, len);
+ else
+ __microcode_fini_cpu(cpu);
+
+ spin_unlock(µcode_mutex);
+
+ if ( rc <= 0 )
+ return -EINVAL;
+
+ return microcode_update_cpu();
}
else
return -ENOMEM;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 6e6598a..6d860f3 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -299,6 +299,10 @@ static int apply_microcode(unsigned int cpu)
uci->cpu_sig.rev = rev;
+#if CONFIG_HVM
+ svm_host_osvw_init();
+#endif
+
return 0;
}
@@ -466,6 +470,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
unsigned int current_cpu_id;
unsigned int equiv_cpu_id;
+ unsigned int matched_cnt = 0;
/* We should bind the task to the CPU */
BUG_ON(cpu != raw_smp_processor_id());
@@ -572,9 +577,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
if ( microcode_fits(mc_amd, cpu) )
{
- error = apply_microcode(cpu);
- if ( error )
- break;
+ matched_cnt++;
applied_offset = last_offset;
}
@@ -609,17 +612,13 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
}
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
* successfully but subsequent container file processing encounters a
* failure.
*/
- return error;
+ return !error ? matched_cnt : error;
}
static int start_update(void)
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 1857332..a529623 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -466,10 +466,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
if ( offset < 0 )
error = offset;
- if ( !error && matching_count )
- error = apply_microcode(cpu);
-
- return error;
+ return !error ? matching_count : error;
}
static const struct microcode_ops microcode_intel_ops = {
--
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] 41+ messages in thread* Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-28 5:34 ` [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode() Chao Gao
@ 2018-11-28 15:02 ` Roger Pau Monné
2018-11-29 4:28 ` Chao Gao
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 15:02 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:34:14PM +0800, Chao Gao wrote:
> cpu_request_microcode() will only parse microcode file and save
> suitable microcodes to microcode_cache. To update microcode,
> apply_microcode() should be invoked explicitly.
>
> On AMD side, svm_host_osvw_init() is supposed to be called after
> microcode update. As apply_micrcode() won't be called by
> cpu_request_microcode() now, svm_host_osvw_init() is also moved to the
> end of apply_microcode().
I don't understand the previous usage of cpu_request_microcode, was it
used to update the microcode? The name seems to suggest it's used to
get a microcode version without applying anything to the CPU.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> xen/arch/x86/microcode.c | 58 ++++++++++++++++++++++++++++++------------
> xen/arch/x86/microcode_amd.c | 15 +++++------
> xen/arch/x86/microcode_intel.c | 5 +---
> 3 files changed, 50 insertions(+), 28 deletions(-)
>
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 8350d22..cca7b2c 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
> return err;
> }
>
> -static int microcode_update_cpu(const void *buf, size_t size)
> +static int microcode_update_cpu(void)
> {
> int err;
> - unsigned int cpu = smp_processor_id();
> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>
> spin_lock(µcode_mutex);
> -
> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> - if ( likely(!err) )
> - err = microcode_ops->cpu_request_microcode(cpu, buf, size);
> - else
> - __microcode_fini_cpu(cpu);
> -
> + err = microcode_ops->apply_microcode(smp_processor_id());
> spin_unlock(µcode_mutex);
>
> return err;
> @@ -259,7 +251,7 @@ static long do_microcode_update(void *_info)
>
> BUG_ON(info->cpu != smp_processor_id());
>
> - error = microcode_update_cpu(info->buffer, info->buffer_size);
> + error = microcode_update_cpu();
Why don't you just set info->error = microcode_update_cpu()?
AFAICT this is done to attempt to update the remaining CPUs if one
update failed?
Is there anyway to rollback to the previous state so all CPUs have the
same microcode? I assume nothing good will come out of running a
system with CPUs using different microcode versions, but maybe that's
not so bad?
> if ( error )
> info->error = error;
>
> @@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> {
> int ret;
> struct microcode_info *info;
> + unsigned int cpu = smp_processor_id();
> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>
> if ( len != (uint32_t)len )
> return -E2BIG;
> @@ -294,10 +288,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> return ret;
> }
>
> - info->buffer_size = len;
> - info->error = 0;
> - info->cpu = cpumask_first(&cpu_online_map);
> -
> if ( microcode_ops->start_update )
> {
> ret = microcode_ops->start_update();
> @@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> }
> }
>
> + spin_lock(µcode_mutex);
> +
> + ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> + if ( likely(!ret) )
> + ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
> + else
> + __microcode_fini_cpu(cpu);
> +
> + spin_unlock(µcode_mutex);
Why do you need to hold the lock here?
microcode_update is already serialized and should only be executed on
a CPU at a time due to the usage of chained
continue_hypercall_on_cpu.
> +
> + if ( ret <= 0 )
> + {
> + printk("No valid or newer microcode found. Update abort!\n");
> + return -EINVAL;
> + }
> +
> + info->buffer_size = len;
> + info->error = 0;
> + info->cpu = cpumask_first(&cpu_online_map);
> +
> return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> }
>
> @@ -370,13 +380,29 @@ int __init early_microcode_update_cpu(bool start_update)
> }
> if ( data )
> {
> + unsigned int cpu = smp_processor_id();
> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> +
> if ( start_update && microcode_ops->start_update )
> rc = microcode_ops->start_update();
>
> if ( rc )
> return rc;
>
> - return microcode_update_cpu(data, len);
> + spin_lock(µcode_mutex);
> +
> + rc = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> + if ( likely(!rc) )
> + rc = microcode_ops->cpu_request_microcode(cpu, data, len);
> + else
> + __microcode_fini_cpu(cpu);
> +
> + spin_unlock(µcode_mutex);
> +
> + if ( rc <= 0 )
> + return -EINVAL;
> +
> + return microcode_update_cpu();
> }
> else
> return -ENOMEM;
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index 6e6598a..6d860f3 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -299,6 +299,10 @@ static int apply_microcode(unsigned int cpu)
>
> uci->cpu_sig.rev = rev;
>
> +#if CONFIG_HVM
> + svm_host_osvw_init();
> +#endif
> +
> return 0;
> }
>
> @@ -466,6 +470,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> unsigned int current_cpu_id;
> unsigned int equiv_cpu_id;
> + unsigned int matched_cnt = 0;
>
> /* We should bind the task to the CPU */
> BUG_ON(cpu != raw_smp_processor_id());
> @@ -572,9 +577,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>
> if ( microcode_fits(mc_amd, cpu) )
> {
> - error = apply_microcode(cpu);
> - if ( error )
> - break;
> + matched_cnt++;
> applied_offset = last_offset;
> }
>
> @@ -609,17 +612,13 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
> }
>
> 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
> * successfully but subsequent container file processing encounters a
> * failure.
> */
> - return error;
> + return !error ? matched_cnt : error;
You can use error ?: matched_cnt; which is shorter.
> }
>
> static int start_update(void)
> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index 1857332..a529623 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -466,10 +466,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
> if ( offset < 0 )
> error = offset;
>
> - if ( !error && matching_count )
> - error = apply_microcode(cpu);
> -
> - return error;
> + return !error ? matching_count : error;
Same here.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-28 15:02 ` Roger Pau Monné
@ 2018-11-29 4:28 ` Chao Gao
2018-11-29 9:46 ` Roger Pau Monné
0 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-29 4:28 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 04:02:25PM +0100, Roger Pau Monné wrote:
>On Wed, Nov 28, 2018 at 01:34:14PM +0800, Chao Gao wrote:
>> cpu_request_microcode() will only parse microcode file and save
>> suitable microcodes to microcode_cache. To update microcode,
>> apply_microcode() should be invoked explicitly.
>>
>> On AMD side, svm_host_osvw_init() is supposed to be called after
>> microcode update. As apply_micrcode() won't be called by
>> cpu_request_microcode() now, svm_host_osvw_init() is also moved to the
>> end of apply_microcode().
>
>I don't understand the previous usage of cpu_request_microcode, was it
>used to update the microcode?
Yes. I am moving microcode loading out of cpu_request_microcode hook.
>The name seems to suggest it's used to
>get a microcode version without applying anything to the CPU.
>
>>
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>> xen/arch/x86/microcode.c | 58 ++++++++++++++++++++++++++++++------------
>> xen/arch/x86/microcode_amd.c | 15 +++++------
>> xen/arch/x86/microcode_intel.c | 5 +---
>> 3 files changed, 50 insertions(+), 28 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 8350d22..cca7b2c 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
>> return err;
>> }
>>
>> -static int microcode_update_cpu(const void *buf, size_t size)
>> +static int microcode_update_cpu(void)
>> {
>> int err;
>> - unsigned int cpu = smp_processor_id();
>> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>
>> spin_lock(µcode_mutex);
>> -
>> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> - if ( likely(!err) )
>> - err = microcode_ops->cpu_request_microcode(cpu, buf, size);
>> - else
>> - __microcode_fini_cpu(cpu);
>> -
>> + err = microcode_ops->apply_microcode(smp_processor_id());
>> spin_unlock(µcode_mutex);
>>
>> return err;
>> @@ -259,7 +251,7 @@ static long do_microcode_update(void *_info)
>>
>> BUG_ON(info->cpu != smp_processor_id());
>>
>> - error = microcode_update_cpu(info->buffer, info->buffer_size);
>> + error = microcode_update_cpu();
>
>Why don't you just set info->error = microcode_update_cpu()?
>
>AFAICT this is done to attempt to update the remaining CPUs if one
>update failed?
Yes. But this patch doesn't change the logic here. Actually, if HT is
enabled and microcode is shared between the logical threads of the same
core, so if one thread updates microcode successfully, its sibling would
always fail in current logic. I am trying to explain why we cannot abort
the update even though an error is met in current logic. It definitely
can be solved by tweaking the logic slightly.
>
>Is there anyway to rollback to the previous state so all CPUs have the
>same microcode?
Seems it is not allowed to load a microcode with numeratically smaller
revision according to 9.11.7.2.
With patch 6, a panic() would be triggered if some cpus failed to do the
update. I didn't try to change the logic here.
>I assume nothing good will come out of running a
>system with CPUs using different microcode versions, but maybe that's
>not so bad?
It is better that all CPUs have the same microcode revision.
Linux kernel rejects late microcode update if finding some CPUs
offlined. I may port this patch to Xen too in a separate patch.
>
>> if ( error )
>> info->error = error;
>>
>> @@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> {
>> int ret;
>> struct microcode_info *info;
>> + unsigned int cpu = smp_processor_id();
>> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>
>> if ( len != (uint32_t)len )
>> return -E2BIG;
>> @@ -294,10 +288,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> return ret;
>> }
>>
>> - info->buffer_size = len;
>> - info->error = 0;
>> - info->cpu = cpumask_first(&cpu_online_map);
>> -
>> if ( microcode_ops->start_update )
>> {
>> ret = microcode_ops->start_update();
>> @@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> }
>> }
>>
>> + spin_lock(µcode_mutex);
>> +
>> + ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> + if ( likely(!ret) )
>> + ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
>> + else
>> + __microcode_fini_cpu(cpu);
>> +
>> + spin_unlock(µcode_mutex);
>
>Why do you need to hold the lock here?
>
>microcode_update is already serialized and should only be executed on
>a CPU at a time due to the usage of chained
>continue_hypercall_on_cpu.
microcode_resume_cpu() also uses the 'uci' and the global microcode cache.
This lock is to prevent them happening simultaneously (someone is
adding/replacing entries to a list and another is reading the list).
All existing call sites of collec_cpu_info() and cpu_request_microcode()
are protected with this lock.
>
>> +
>> + if ( ret <= 0 )
>> + {
>> + printk("No valid or newer microcode found. Update abort!\n");
>> + return -EINVAL;
>> + }
>> +
>> + info->buffer_size = len;
>> + info->error = 0;
>> + info->cpu = cpumask_first(&cpu_online_map);
>> +
>> return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>> }
>>
>> @@ -370,13 +380,29 @@ int __init early_microcode_update_cpu(bool start_update)
>> }
>> if ( data )
>> {
>> + unsigned int cpu = smp_processor_id();
>> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> +
>> if ( start_update && microcode_ops->start_update )
>> rc = microcode_ops->start_update();
>>
>> if ( rc )
>> return rc;
>>
>> - return microcode_update_cpu(data, len);
>> + spin_lock(µcode_mutex);
>> +
>> + rc = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> + if ( likely(!rc) )
>> + rc = microcode_ops->cpu_request_microcode(cpu, data, len);
>> + else
>> + __microcode_fini_cpu(cpu);
>> +
>> + spin_unlock(µcode_mutex);
>> +
>> + if ( rc <= 0 )
>> + return -EINVAL;
>> +
>> + return microcode_update_cpu();
>> }
>> else
>> return -ENOMEM;
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index 6e6598a..6d860f3 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -299,6 +299,10 @@ static int apply_microcode(unsigned int cpu)
>>
>> uci->cpu_sig.rev = rev;
>>
>> +#if CONFIG_HVM
>> + svm_host_osvw_init();
>> +#endif
>> +
>> return 0;
>> }
>>
>> @@ -466,6 +470,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> unsigned int current_cpu_id;
>> unsigned int equiv_cpu_id;
>> + unsigned int matched_cnt = 0;
>>
>> /* We should bind the task to the CPU */
>> BUG_ON(cpu != raw_smp_processor_id());
>> @@ -572,9 +577,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>>
>> if ( microcode_fits(mc_amd, cpu) )
>> {
>> - error = apply_microcode(cpu);
>> - if ( error )
>> - break;
>> + matched_cnt++;
>> applied_offset = last_offset;
>> }
>>
>> @@ -609,17 +612,13 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>> }
>>
>> 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
>> * successfully but subsequent container file processing encounters a
>> * failure.
>> */
>> - return error;
>> + return !error ? matched_cnt : error;
>
>You can use error ?: matched_cnt; which is shorter.
I was told that It would be good to put the likely case in the first place.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-29 4:28 ` Chao Gao
@ 2018-11-29 9:46 ` Roger Pau Monné
2018-11-30 8:57 ` Chao Gao
0 siblings, 1 reply; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-29 9:46 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Thu, Nov 29, 2018 at 12:28:46PM +0800, Chao Gao wrote:
> On Wed, Nov 28, 2018 at 04:02:25PM +0100, Roger Pau Monné wrote:
> >On Wed, Nov 28, 2018 at 01:34:14PM +0800, Chao Gao wrote:
> >> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> >> index 8350d22..cca7b2c 100644
> >> --- a/xen/arch/x86/microcode.c
> >> +++ b/xen/arch/x86/microcode.c
> >> @@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
> >> return err;
> >> }
> >>
> >> -static int microcode_update_cpu(const void *buf, size_t size)
> >> +static int microcode_update_cpu(void)
> >> {
> >> int err;
> >> - unsigned int cpu = smp_processor_id();
> >> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> >>
> >> spin_lock(µcode_mutex);
> >> -
> >> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> >> - if ( likely(!err) )
> >> - err = microcode_ops->cpu_request_microcode(cpu, buf, size);
> >> - else
> >> - __microcode_fini_cpu(cpu);
> >> -
> >> + err = microcode_ops->apply_microcode(smp_processor_id());
> >> spin_unlock(µcode_mutex);
> >>
> >> return err;
> >> @@ -259,7 +251,7 @@ static long do_microcode_update(void *_info)
> >>
> >> BUG_ON(info->cpu != smp_processor_id());
> >>
> >> - error = microcode_update_cpu(info->buffer, info->buffer_size);
> >> + error = microcode_update_cpu();
> >
> >Why don't you just set info->error = microcode_update_cpu()?
> >
> >AFAICT this is done to attempt to update the remaining CPUs if one
> >update failed?
>
> Yes. But this patch doesn't change the logic here. Actually, if HT is
> enabled and microcode is shared between the logical threads of the same
> core, so if one thread updates microcode successfully, its sibling would
> always fail in current logic. I am trying to explain why we cannot abort
> the update even though an error is met in current logic. It definitely
> can be solved by tweaking the logic slightly.
>
> >
> >Is there anyway to rollback to the previous state so all CPUs have the
> >same microcode?
>
> Seems it is not allowed to load a microcode with numeratically smaller
> revision according to 9.11.7.2.
>
> With patch 6, a panic() would be triggered if some cpus failed to do the
> update. I didn't try to change the logic here.
>
> >I assume nothing good will come out of running a
> >system with CPUs using different microcode versions, but maybe that's
> >not so bad?
>
> It is better that all CPUs have the same microcode revision.
>
> Linux kernel rejects late microcode update if finding some CPUs
> offlined. I may port this patch to Xen too in a separate patch.
What happens with hotplug CPUs?
Even if you disable late loading when there are offlined CPUs you
still need to handle hotplug CPUs, which IMO should share the same
path with offlined CPUs: the microcode update should be done ASAP
after bringing the CPU up.
> >
> >> if ( error )
> >> info->error = error;
> >>
> >> @@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> >> {
> >> int ret;
> >> struct microcode_info *info;
> >> + unsigned int cpu = smp_processor_id();
> >> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> >>
> >> if ( len != (uint32_t)len )
> >> return -E2BIG;
> >> @@ -294,10 +288,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> >> return ret;
> >> }
> >>
> >> - info->buffer_size = len;
> >> - info->error = 0;
> >> - info->cpu = cpumask_first(&cpu_online_map);
> >> -
> >> if ( microcode_ops->start_update )
> >> {
> >> ret = microcode_ops->start_update();
> >> @@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> >> }
> >> }
> >>
> >> + spin_lock(µcode_mutex);
> >> +
> >> + ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> >> + if ( likely(!ret) )
> >> + ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
> >> + else
> >> + __microcode_fini_cpu(cpu);
> >> +
> >> + spin_unlock(µcode_mutex);
> >
> >Why do you need to hold the lock here?
> >
> >microcode_update is already serialized and should only be executed on
> >a CPU at a time due to the usage of chained
> >continue_hypercall_on_cpu.
>
> microcode_resume_cpu() also uses the 'uci' and the global microcode cache.
> This lock is to prevent them happening simultaneously (someone is
> adding/replacing entries to a list and another is reading the list).
> All existing call sites of collec_cpu_info() and cpu_request_microcode()
> are protected with this lock.
I mean, there should be some kind of protection to prevent the list
from changing at all while there's an update in progress, or else you
risk using different versions for different CPUs if there's a list
addition while a microcode update is in progress?
The current lock will just prevent list corruption by serializing
users, but it's not going to prevent changes while there's a microcode
update in progress.
> >> @@ -609,17 +612,13 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
> >> }
> >>
> >> 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
> >> * successfully but subsequent container file processing encounters a
> >> * failure.
> >> */
> >> - return error;
> >> + return !error ? matched_cnt : error;
> >
> >You can use error ?: matched_cnt; which is shorter.
>
> I was told that It would be good to put the likely case in the first place.
This is not a hot path anyway, but if you prefer the current code I'm
fine with it.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-29 9:46 ` Roger Pau Monné
@ 2018-11-30 8:57 ` Chao Gao
2018-11-30 9:38 ` Jan Beulich
0 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-30 8:57 UTC (permalink / raw)
To: Roger Pau Monné; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Thu, Nov 29, 2018 at 10:46:05AM +0100, Roger Pau Monné wrote:
>On Thu, Nov 29, 2018 at 12:28:46PM +0800, Chao Gao wrote:
>> On Wed, Nov 28, 2018 at 04:02:25PM +0100, Roger Pau Monné wrote:
>> >On Wed, Nov 28, 2018 at 01:34:14PM +0800, Chao Gao wrote:
>> >> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> >> index 8350d22..cca7b2c 100644
>> >> --- a/xen/arch/x86/microcode.c
>> >> +++ b/xen/arch/x86/microcode.c
>> >> @@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
>> >> return err;
>> >> }
>> >>
>> >> -static int microcode_update_cpu(const void *buf, size_t size)
>> >> +static int microcode_update_cpu(void)
>> >> {
>> >> int err;
>> >> - unsigned int cpu = smp_processor_id();
>> >> - struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> >>
>> >> spin_lock(µcode_mutex);
>> >> -
>> >> - err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> >> - if ( likely(!err) )
>> >> - err = microcode_ops->cpu_request_microcode(cpu, buf, size);
>> >> - else
>> >> - __microcode_fini_cpu(cpu);
>> >> -
>> >> + err = microcode_ops->apply_microcode(smp_processor_id());
>> >> spin_unlock(µcode_mutex);
>> >>
>> >> return err;
>> >> @@ -259,7 +251,7 @@ static long do_microcode_update(void *_info)
>> >>
>> >> BUG_ON(info->cpu != smp_processor_id());
>> >>
>> >> - error = microcode_update_cpu(info->buffer, info->buffer_size);
>> >> + error = microcode_update_cpu();
>> >
>> >Why don't you just set info->error = microcode_update_cpu()?
>> >
>> >AFAICT this is done to attempt to update the remaining CPUs if one
>> >update failed?
>>
>> Yes. But this patch doesn't change the logic here. Actually, if HT is
>> enabled and microcode is shared between the logical threads of the same
>> core, so if one thread updates microcode successfully, its sibling would
>> always fail in current logic. I am trying to explain why we cannot abort
>> the update even though an error is met in current logic. It definitely
>> can be solved by tweaking the logic slightly.
>>
>> >
>> >Is there anyway to rollback to the previous state so all CPUs have the
>> >same microcode?
>>
>> Seems it is not allowed to load a microcode with numeratically smaller
>> revision according to 9.11.7.2.
>>
>> With patch 6, a panic() would be triggered if some cpus failed to do the
>> update. I didn't try to change the logic here.
>>
>> >I assume nothing good will come out of running a
>> >system with CPUs using different microcode versions, but maybe that's
>> >not so bad?
>>
>> It is better that all CPUs have the same microcode revision.
>>
>> Linux kernel rejects late microcode update if finding some CPUs
>> offlined. I may port this patch to Xen too in a separate patch.
>
>What happens with hotplug CPUs?
>
>Even if you disable late loading when there are offlined CPUs you
>still need to handle hotplug CPUs, which IMO should share the same
>path with offlined CPUs: the microcode update should be done ASAP
>after bringing the CPU up.
>
In linux, CPU's being offline is just a logical offline. It may participate
actions like MCE. It would lead to a situation that some cpus are using old
microcode while some are using the new one, which introduces instability.
CPU hotplug doesn't have such issue.
>> >
>> >> if ( error )
>> >> info->error = error;
>> >>
>> >> @@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> >> {
>> >> int ret;
>> >> struct microcode_info *info;
>> >> + unsigned int cpu = smp_processor_id();
>> >> + struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> >>
>> >> if ( len != (uint32_t)len )
>> >> return -E2BIG;
>> >> @@ -294,10 +288,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> >> return ret;
>> >> }
>> >>
>> >> - info->buffer_size = len;
>> >> - info->error = 0;
>> >> - info->cpu = cpumask_first(&cpu_online_map);
>> >> -
>> >> if ( microcode_ops->start_update )
>> >> {
>> >> ret = microcode_ops->start_update();
>> >> @@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> >> }
>> >> }
>> >>
>> >> + spin_lock(µcode_mutex);
>> >> +
>> >> + ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> >> + if ( likely(!ret) )
>> >> + ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
>> >> + else
>> >> + __microcode_fini_cpu(cpu);
>> >> +
>> >> + spin_unlock(µcode_mutex);
>> >
>> >Why do you need to hold the lock here?
>> >
>> >microcode_update is already serialized and should only be executed on
>> >a CPU at a time due to the usage of chained
>> >continue_hypercall_on_cpu.
>>
>> microcode_resume_cpu() also uses the 'uci' and the global microcode cache.
>> This lock is to prevent them happening simultaneously (someone is
>> adding/replacing entries to a list and another is reading the list).
>> All existing call sites of collec_cpu_info() and cpu_request_microcode()
>> are protected with this lock.
>
>I mean, there should be some kind of protection to prevent the list
>from changing at all while there's an update in progress, or else you
>risk using different versions for different CPUs if there's a list
>addition while a microcode update is in progress?
Both adding to and using the microcode cache are done with
microcode_mutex held. So I don't think it can happen.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()
2018-11-30 8:57 ` Chao Gao
@ 2018-11-30 9:38 ` Jan Beulich
0 siblings, 0 replies; 41+ messages in thread
From: Jan Beulich @ 2018-11-30 9:38 UTC (permalink / raw)
To: Chao Gao; +Cc: Andrew Cooper, Wei Liu, xen-devel, Roger Pau Monne
>>> On 30.11.18 at 09:57, <chao.gao@intel.com> wrote:
> On Thu, Nov 29, 2018 at 10:46:05AM +0100, Roger Pau Monné wrote:
>>On Thu, Nov 29, 2018 at 12:28:46PM +0800, Chao Gao wrote:
>>> It is better that all CPUs have the same microcode revision.
>>>
>>> Linux kernel rejects late microcode update if finding some CPUs
>>> offlined. I may port this patch to Xen too in a separate patch.
>>
>>What happens with hotplug CPUs?
>>
>>Even if you disable late loading when there are offlined CPUs you
>>still need to handle hotplug CPUs, which IMO should share the same
>>path with offlined CPUs: the microcode update should be done ASAP
>>after bringing the CPU up.
>
> In linux, CPU's being offline is just a logical offline. It may participate
> actions like MCE. It would lead to a situation that some cpus are using old
> microcode while some are using the new one, which introduces instability.
> CPU hotplug doesn't have such issue.
But please be careful not to disallow ucode loading in cases where
doing so is fine. I'm in particular thinking about "smt=no", where
secondary hyperthreads get parked. Since ucode updates on one
hyperthread of a core affect all hyperthreads, updating is fine in
this particular case.
Furthermore, if you look at my patch series "x86: more power-
efficient CPU parking", it is also within realm of possibility to
briefly wake up parked CPUs, just to get their ucode updated.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 5/6] microcode: delete microcode pointer and size from microcode_info
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
` (3 preceding siblings ...)
2018-11-28 5:34 ` [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode() Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 15:04 ` Roger Pau Monné
2018-11-28 5:34 ` [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading Chao Gao
5 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Chao Gao
Microcode pointer and size is passed to other cpu to parse microcode
locally. Now, parsing microcode is done on one CPU. Others just
find a suitable microcode stored in microcode_cache.
Signed-off-by: Chao Gao <chao.gao@intel.com>
---
xen/arch/x86/microcode.c | 13 ++++++-------
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index cca7b2c..0b435f4 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -190,9 +190,7 @@ DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
struct microcode_info {
unsigned int cpu;
- uint32_t buffer_size;
int error;
- char buffer[1];
};
static void __microcode_fini_cpu(unsigned int cpu)
@@ -270,6 +268,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
struct microcode_info *info;
unsigned int cpu = smp_processor_id();
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+ void * buffer;
if ( len != (uint32_t)len )
return -E2BIG;
@@ -277,11 +276,12 @@ 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 )
+ info = xmalloc(struct microcode_info);
+ buffer = xmalloc_bytes(len);
+ if ( !info || !buffer )
return -ENOMEM;
- ret = copy_from_guest(info->buffer, buf, len);
+ ret = copy_from_guest(buffer, buf, len);
if ( ret != 0 )
{
xfree(info);
@@ -302,7 +302,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
if ( likely(!ret) )
- ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
+ ret = microcode_ops->cpu_request_microcode(cpu, buffer, len);
else
__microcode_fini_cpu(cpu);
@@ -314,7 +314,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
return -EINVAL;
}
- info->buffer_size = len;
info->error = 0;
info->cpu = cpumask_first(&cpu_online_map);
--
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] 41+ messages in thread* Re: [PATCH v4 5/6] microcode: delete microcode pointer and size from microcode_info
2018-11-28 5:34 ` [PATCH v4 5/6] microcode: delete microcode pointer and size from microcode_info Chao Gao
@ 2018-11-28 15:04 ` Roger Pau Monné
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 15:04 UTC (permalink / raw)
To: Chao Gao; +Cc: xen-devel, Wei Liu, Jan Beulich, Andrew Cooper
On Wed, Nov 28, 2018 at 01:34:15PM +0800, Chao Gao wrote:
> Microcode pointer and size is passed to other cpu to parse microcode
> locally. Now, parsing microcode is done on one CPU. Others just
> find a suitable microcode stored in microcode_cache.
>
> Signed-off-by: Chao Gao <chao.gao@intel.com>
> ---
> xen/arch/x86/microcode.c | 13 ++++++-------
> 1 file changed, 6 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index cca7b2c..0b435f4 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -190,9 +190,7 @@ DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
>
> struct microcode_info {
> unsigned int cpu;
> - uint32_t buffer_size;
> int error;
> - char buffer[1];
> };
>
> static void __microcode_fini_cpu(unsigned int cpu)
> @@ -270,6 +268,7 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> struct microcode_info *info;
> unsigned int cpu = smp_processor_id();
> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> + void * buffer;
>
> if ( len != (uint32_t)len )
> return -E2BIG;
> @@ -277,11 +276,12 @@ 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 )
> + info = xmalloc(struct microcode_info);
> + buffer = xmalloc_bytes(len);
> + if ( !info || !buffer )
You are again likely to leak memory with this construct.
Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-28 5:34 [PATCH v4 0/6] improve late microcode loading Chao Gao
` (4 preceding siblings ...)
2018-11-28 5:34 ` [PATCH v4 5/6] microcode: delete microcode pointer and size from microcode_info Chao Gao
@ 2018-11-28 5:34 ` Chao Gao
2018-11-28 15:22 ` Roger Pau Monné
2018-12-11 17:01 ` Jan Beulich
5 siblings, 2 replies; 41+ messages in thread
From: Chao Gao @ 2018-11-28 5:34 UTC (permalink / raw)
To: xen-devel
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
Ashok Raj, Borislav Petkov, Thomas Gleixner, Chao Gao
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>
---
xen/arch/x86/microcode.c | 123 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 97 insertions(+), 26 deletions(-)
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 0b435f4..d5a2a94 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -22,6 +22,7 @@
*/
#include <xen/cpu.h>
+#include <xen/cpumask.h>
#include <xen/lib.h>
#include <xen/kernel.h>
#include <xen/init.h>
@@ -30,18 +31,25 @@
#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>
+/* By default, wait for 30000us */
+#define MICROCODE_DEFAULT_TIMEOUT_US 30000
+
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;
/*
* If we scan the initramfs.cpio for the early microcode code
@@ -189,8 +197,7 @@ static DEFINE_SPINLOCK(microcode_mutex);
DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
struct microcode_info {
- unsigned int cpu;
- int error;
+ atomic_t cpu_in, cpu_out;
};
static void __microcode_fini_cpu(unsigned int cpu)
@@ -242,31 +249,62 @@ static int microcode_update_cpu(void)
return err;
}
-static long do_microcode_update(void *_info)
+/* Wait for all CPUs to rendezvous with a timeout (us) */
+static int wait_for_cpus(atomic_t *cnt, unsigned int timeout)
{
- struct microcode_info *info = _info;
- int error;
+ unsigned int cpus = num_online_cpus();
- BUG_ON(info->cpu != smp_processor_id());
+ atomic_inc(cnt);
- error = microcode_update_cpu();
- if ( error )
- info->error = error;
+ while ( atomic_read(cnt) != cpus )
+ {
+ if ( timeout <= 0 )
+ {
+ printk("Timeout when waiting for CPUs calling in\n");
+ return -EBUSY;
+ }
+ udelay(1);
+ timeout--;
+ }
- 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);
+ return 0;
+}
- error = info->error;
- xfree(info);
- return error;
+static int do_microcode_update(void *_info)
+{
+ struct microcode_info *info = _info;
+ unsigned int cpu = smp_processor_id();
+ int ret;
+
+ ret = wait_for_cpus(&info->cpu_in, MICROCODE_DEFAULT_TIMEOUT_US);
+ if ( ret )
+ return ret;
+
+ /*
+ * Initiate an update on all processors which don't have an online sibling
+ * thread with a lower thread id. Other sibling threads just await the
+ * completion of microcode update.
+ */
+ if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
+ ret = microcode_update_cpu();
+ /*
+ * Increase the wait timeout to a safe value here since we're serializing
+ * the microcode update and that could take a while on a large number of
+ * CPUs. And that is fine as the *actual* timeout will be determined by
+ * the last CPU finished updating and thus cut short
+ */
+ if ( wait_for_cpus(&info->cpu_out, MICROCODE_DEFAULT_TIMEOUT_US *
+ nr_cores) )
+ panic("Timeout when finishing updating microcode");
+
+ return ret;
}
int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
{
int ret;
- struct microcode_info *info;
unsigned int cpu = smp_processor_id();
+ struct microcode_info *info;
struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
void * buffer;
@@ -283,19 +321,20 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
ret = copy_from_guest(buffer, buf, len);
if ( ret != 0 )
+ goto free;
+
+ /* cpu_online_map must not change during update */
+ if ( !get_cpu_maps() )
{
- xfree(info);
- return ret;
+ ret = -EBUSY;
+ goto free;
}
if ( microcode_ops->start_update )
{
ret = microcode_ops->start_update();
if ( ret != 0 )
- {
- xfree(info);
- return ret;
- }
+ goto put;
}
spin_lock(µcode_mutex);
@@ -311,13 +350,45 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
if ( ret <= 0 )
{
printk("No valid or newer microcode found. Update abort!\n");
- return -EINVAL;
+ ret = -EINVAL;
+ goto put;
}
- info->error = 0;
- info->cpu = cpumask_first(&cpu_online_map);
+ atomic_set(&info->cpu_in, 0);
+ atomic_set(&info->cpu_out, 0);
+
+ /* 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("%d cores are to update its microcode\n", nr_cores);
- return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
+ /*
+ * 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, info, NR_CPUS);
+ watchdog_enable();
+
+ put:
+ put_cpu_maps();
+ free:
+ xfree(info);
+ return ret;
}
static int __init microcode_init(void)
--
1.8.3.1
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 41+ messages in thread* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-28 5:34 ` [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading Chao Gao
@ 2018-11-28 15:22 ` Roger Pau Monné
2018-11-29 4:43 ` Chao Gao
2018-12-11 17:01 ` Jan Beulich
1 sibling, 1 reply; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-28 15:22 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Ashok Raj
On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
> 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]
If this patch is the squash of two Linux commits, please post the
ported versions of the two commits separately.
> 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>
> ---
> xen/arch/x86/microcode.c | 123 +++++++++++++++++++++++++++++++++++++----------
> 1 file changed, 97 insertions(+), 26 deletions(-)
>
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 0b435f4..d5a2a94 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -22,6 +22,7 @@
> */
>
> #include <xen/cpu.h>
> +#include <xen/cpumask.h>
> #include <xen/lib.h>
> #include <xen/kernel.h>
> #include <xen/init.h>
> @@ -30,18 +31,25 @@
> #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>
>
> +/* By default, wait for 30000us */
> +#define MICROCODE_DEFAULT_TIMEOUT_US 30000
> +
> 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;
>
> /*
> * If we scan the initramfs.cpio for the early microcode code
> @@ -189,8 +197,7 @@ static DEFINE_SPINLOCK(microcode_mutex);
> DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
>
> struct microcode_info {
> - unsigned int cpu;
> - int error;
> + atomic_t cpu_in, cpu_out;
Can you make this variables global to the file and just remove
microcode_info?
> };
>
> static void __microcode_fini_cpu(unsigned int cpu)
> @@ -242,31 +249,62 @@ static int microcode_update_cpu(void)
> return err;
> }
>
> -static long do_microcode_update(void *_info)
> +/* Wait for all CPUs to rendezvous with a timeout (us) */
> +static int wait_for_cpus(atomic_t *cnt, unsigned int timeout)
> {
> - struct microcode_info *info = _info;
> - int error;
> + unsigned int cpus = num_online_cpus();
>
> - BUG_ON(info->cpu != smp_processor_id());
> + atomic_inc(cnt);
>
> - error = microcode_update_cpu();
> - if ( error )
> - info->error = error;
> + while ( atomic_read(cnt) != cpus )
> + {
> + if ( timeout <= 0 )
> + {
> + printk("Timeout when waiting for CPUs calling in\n");
> + return -EBUSY;
> + }
> + udelay(1);
> + timeout--;
> + }
>
> - 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);
> + return 0;
> +}
>
> - error = info->error;
> - xfree(info);
> - return error;
> +static int do_microcode_update(void *_info)
> +{
> + struct microcode_info *info = _info;
> + unsigned int cpu = smp_processor_id();
> + int ret;
> +
> + ret = wait_for_cpus(&info->cpu_in, MICROCODE_DEFAULT_TIMEOUT_US);
> + if ( ret )
> + return ret;
> +
> + /*
> + * Initiate an update on all processors which don't have an online sibling
> + * thread with a lower thread id. Other sibling threads just await the
> + * completion of microcode update.
> + */
> + if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
> + ret = microcode_update_cpu();
> + /*
> + * Increase the wait timeout to a safe value here since we're serializing
> + * the microcode update and that could take a while on a large number of
> + * CPUs. And that is fine as the *actual* timeout will be determined by
> + * the last CPU finished updating and thus cut short
> + */
> + if ( wait_for_cpus(&info->cpu_out, MICROCODE_DEFAULT_TIMEOUT_US *
> + nr_cores) )
Isn't this likely to trigger the watchdog on big systems? Oh I see
below that you disable the watchdog.
> + panic("Timeout when finishing updating microcode");
> +
> + return ret;
> }
>
> int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> {
> int ret;
> - struct microcode_info *info;
> unsigned int cpu = smp_processor_id();
> + struct microcode_info *info;
> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> void * buffer;
>
> @@ -283,19 +321,20 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>
> ret = copy_from_guest(buffer, buf, len);
> if ( ret != 0 )
> + goto free;
> +
> + /* cpu_online_map must not change during update */
> + if ( !get_cpu_maps() )
> {
> - xfree(info);
> - return ret;
> + ret = -EBUSY;
> + goto free;
> }
>
> if ( microcode_ops->start_update )
> {
> ret = microcode_ops->start_update();
> if ( ret != 0 )
> - {
> - xfree(info);
> - return ret;
> - }
> + goto put;
> }
>
> spin_lock(µcode_mutex);
> @@ -311,13 +350,45 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> if ( ret <= 0 )
> {
> printk("No valid or newer microcode found. Update abort!\n");
> - return -EINVAL;
> + ret = -EINVAL;
> + goto put;
> }
>
> - info->error = 0;
> - info->cpu = cpumask_first(&cpu_online_map);
> + atomic_set(&info->cpu_in, 0);
> + atomic_set(&info->cpu_out, 0);
> +
> + /* 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("%d cores are to update its microcode\n", nr_cores);
>
> - return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> + /*
> + * 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.
Well, the HT siblings will be executing code, since they are in a
while loop waiting for the non-siblings cores to finish updating.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-28 15:22 ` Roger Pau Monné
@ 2018-11-29 4:43 ` Chao Gao
2018-11-29 9:56 ` Roger Pau Monné
0 siblings, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-29 4:43 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Ashok Raj
On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
>On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
>> 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]
>
>If this patch is the squash of two Linux commits, please post the
>ported versions of the two commits separately.
I don't understand this one.
>
>> 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>
>> ---
>> xen/arch/x86/microcode.c | 123 +++++++++++++++++++++++++++++++++++++----------
>> 1 file changed, 97 insertions(+), 26 deletions(-)
>>
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 0b435f4..d5a2a94 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -22,6 +22,7 @@
>> */
>>
>> #include <xen/cpu.h>
>> +#include <xen/cpumask.h>
>> #include <xen/lib.h>
>> #include <xen/kernel.h>
>> #include <xen/init.h>
>> @@ -30,18 +31,25 @@
>> #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>
>>
>> +/* By default, wait for 30000us */
>> +#define MICROCODE_DEFAULT_TIMEOUT_US 30000
>> +
>> 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;
>>
>> /*
>> * If we scan the initramfs.cpio for the early microcode code
>> @@ -189,8 +197,7 @@ static DEFINE_SPINLOCK(microcode_mutex);
>> DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
>>
>> struct microcode_info {
>> - unsigned int cpu;
>> - int error;
>> + atomic_t cpu_in, cpu_out;
>
>Can you make this variables global to the file and just remove
>microcode_info?
Yes. Good suggestion.
>
>> };
>>
>> static void __microcode_fini_cpu(unsigned int cpu)
>> @@ -242,31 +249,62 @@ static int microcode_update_cpu(void)
>> return err;
>> }
>>
>> -static long do_microcode_update(void *_info)
>> +/* Wait for all CPUs to rendezvous with a timeout (us) */
>> +static int wait_for_cpus(atomic_t *cnt, unsigned int timeout)
>> {
>> - struct microcode_info *info = _info;
>> - int error;
>> + unsigned int cpus = num_online_cpus();
>>
>> - BUG_ON(info->cpu != smp_processor_id());
>> + atomic_inc(cnt);
>>
>> - error = microcode_update_cpu();
>> - if ( error )
>> - info->error = error;
>> + while ( atomic_read(cnt) != cpus )
>> + {
>> + if ( timeout <= 0 )
>> + {
>> + printk("Timeout when waiting for CPUs calling in\n");
>> + return -EBUSY;
>> + }
>> + udelay(1);
>> + timeout--;
>> + }
>>
>> - 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);
>> + return 0;
>> +}
>>
>> - error = info->error;
>> - xfree(info);
>> - return error;
>> +static int do_microcode_update(void *_info)
>> +{
>> + struct microcode_info *info = _info;
>> + unsigned int cpu = smp_processor_id();
>> + int ret;
>> +
>> + ret = wait_for_cpus(&info->cpu_in, MICROCODE_DEFAULT_TIMEOUT_US);
>> + if ( ret )
>> + return ret;
>> +
>> + /*
>> + * Initiate an update on all processors which don't have an online sibling
>> + * thread with a lower thread id. Other sibling threads just await the
>> + * completion of microcode update.
>> + */
>> + if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>> + ret = microcode_update_cpu();
>> + /*
>> + * Increase the wait timeout to a safe value here since we're serializing
>> + * the microcode update and that could take a while on a large number of
>> + * CPUs. And that is fine as the *actual* timeout will be determined by
>> + * the last CPU finished updating and thus cut short
>> + */
>> + if ( wait_for_cpus(&info->cpu_out, MICROCODE_DEFAULT_TIMEOUT_US *
>> + nr_cores) )
>
>Isn't this likely to trigger the watchdog on big systems? Oh I see
>below that you disable the watchdog.
>
>> + panic("Timeout when finishing updating microcode");
>> +
>> + return ret;
>> }
>>
>> int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> {
>> int ret;
>> - struct microcode_info *info;
>> unsigned int cpu = smp_processor_id();
>> + struct microcode_info *info;
>> struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> void * buffer;
>>
>> @@ -283,19 +321,20 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>
>> ret = copy_from_guest(buffer, buf, len);
>> if ( ret != 0 )
>> + goto free;
>> +
>> + /* cpu_online_map must not change during update */
>> + if ( !get_cpu_maps() )
>> {
>> - xfree(info);
>> - return ret;
>> + ret = -EBUSY;
>> + goto free;
>> }
>>
>> if ( microcode_ops->start_update )
>> {
>> ret = microcode_ops->start_update();
>> if ( ret != 0 )
>> - {
>> - xfree(info);
>> - return ret;
>> - }
>> + goto put;
>> }
>>
>> spin_lock(µcode_mutex);
>> @@ -311,13 +350,45 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>> if ( ret <= 0 )
>> {
>> printk("No valid or newer microcode found. Update abort!\n");
>> - return -EINVAL;
>> + ret = -EINVAL;
>> + goto put;
>> }
>>
>> - info->error = 0;
>> - info->cpu = cpumask_first(&cpu_online_map);
>> + atomic_set(&info->cpu_in, 0);
>> + atomic_set(&info->cpu_out, 0);
>> +
>> + /* 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("%d cores are to update its microcode\n", nr_cores);
>>
>> - return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>> + /*
>> + * 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.
>
>Well, the HT siblings will be executing code, since they are in a
>while loop waiting for the non-siblings cores to finish updating.
Strictly speaking, you are right. The 'idle' I think means no other
workload on the cpu except microcode loading (for a HT sibling which
isn't chosen to do the update, means waiting for the completion of
the other sibling).
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-29 4:43 ` Chao Gao
@ 2018-11-29 9:56 ` Roger Pau Monné
2018-11-29 22:43 ` Boris Ostrovsky
2018-11-30 9:01 ` Chao Gao
0 siblings, 2 replies; 41+ messages in thread
From: Roger Pau Monné @ 2018-11-29 9:56 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Ashok Raj
On Thu, Nov 29, 2018 at 12:43:25PM +0800, Chao Gao wrote:
> On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
> >On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
> >> 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]
> >
> >If this patch is the squash of two Linux commits, please post the
> >ported versions of the two commits separately.
>
> I don't understand this one.
You reference two Linux commits above, why is this done?
I assume this is because you are porting two Linux commits to Xen, in
which case I think that should be done in two different patches, or a
note needs to be added to why you merge two Linux commits into a
single Xen patch.
> >> @@ -311,13 +350,45 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
> >> if ( ret <= 0 )
> >> {
> >> printk("No valid or newer microcode found. Update abort!\n");
> >> - return -EINVAL;
> >> + ret = -EINVAL;
> >> + goto put;
> >> }
> >>
> >> - info->error = 0;
> >> - info->cpu = cpumask_first(&cpu_online_map);
> >> + atomic_set(&info->cpu_in, 0);
> >> + atomic_set(&info->cpu_out, 0);
> >> +
> >> + /* 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("%d cores are to update its microcode\n", nr_cores);
> >>
> >> - return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
> >> + /*
> >> + * 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.
> >
> >Well, the HT siblings will be executing code, since they are in a
> >while loop waiting for the non-siblings cores to finish updating.
>
> Strictly speaking, you are right. The 'idle' I think means no other
> workload on the cpu except microcode loading (for a HT sibling which
> isn't chosen to do the update, means waiting for the completion of
> the other sibling).
Could you clarify the comment then?
By workload you mean that no other microcode loading should be
attempted from a HT sibling?
Is there a set of instructions or functionality that cannot be used by
HT siblings while performing a microcode load?
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-29 9:56 ` Roger Pau Monné
@ 2018-11-29 22:43 ` Boris Ostrovsky
2018-11-30 9:46 ` Jan Beulich
2018-11-30 9:01 ` Chao Gao
1 sibling, 1 reply; 41+ messages in thread
From: Boris Ostrovsky @ 2018-11-29 22:43 UTC (permalink / raw)
To: Roger Pau Monné, Chao Gao
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Ashok Raj
On 11/29/18 4:56 AM, Roger Pau Monné wrote:
> On Thu, Nov 29, 2018 at 12:43:25PM +0800, Chao Gao wrote:
>> On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
>>> On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
>>>
>>>> @@ -311,13 +350,45 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>>> if ( ret <= 0 )
>>>> {
>>>> printk("No valid or newer microcode found. Update abort!\n");
>>>> - return -EINVAL;
>>>> + ret = -EINVAL;
>>>> + goto put;
>>>> }
>>>>
>>>> - info->error = 0;
>>>> - info->cpu = cpumask_first(&cpu_online_map);
>>>> + atomic_set(&info->cpu_in, 0);
>>>> + atomic_set(&info->cpu_out, 0);
>>>> +
>>>> + /* 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("%d cores are to update its microcode\n", nr_cores);
>>>>
>>>> - return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>>>> + /*
>>>> + * 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.
>>> Well, the HT siblings will be executing code, since they are in a
>>> while loop waiting for the non-siblings cores to finish updating.
>> Strictly speaking, you are right. The 'idle' I think means no other
>> workload on the cpu except microcode loading (for a HT sibling which
>> isn't chosen to do the update, means waiting for the completion of
>> the other sibling).
> Could you clarify the comment then?
>
> By workload you mean that no other microcode loading should be
> attempted from a HT sibling?
>
> Is there a set of instructions or functionality that cannot be used by
> HT siblings while performing a microcode load?
The sibling should really not execute anything. For example, when
updating from microcode which introduced MSR0x48 to a newer microcode
which also updates 0x48 behavior the MSR (apparently) momentarily
disappears. We've seen this reliably happen, with crashes when the
sibling tries to access the MSR while the other thread is loading the
microcode.
One other comment about this patch (which IIRC was raised by Andrew on
an earlier version) is that it may be worth to stop timer calibration. I
am pretty sure we've seen deadlocks, which is why we ended up disabling
it during microcode updates.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-29 22:43 ` Boris Ostrovsky
@ 2018-11-30 9:46 ` Jan Beulich
2018-11-30 16:49 ` Boris Ostrovsky
0 siblings, 1 reply; 41+ messages in thread
From: Jan Beulich @ 2018-11-30 9:46 UTC (permalink / raw)
To: Boris Ostrovsky
Cc: Kevin Tian, Borislav Petkov, Wei Liu, Ashok Raj, Andrew Cooper,
Jun Nakajima, xen-devel, tglx, Roger Pau Monne, Chao Gao
>>> On 29.11.18 at 23:43, <boris.ostrovsky@oracle.com> wrote:
> One other comment about this patch (which IIRC was raised by Andrew on
> an earlier version) is that it may be worth to stop timer calibration. I
> am pretty sure we've seen deadlocks, which is why we ended up disabling
> it during microcode updates.
I recall the claim, but I don't think I've ever seen proof. My point was
ans still is that if there's a problem with ucode loading using the
stop-machine logic here, then there's a problem with the stop-machine
logic in general, which would make other uses, perhaps most notably
rcu_barrier(), unsafe too. Otoh from your reply it's not clear whether
the observed issue wasn't with our present way of loading ucode,
but then it would put under question the general correctness of
continue_hypercall_on_cpu(), which again we use for more than just
ucode loading.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-30 9:46 ` Jan Beulich
@ 2018-11-30 16:49 ` Boris Ostrovsky
0 siblings, 0 replies; 41+ messages in thread
From: Boris Ostrovsky @ 2018-11-30 16:49 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Borislav Petkov, Wei Liu, Ashok Raj, Andrew Cooper,
Jun Nakajima, xen-devel, tglx, Roger Pau Monne, Chao Gao
On 11/30/18 4:46 AM, Jan Beulich wrote:
>>>> On 29.11.18 at 23:43, <boris.ostrovsky@oracle.com> wrote:
>> One other comment about this patch (which IIRC was raised by Andrew on
>> an earlier version) is that it may be worth to stop timer calibration. I
>> am pretty sure we've seen deadlocks, which is why we ended up disabling
>> it during microcode updates.
> I recall the claim, but I don't think I've ever seen proof.
I can't provide proof at this point, only somewhat vague memory of
seeing calibration code in the stack dump.
> My point was
> ans still is that if there's a problem with ucode loading using the
> stop-machine logic here, then there's a problem with the stop-machine
> logic in general, which would make other uses, perhaps most notably
> rcu_barrier(), unsafe too.
Possibly.
rcu_barrier() appears to be only used in cpu hotplug code and power
management, and I don't know whether either has been tested under stress.
In our case it would take multiple microcode updates on relatively large
(~100 cpus) systems before we'd hit the deadlock.
> Otoh from your reply it's not clear whether
> the observed issue wasn't with our present way of loading ucode,
> but then it would put under question the general correctness of
> continue_hypercall_on_cpu(), which again we use for more than just
> ucode loading.
It was with a variation of this new code, not with what's currently in
the tree.
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-29 9:56 ` Roger Pau Monné
2018-11-29 22:43 ` Boris Ostrovsky
@ 2018-11-30 9:01 ` Chao Gao
2019-01-15 15:24 ` Andrew Cooper
1 sibling, 1 reply; 41+ messages in thread
From: Chao Gao @ 2018-11-30 9:01 UTC (permalink / raw)
To: Roger Pau Monné
Cc: Kevin Tian, Wei Liu, Jan Beulich, Andrew Cooper, Jun Nakajima,
xen-devel, Thomas Gleixner, Borislav Petkov, Ashok Raj
On Thu, Nov 29, 2018 at 10:56:53AM +0100, Roger Pau Monné wrote:
>On Thu, Nov 29, 2018 at 12:43:25PM +0800, Chao Gao wrote:
>> On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
>> >On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
>> >> 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]
>> >
>> >If this patch is the squash of two Linux commits, please post the
>> >ported versions of the two commits separately.
>>
>> I don't understand this one.
>
>You reference two Linux commits above, why is this done?
>
>I assume this is because you are porting two Linux commits to Xen, in
>which case I think that should be done in two different patches, or a
>note needs to be added to why you merge two Linux commits into a
>single Xen patch.
The latter fixed a severe bug introduced the first one. Maybe I need
to add a note to clarify this.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-30 9:01 ` Chao Gao
@ 2019-01-15 15:24 ` Andrew Cooper
2019-01-15 16:24 ` Roger Pau Monné
0 siblings, 1 reply; 41+ messages in thread
From: Andrew Cooper @ 2019-01-15 15:24 UTC (permalink / raw)
To: Chao Gao, Roger Pau Monné
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, xen-devel,
Thomas Gleixner, Borislav Petkov, Ashok Raj
On 30/11/2018 09:01, Chao Gao wrote:
> On Thu, Nov 29, 2018 at 10:56:53AM +0100, Roger Pau Monné wrote:
>> On Thu, Nov 29, 2018 at 12:43:25PM +0800, Chao Gao wrote:
>>> On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
>>>> On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
>>>>> 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]
>>>> If this patch is the squash of two Linux commits, please post the
>>>> ported versions of the two commits separately.
>>> I don't understand this one.
>> You reference two Linux commits above, why is this done?
>>
>> I assume this is because you are porting two Linux commits to Xen, in
>> which case I think that should be done in two different patches, or a
>> note needs to be added to why you merge two Linux commits into a
>> single Xen patch.
> The latter fixed a severe bug introduced the first one. Maybe I need
> to add a note to clarify this.
That is fine. Given that there is a large divergence between the Linux
and Xen code, I would recommend not trying to port the Linux changes
patch for patch.
This patch is fine in this form, IMO.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2019-01-15 15:24 ` Andrew Cooper
@ 2019-01-15 16:24 ` Roger Pau Monné
0 siblings, 0 replies; 41+ messages in thread
From: Roger Pau Monné @ 2019-01-15 16:24 UTC (permalink / raw)
To: Andrew Cooper
Cc: Kevin Tian, Wei Liu, Jan Beulich, Jun Nakajima, xen-devel,
Ashok Raj, Borislav Petkov, Thomas Gleixner, Chao Gao
On Tue, Jan 15, 2019 at 03:24:31PM +0000, Andrew Cooper wrote:
> On 30/11/2018 09:01, Chao Gao wrote:
> > On Thu, Nov 29, 2018 at 10:56:53AM +0100, Roger Pau Monné wrote:
> >> On Thu, Nov 29, 2018 at 12:43:25PM +0800, Chao Gao wrote:
> >>> On Wed, Nov 28, 2018 at 04:22:09PM +0100, Roger Pau Monné wrote:
> >>>> On Wed, Nov 28, 2018 at 01:34:16PM +0800, Chao Gao wrote:
> >>>>> 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]
> >>>> If this patch is the squash of two Linux commits, please post the
> >>>> ported versions of the two commits separately.
> >>> I don't understand this one.
> >> You reference two Linux commits above, why is this done?
> >>
> >> I assume this is because you are porting two Linux commits to Xen, in
> >> which case I think that should be done in two different patches, or a
> >> note needs to be added to why you merge two Linux commits into a
> >> single Xen patch.
> > The latter fixed a severe bug introduced the first one. Maybe I need
> > to add a note to clarify this.
>
> That is fine. Given that there is a large divergence between the Linux
> and Xen code, I would recommend not trying to port the Linux changes
> patch for patch.
>
> This patch is fine in this form, IMO.
Sorry, somehow this slipped through the cracks and I didn't reply. I
agree it's fine as a single commit since it doesn't make any sense to
commit something with a know bug that's fixed by the next commit.
Thanks, Roger.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-11-28 5:34 ` [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading Chao Gao
2018-11-28 15:22 ` Roger Pau Monné
@ 2018-12-11 17:01 ` Jan Beulich
2018-12-11 18:16 ` Raj, Ashok
2018-12-12 4:53 ` Chao Gao
1 sibling, 2 replies; 41+ messages in thread
From: Jan Beulich @ 2018-12-11 17:01 UTC (permalink / raw)
To: Chao Gao
Cc: Kevin Tian, Wei Liu, Ashok Raj, Andrew Cooper, Jun Nakajima,
xen-devel, tglx, Borislav Petkov
>>> On 28.11.18 at 06:34, <chao.gao@intel.com> wrote:
> 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.
So you still didn't switch to process cores or at the very least
sockets in parallel?
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-12-11 17:01 ` Jan Beulich
@ 2018-12-11 18:16 ` Raj, Ashok
2018-12-12 7:26 ` Jan Beulich
2018-12-12 4:53 ` Chao Gao
1 sibling, 1 reply; 41+ messages in thread
From: Raj, Ashok @ 2018-12-11 18:16 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Wei Liu, Ashok Raj, Andrew Cooper, Jun Nakajima,
xen-devel, tglx, Borislav Petkov, Chao Gao
On Tue, Dec 11, 2018 at 10:01:11AM -0700, Jan Beulich wrote:
> >>> On 28.11.18 at 06:34, <chao.gao@intel.com> wrote:
> > 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.
>
> So you still didn't switch to process cores or at the very least
> sockets in parallel?
Let me sync with Gao on this.. we did do a patch for upstream kernel
but Boris wasn't interested in talking any patches to improve
live microcode update because there is no report of breakage with what we do
today with the big hammer approach. Plus the preferred path should be BIOS,
next is early load and 3rd option is live update.
But since this is xen a different source base you are free to make
those changes.
BTW: Apart from the fact its ugly and take a loooong time to complete, do you
have any practical isssues you want to highlight? maybe that can
help upstream as well.
Cheers,
Ashok
>
> Jan
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-12-11 18:16 ` Raj, Ashok
@ 2018-12-12 7:26 ` Jan Beulich
2018-12-13 2:10 ` Boris Ostrovsky
0 siblings, 1 reply; 41+ messages in thread
From: Jan Beulich @ 2018-12-12 7:26 UTC (permalink / raw)
To: Ashok Raj
Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel,
tglx, Borislav Petkov, Chao Gao
>>> On 11.12.18 at 19:16, <ashok.raj@intel.com> wrote:
> BTW: Apart from the fact its ugly and take a loooong time to complete, do you
> have any practical isssues you want to highlight? maybe that can
> help upstream as well.
The situation for a kernel and a hypervisor might be different here
(but in the Linux case may then be more hypervisor-like when
considering KVM): The hypervisor needs to make sure in particular
time management within guests won't break. Stopping the
machine for an extended period of time may not be helpful there.
For processes in an OS the constraints might not be as tight, but
I could imaging problems even there in some less common cases.
That said, I'm not convinced at all it is a good idea to load new
ucode while _any_ guests are running, but we're talking about a
last resort approach here anyway.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-12-12 7:26 ` Jan Beulich
@ 2018-12-13 2:10 ` Boris Ostrovsky
0 siblings, 0 replies; 41+ messages in thread
From: Boris Ostrovsky @ 2018-12-13 2:10 UTC (permalink / raw)
To: Jan Beulich, Ashok Raj
Cc: Kevin Tian, Wei Liu, Andrew Cooper, Jun Nakajima, xen-devel,
tglx, Borislav Petkov, Chao Gao
On 12/12/18 2:26 AM, Jan Beulich wrote:
>>>> On 11.12.18 at 19:16, <ashok.raj@intel.com> wrote:
>> BTW: Apart from the fact its ugly and take a loooong time to complete, do you
>> have any practical isssues you want to highlight? maybe that can
>> help upstream as well.
> The situation for a kernel and a hypervisor might be different here
> (but in the Linux case may then be more hypervisor-like when
> considering KVM): The hypervisor needs to make sure in particular
> time management within guests won't break. Stopping the
> machine for an extended period of time may not be helpful there.
> For processes in an OS the constraints might not be as tight, but
> I could imaging problems even there in some less common cases.
>
> That said, I'm not convinced at all it is a good idea to load new
> ucode while _any_ guests are running, but we're talking about a
> last resort approach here anyway.
>
BTW, one thing I meant to mention about this patch earlier: we observed
that updating microcode from only one sibling resulted in guest reading
stale version value from another thread. This causes at least some
Windows versions to crash.
We ended up executing "wrmsr(0x8b, 0); cpuid_eax(1);" on all threads
after the update. (Another alternative could be to intercept the reads)
-boris
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 6/6] x86/microcode: Synchronize late microcode loading
2018-12-11 17:01 ` Jan Beulich
2018-12-11 18:16 ` Raj, Ashok
@ 2018-12-12 4:53 ` Chao Gao
1 sibling, 0 replies; 41+ messages in thread
From: Chao Gao @ 2018-12-12 4:53 UTC (permalink / raw)
To: Jan Beulich
Cc: Kevin Tian, Wei Liu, Ashok Raj, Andrew Cooper, Jun Nakajima,
xen-devel, tglx, Borislav Petkov
On Tue, Dec 11, 2018 at 10:01:11AM -0700, Jan Beulich wrote:
>>>> On 28.11.18 at 06:34, <chao.gao@intel.com> wrote:
>> 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.
>
>So you still didn't switch to process cores or at the very least
>sockets in parallel?
Yes. This patch allows different cores to run microcode_update_cpu()
in parallel. It is the lock in that function that make microcode update
sequentially. I plan to remove the lock in a separate patch and you can
find it in next version.
Thanks
Chao
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 41+ messages in thread