Re: [Xen-devel] [PATCH v8 06/16] microcode: introduce a global cache of ucode patch

[Chao Gao <chao.gao@intel.com>]

On Fri, Aug 02, 2019 at 02:46:58PM +0000, Jan Beulich wrote:
>On 01.08.2019 12:22, Chao Gao wrote:
>> +bool microcode_update_cache(struct microcode_patch *patch)
>> +{
>> +
>> +    ASSERT(spin_is_locked(&microcode_mutex));
>> +
>> +    if ( !microcode_cache )
>> +        microcode_cache = patch;
>> +    else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
>> +                  NEW_UCODE )
>
>Indentation is wrong here.
>
>> +static struct microcode_patch *alloc_microcode_patch(
>> +    const struct microcode_amd *mc_amd)
>> +{
>> +    struct microcode_patch *microcode_patch = xmalloc(struct microcode_patch);
>> +    struct microcode_amd *cache = xmalloc(struct microcode_amd);
>> +    void *mpb = xmalloc_bytes(mc_amd->mpb_size);
>> +    struct equiv_cpu_entry *equiv_cpu_table =
>> +                                xmalloc_bytes(mc_amd->equiv_cpu_table_size);
>> +
>> +    if ( !microcode_patch || !cache || !mpb || !equiv_cpu_table )
>> +    {
>> +        xfree(microcode_patch);
>> +        xfree(cache);
>> +        xfree(mpb);
>> +        xfree(equiv_cpu_table);
>> +        return ERR_PTR(-ENOMEM);
>> +    }
>> +
>> +    memcpy(mpb, mc_amd->mpb, mc_amd->mpb_size);
>> +    cache->mpb = mpb;
>> +    cache->mpb_size = mc_amd->mpb_size;
>> +    memcpy(equiv_cpu_table, mc_amd->equiv_cpu_table,
>> +           mc_amd->equiv_cpu_table_size);
>> +    cache->equiv_cpu_table = equiv_cpu_table;
>> +    cache->equiv_cpu_table_size = mc_amd->equiv_cpu_table_size;
>> +    microcode_patch->mc_amd = cache;
>> +
>> +    return microcode_patch;
>> +}
>
>Why is it that everything needs to be copied here, rather than
>simply shuffling one (or a few) pointer(s)? Can't the caller
>simply install the argument it passes here as the new cache blob?

The old per-cpu cache would use the pointers. And the old cache struct
is removed in "microcode: remove struct ucode_cpu_info". I can add a
patch after that one to reuse the pointers. Otherwise, I may have to
merge following two patches into this one.

>
>> +static enum microcode_match_result compare_patch(
>> +    const struct microcode_patch *new, const struct microcode_patch *old)
>> +{
>> +    const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
>> +
>> +    return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
>> +                                  old_header->pf, old_header->rev);
>
>So this is exactly what I said on the earlier patch the function
>cannot be used for. The way "pf" works precludes this, as said in
>reply to an earlier version, and no-one corrected me (i.e. I'm in
>no way excluding I'm misunderstanding something here).

How about just check 'rev' here and leave a comment here to explain
why?

We drops all mismatched patches (including that only has 'pf'
mismatched) compared with current CPU signature. Given that, it is
fine to only check the revision number.

Thanks
Chao

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