Re: [Xen-devel] [PATCH v7 02/10] microcode/intel: extend microcode_update_match()

From: "Jan Beulich" <JBeulich@suse.com>
To: "Chao Gao" <chao.gao@intel.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Ashok Raj <ashok.raj@intel.com>, WeiLiu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v7 02/10] microcode/intel: extend microcode_update_match()
Date: Thu, 06 Jun 2019 03:01:04 -0600	[thread overview]
Message-ID: <5CF8D6500200007800235D03@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190606082644.GB8859@gao-cwp>

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel