diff mbox series

[v7,02/10] microcode/intel: extend microcode_update_match()

Message ID 1558945891-3015-3-git-send-email-chao.gao@intel.com (mailing list archive)
State Superseded
Headers show
Series improve late microcode loading | expand

Commit Message

Chao Gao May 27, 2019, 8:31 a.m. UTC
to a more generic function. Then, this function can compare two given
microcodes' signature/revision as well. Comparing two microcodes is
used to update the global microcode cache (introduced by the later
patches in this series) when a new microcode is given.

Note that enum microcode_match_result will be used in common code
(aka microcode.c), it has been placed in the common header.

Signed-off-by: Chao Gao <chao.gao@intel.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes in v6:
 - eliminate unnecessary type casting in microcode_update_match
 - check if a patch has an extend header

Changes in v5:
 - constify the extended_signature
 - use named enum type for the return value of microcode_update_match
---
 xen/arch/x86/microcode_intel.c  | 48 +++++++++++++++++++----------------------
 xen/include/asm-x86/microcode.h |  6 ++++++
 2 files changed, 28 insertions(+), 26 deletions(-)

Comments

Jan Beulich June 4, 2019, 2:39 p.m. UTC | #1
>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>      return 0;
>  }
>  
> -static inline int microcode_update_match(
> -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
> -    int sig, int pf)
> +static enum microcode_match_result microcode_update_match(
> +    const struct microcode_header_intel *mc_header, unsigned int sig,
> +    unsigned int pf, unsigned int rev)
>  {
> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
> +    const struct extended_sigtable *ext_header;
> +    const struct extended_signature *ext_sig;
> +    unsigned long data_size = get_datasize(mc_header);
> +    unsigned int i;
> +
> +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
> +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;

As indicated before, I think you would better also provide an "equal"
indication. Iirc I've told you that I have one system where the cores
get handed over from the BIOS in an inconsistent state (only core
has ucode loaded). Hence we'd want to be able to also _store_
ucode matching that found on CPU 0, without actually want to _load_
it there.

> -    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
> -            (mc_header->rev > uci->cpu_sig.rev));
> +    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
> +        return MIS_UCODE;

Okay, you're tightening the original <= to == here. But if you're
already tightening things, why don't you make sure you actually
have enough data to ...

> +    ext_header = (const void *)(mc_header + 1) + data_size;

... hold an extended header, and then also to hold ...

> +    ext_sig = (const void *)(ext_header + 1);
> +    for ( i = 0; i < ext_header->count; i++ )
> +        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
> +            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;

... enough array elements?

Jan
Roger Pau Monné June 5, 2019, 1:22 p.m. UTC | #2
On Tue, Jun 04, 2019 at 08:39:15AM -0600, Jan Beulich wrote:
> >>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
> > --- a/xen/arch/x86/microcode_intel.c
> > +++ b/xen/arch/x86/microcode_intel.c
> > @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
> >      return 0;
> >  }
> >  
> > -static inline int microcode_update_match(
> > -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
> > -    int sig, int pf)
> > +static enum microcode_match_result microcode_update_match(
> > +    const struct microcode_header_intel *mc_header, unsigned int sig,
> > +    unsigned int pf, unsigned int rev)
> >  {
> > -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
> > +    const struct extended_sigtable *ext_header;
> > +    const struct extended_signature *ext_sig;
> > +    unsigned long data_size = get_datasize(mc_header);
> > +    unsigned int i;
> > +
> > +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
> > +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
> 
> As indicated before, I think you would better also provide an "equal"
> indication. Iirc I've told you that I have one system where the cores
> get handed over from the BIOS in an inconsistent state (only core
> has ucode loaded). Hence we'd want to be able to also _store_
> ucode matching that found on CPU 0, without actually want to _load_
> it there.

Hm, without me being an expert on microcode, isn't such a system utterly
broken?

I'm not against making Xen capable of booting in this scenario where
firmware leaves the CPUs with different microcode versions, but this
is something that should be reported to the vendor in order to get it
fixed IMO?

What happens when you don't load any microcode at all, is the system
capable of operating normally with such mixed microcode?

Thanks, Roger.
Jan Beulich June 5, 2019, 2:16 p.m. UTC | #3
>>> On 05.06.19 at 15:22, <roger.pau@citrix.com> wrote:
> On Tue, Jun 04, 2019 at 08:39:15AM -0600, Jan Beulich wrote:
>> >>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
>> > --- a/xen/arch/x86/microcode_intel.c
>> > +++ b/xen/arch/x86/microcode_intel.c
>> > @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, 
> struct cpu_signature *csig)
>> >      return 0;
>> >  }
>> >  
>> > -static inline int microcode_update_match(
>> > -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>> > -    int sig, int pf)
>> > +static enum microcode_match_result microcode_update_match(
>> > +    const struct microcode_header_intel *mc_header, unsigned int sig,
>> > +    unsigned int pf, unsigned int rev)
>> >  {
>> > -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> > +    const struct extended_sigtable *ext_header;
>> > +    const struct extended_signature *ext_sig;
>> > +    unsigned long data_size = get_datasize(mc_header);
>> > +    unsigned int i;
>> > +
>> > +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>> > +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>> 
>> As indicated before, I think you would better also provide an "equal"
>> indication. Iirc I've told you that I have one system where the cores
>> get handed over from the BIOS in an inconsistent state (only core
>> has ucode loaded). Hence we'd want to be able to also _store_
>> ucode matching that found on CPU 0, without actually want to _load_
>> it there.
> 
> Hm, without me being an expert on microcode, isn't such a system utterly
> broken?

It's working fine (from all I can tell). It really depends on what exactly
the ucode update changes.

> I'm not against making Xen capable of booting in this scenario where
> firmware leaves the CPUs with different microcode versions, but this
> is something that should be reported to the vendor in order to get it
> fixed IMO?

I did report it, years ago.

> What happens when you don't load any microcode at all, is the system
> capable of operating normally with such mixed microcode?

Yes. Obviously there are anomalies (like fixed errata of particular
insns showing up or not depending on what core a process executes),
but the system has been stable this way for many years. (I've always
been doing early loading of ucode when running it with Xen, but the
distro that's on it never managed to arrange for early ucode loading
when booting a bare metal kernel).

Jan
Chao Gao June 6, 2019, 8:26 a.m. UTC | #4
On Tue, Jun 04, 2019 at 08:39:15AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
>> --- a/xen/arch/x86/microcode_intel.c
>> +++ b/xen/arch/x86/microcode_intel.c
>> @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
>>      return 0;
>>  }
>>  
>> -static inline int microcode_update_match(
>> -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>> -    int sig, int pf)
>> +static enum microcode_match_result microcode_update_match(
>> +    const struct microcode_header_intel *mc_header, unsigned int sig,
>> +    unsigned int pf, unsigned int rev)
>>  {
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>> +    const struct extended_sigtable *ext_header;
>> +    const struct extended_signature *ext_sig;
>> +    unsigned long data_size = get_datasize(mc_header);
>> +    unsigned int i;
>> +
>> +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>> +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>As indicated before, I think you would better also provide an "equal"
>indication. Iirc I've told you that I have one system where the cores
>get handed over from the BIOS in an inconsistent state (only core
>has ucode loaded). Hence we'd want to be able to also _store_
>ucode matching that found on CPU 0, without actually want to _load_
>it there.

Will do. What if no microcode update is provided in this case? Shall
we refuse to boot? If we allow different microcode revisions in the
system, it would complicate late microcode loading.

>
>> -    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>> -            (mc_header->rev > uci->cpu_sig.rev));
>> +    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
>> +        return MIS_UCODE;
>
>Okay, you're tightening the original <= to == here. But if you're
>already tightening things, why don't you make sure you actually
>have enough data to ...
>
>> +    ext_header = (const void *)(mc_header + 1) + data_size;
>
>... hold an extended header, and then also to hold ...
>
>> +    ext_sig = (const void *)(ext_header + 1);
>> +    for ( i = 0; i < ext_header->count; i++ )
>> +        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>> +            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>
>... enough array elements?

Do you think below incremental change is fine?

diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 94a1561..3dcbd28 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -138,18 +138,25 @@ static enum microcode_match_result microcode_update_match(
     const struct extended_signature *ext_sig;
     unsigned long data_size = get_datasize(mc_header);
     unsigned int i;
+    const void *end = (const void *)mc_header + get_totalsize(mc_header);
 
     if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
         return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
-    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
-        return MIS_UCODE;
-
     ext_header = (const void *)(mc_header + 1) + data_size;
     ext_sig = (const void *)(ext_header + 1);
-    for ( i = 0; i < ext_header->count; i++ )
-        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
-            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+    /*
+     * Make sure there is enough space to hold an extended header and enough
+     * array elements.
+     */
+    if ( (end >= (const void *)ext_sig) &&
+         (end >= (const void *)(ext_sig + ext_header->count)) )
+    {
+        for ( i = 0; i < ext_header->count; i++ )
+            if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
+                return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+    }
 
     return MIS_UCODE;
 }

Thanks
Chao
Jan Beulich June 6, 2019, 9:01 a.m. UTC | #5
>>> On 06.06.19 at 10:26, <chao.gao@intel.com> wrote:
> On Tue, Jun 04, 2019 at 08:39:15AM -0600, Jan Beulich wrote:
>>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
>>> --- a/xen/arch/x86/microcode_intel.c
>>> +++ b/xen/arch/x86/microcode_intel.c
>>> @@ -134,14 +134,28 @@ static int collect_cpu_info(unsigned int cpu_num, 
> struct cpu_signature *csig)
>>>      return 0;
>>>  }
>>>  
>>> -static inline int microcode_update_match(
>>> -    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
>>> -    int sig, int pf)
>>> +static enum microcode_match_result microcode_update_match(
>>> +    const struct microcode_header_intel *mc_header, unsigned int sig,
>>> +    unsigned int pf, unsigned int rev)
>>>  {
>>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
>>> +    const struct extended_sigtable *ext_header;
>>> +    const struct extended_signature *ext_sig;
>>> +    unsigned long data_size = get_datasize(mc_header);
>>> +    unsigned int i;
>>> +
>>> +    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>>> +        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>
>>As indicated before, I think you would better also provide an "equal"
>>indication. Iirc I've told you that I have one system where the cores
>>get handed over from the BIOS in an inconsistent state (only core
>>has ucode loaded). Hence we'd want to be able to also _store_
>>ucode matching that found on CPU 0, without actually want to _load_
>>it there.
> 
> Will do. What if no microcode update is provided in this case? Shall
> we refuse to boot? If we allow different microcode revisions in the
> system, it would complicate late microcode loading.

No, I don't think we should refuse to boot in such a case. We may
want to warn about the situation, but should continue booting in
a best effort manner. And no, I also don't think it'll complicate
late loading meaningfully. Late loading, after all, is then the only
way to get the system into "proper" shape again (other than
rebooting after making available ucode to early boot).

>>> -    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
>>> -            (mc_header->rev > uci->cpu_sig.rev));
>>> +    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
>>> +        return MIS_UCODE;
>>
>>Okay, you're tightening the original <= to == here. But if you're
>>already tightening things, why don't you make sure you actually
>>have enough data to ...
>>
>>> +    ext_header = (const void *)(mc_header + 1) + data_size;
>>
>>... hold an extended header, and then also to hold ...
>>
>>> +    ext_sig = (const void *)(ext_header + 1);
>>> +    for ( i = 0; i < ext_header->count; i++ )
>>> +        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
>>> +            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>>
>>... enough array elements?
> 
> Do you think below incremental change is fine?

Something along these lines, yes. I don't think ...

> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -138,18 +138,25 @@ static enum microcode_match_result microcode_update_match(
>      const struct extended_signature *ext_sig;
>      unsigned long data_size = get_datasize(mc_header);
>      unsigned int i;
> +    const void *end = (const void *)mc_header + get_totalsize(mc_header);
>  
>      if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
>          return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
>  
> -    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
> -        return MIS_UCODE;
> -
>      ext_header = (const void *)(mc_header + 1) + data_size;
>      ext_sig = (const void *)(ext_header + 1);
> -    for ( i = 0; i < ext_header->count; i++ )
> -        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
> -            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
> +
> +    /*
> +     * Make sure there is enough space to hold an extended header and enough
> +     * array elements.
> +     */
> +    if ( (end >= (const void *)ext_sig) &&
> +         (end >= (const void *)(ext_sig + ext_header->count)) )
> +    {
> +        for ( i = 0; i < ext_header->count; i++ )
> +            if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
> +                return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
> +    }

... this re-indentation of the for() is needed. Just like the function was
previously coded, the if() condition could be inverted and its body
could be "return MIS_UCODE;". But it's a style question, and hence
largely up to you.

Jan
diff mbox series

Patch

diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 22fdeca..ecec83b 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -134,14 +134,28 @@  static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
     return 0;
 }
 
-static inline int microcode_update_match(
-    unsigned int cpu_num, const struct microcode_header_intel *mc_header,
-    int sig, int pf)
+static enum microcode_match_result microcode_update_match(
+    const struct microcode_header_intel *mc_header, unsigned int sig,
+    unsigned int pf, unsigned int rev)
 {
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
+    const struct extended_sigtable *ext_header;
+    const struct extended_signature *ext_sig;
+    unsigned long data_size = get_datasize(mc_header);
+    unsigned int i;
+
+    if ( sigmatch(sig, mc_header->sig, pf, mc_header->pf) )
+        return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
 
-    return (sigmatch(sig, uci->cpu_sig.sig, pf, uci->cpu_sig.pf) &&
-            (mc_header->rev > uci->cpu_sig.rev));
+    if ( get_totalsize(mc_header) == (data_size + MC_HEADER_SIZE) )
+        return MIS_UCODE;
+
+    ext_header = (const void *)(mc_header + 1) + data_size;
+    ext_sig = (const void *)(ext_header + 1);
+    for ( i = 0; i < ext_header->count; i++ )
+        if ( sigmatch(sig, ext_sig[i].sig, pf, ext_sig[i].pf) )
+            return (mc_header->rev > rev) ? NEW_UCODE : OLD_UCODE;
+
+    return MIS_UCODE;
 }
 
 static int microcode_sanity_check(void *mc)
@@ -243,31 +257,13 @@  static int get_matching_microcode(const void *mc, unsigned int cpu)
 {
     struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     const struct microcode_header_intel *mc_header = mc;
-    const struct extended_sigtable *ext_header;
     unsigned long total_size = get_totalsize(mc_header);
-    int ext_sigcount, i;
-    struct extended_signature *ext_sig;
     void *new_mc;
 
-    if ( microcode_update_match(cpu, mc_header,
-                                mc_header->sig, mc_header->pf) )
-        goto find;
-
-    if ( total_size <= (get_datasize(mc_header) + MC_HEADER_SIZE) )
+    if ( microcode_update_match(mc, uci->cpu_sig.sig, uci->cpu_sig.pf,
+                                uci->cpu_sig.rev) != NEW_UCODE )
         return 0;
 
-    ext_header = mc + get_datasize(mc_header) + MC_HEADER_SIZE;
-    ext_sigcount = ext_header->count;
-    ext_sig = (void *)ext_header + EXT_HEADER_SIZE;
-    for ( i = 0; i < ext_sigcount; i++ )
-    {
-        if ( microcode_update_match(cpu, mc_header,
-                                    ext_sig->sig, ext_sig->pf) )
-            goto find;
-        ext_sig++;
-    }
-    return 0;
- find:
     pr_debug("microcode: CPU%d found a matching microcode update with"
              " version %#x (current=%#x)\n",
              cpu, mc_header->rev, uci->cpu_sig.rev);
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 23ea954..73ebe9a 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -3,6 +3,12 @@ 
 
 #include <xen/percpu.h>
 
+enum microcode_match_result {
+    OLD_UCODE, /* signature matched, but revision id isn't newer */
+    NEW_UCODE, /* signature matched, but revision id is newer */
+    MIS_UCODE, /* signature mismatched */
+};
+
 struct cpu_signature;
 struct ucode_cpu_info;