Re: [Xen-devel] [PATCH v7 03/10] microcode: introduce a global cache of ucode patch

[Chao Gao <chao.gao@xxxxxxxxx>]

On Tue, Jun 04, 2019 at 09:03:20AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@xxxxxxxxx> wrote:
>> +bool microcode_update_cache(struct microcode_patch *patch)
>> +{
>> +
>> +    ASSERT(spin_is_locked(&microcode_mutex));
>> +
>> +    if ( !microcode_ops->match_cpu(patch) )
>> +        return false;
>> +
>> +    if ( !microcode_cache )
>> +        microcode_cache = patch;
>> +    else if ( microcode_ops->compare_patch(patch, microcode_cache) ==
>> +                  NEW_UCODE )
>> +    {
>> +        microcode_ops->free_patch(microcode_cache);
>> +        microcode_cache = patch;
>> +    }
>
>Hmm, okay, the way you do things here three enumeration values
>may indeed be sufficient. "old" may just be a little misleading then.
>(As to my respective comment on the previous patch.)
>
>> +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);
>> +        printk(XENLOG_ERR "microcode: Can not allocate memory\n");
>
>I'm not convinced this needs logging.
>
>> +        return ERR_PTR(-ENOMEM);
>> +    }
>> +
>> +    cache->equiv_cpu_table = equiv_cpu_table;
>> +    cache->mpb = mpb;
>> +    memcpy(cache->equiv_cpu_table, mc_amd->equiv_cpu_table,
>
>Why not use the local variable here and ...
>
>> +           mc_amd->equiv_cpu_table_size);
>> +    memcpy(cache->mpb, mc_amd->mpb, mc_amd->mpb_size);
>
>here? Less source code and presumably also slightly less binary
>code. In fact I wonder if you wouldn't better memcpy() first
>anyway, and only then store the values into the fields. It won't
>matter much with the global lock held, but it's generally good
>practice to do things in an order that won't risk to confuse
>hypothetical consumers of the data.

Will do.

>
>> +static void free_patch(struct microcode_patch *microcode_patch)
>> +{
>> +    struct microcode_amd *mc_amd = microcode_patch->mc_amd;
>> +
>> +    xfree(mc_amd->equiv_cpu_table);
>> +    xfree(mc_amd->mpb);
>> +    xfree(mc_amd);
>> +    xfree(microcode_patch);
>
>I think I said so before: Freeing of the generic wrapper struct
>would probably better be placed in generic code.

Do you mean something as shown below:

/* in generic code */

struct microcode_patch {
    union {
        struct microcode_intel *mc_intel;
        struct microcode_amd *mc_amd;
        void *mc;
    };
};

void microcode_free_patch(struct microcode_patch *microcode_patch)
{
    microcode_ops->free_patch(microcode_patch->mc);
    xfree(microcode_patch);
}

/* in vendor-specific (AMD) code */

static void free_patch(void *mc)
{
    struct microcode_amd *mc_amd = mc;

    xfree(mc_amd->equiv_cpu_table);
    xfree(mc_amd->mpb);
    xfree(mc_amd);
}

>
>> @@ -497,7 +558,20 @@ static int cpu_request_microcode(unsigned int cpu, 
>> const void *buf,
>>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>                                                 &offset)) == 0 )
>>      {
>> -        if ( microcode_fits(mc_amd, cpu) )
>> +        struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd);
>> +
>> +        if ( IS_ERR(new_patch) )
>> +        {
>> +            error = PTR_ERR(new_patch);
>> +            break;
>> +        }
>> +
>> +        if ( match_cpu(new_patch) )
>> +            microcode_update_cache(new_patch);
>> +        else
>> +            free_patch(new_patch);
>
>Why do you re-do what microcode_update_cache() already does?
>It calls ->match_cpu() and ->free_patch() all by itself. It looks as
>if it would need to gain one more ->free_patch() invocation though.
>

Will remove both invocations of match_cpu().

To support the case (the broken bios) you described, a patch which
needs to be stored isn't necessary to be newer than the microcode loaded
to current CPU. As long as the processor's signature is covered by the
patch, we will store the patch regardless the revision number.

Thanks
Chao

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