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

[Jan Beulich <JBeulich@xxxxxxxx>]

On 05.08.2019 09:02, Chao Gao wrote:
> On Fri, Aug 02, 2019 at 02:46:58PM +0000, Jan Beulich wrote:
>> On 01.08.2019 12:22, Chao Gao wrote:
>>> +bool microcode_update_cache(struct microcode_patch *patch)
>>> +{
>>> +
>>> +    ASSERT(spin_is_locked(&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.

If this is just a transitory step, then it's fine, but you should
say so in the description (and the patch to re-use the already
allocated space would then be nice to be part of this series).

>>> +static enum microcode_match_result compare_patch(
>>> +    const struct microcode_patch *new, const struct microcode_patch *old)
>>> +{
>>> +    const struct microcode_header_intel *old_header = &old->mc_intel->hdr;
>>> +
>>> +    return microcode_update_match(&new->mc_intel->hdr, old_header->sig,
>>> +                                  old_header->pf, old_header->rev);
>>
>> So this is exactly what I said on the earlier patch the function
>> cannot be used for. The way "pf" works precludes this, as said in
>> reply to an earlier version, and no-one corrected me (i.e. I'm in
>> no way excluding I'm misunderstanding something here).
> 
> How about just check 'rev' here and leave a comment here to explain
> why?
> 
> We drops all mismatched patches (including that only has 'pf'
> mismatched) compared with current CPU signature. Given that, it is
> fine to only check the revision number.

Ah, yes, that's apparently another option. But it'll require re-working
microcode_update_match(), or not using it here, afaics.

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