Re: [Xen-devel] [PATCH v9 12/15] microcode: reduce memory allocation and copy when creating a patch

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

On Fri, Aug 23, 2019 at 10:11:21AM +0200, Roger Pau Monné wrote:
>On Mon, Aug 19, 2019 at 09:25:25AM +0800, Chao Gao wrote:
>> To create a microcode patch from a vendor-specific update,
>> allocate_microcode_patch() copied everything from the update.
>> It is not efficient. Essentially, we just need to go through
>> ucodes in the blob, find the one with the newest revision and
>> install it into the microcode_patch. In the process, buffers
>> like mc_amd, equiv_cpu_table (on AMD side), and mc (on Intel
>> side) can be reused. microcode_patch now is allocated after
>> it is sure that there is a matching ucode.
>
>Oh, I think this answers my question on a previous patch.
>
>For future series it would be nice to avoid so many rewrites in the
>same series, alloc_microcode_patch is already modified in a previous
>patch, just to be removed here. It also makes it harder to follow
>what's going on.

Got it. This patch is added in this new version. And some trivial
patches already got reviewed-by. So I don't merge it with them.

>>      while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
>>                                                 &offset)) == 0 )
>>      {
>> -        struct microcode_patch *new_patch = alloc_microcode_patch(mc_amd);
>> -
>> -        if ( IS_ERR(new_patch) )
>> -        {
>> -            error = PTR_ERR(new_patch);
>> -            break;
>> -        }
>> -
>>          /*
>> -         * If the new patch covers current CPU, compare patches and store the
>> +         * If the new ucode covers current CPU, compare ucodes and store the
>>           * one with higher revision.
>>           */
>> -        if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
>> -             (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
>> +#define REV_ID(mpb) (((struct microcode_header_amd *)(mpb))->processor_rev_id)
>> +        if ( (microcode_fits(mc_amd) != MIS_UCODE) &&
>> +             (!saved || (REV_ID(mc_amd->mpb) > REV_ID(saved))) )
>> +#undef REV_ID
>>          {
>> -            struct microcode_patch *tmp = patch;
>> -
>> -            patch = new_patch;
>> -            new_patch = tmp;
>> +            xfree(saved);
>> +            saved = mc_amd->mpb;
>> +            saved_size = mc_amd->mpb_size;
>>          }
>> -
>> -        if ( new_patch )
>> -            microcode_free_patch(new_patch);
>> +        else
>> +            xfree(mc_amd->mpb);

It might be better to move 'mc_amd->mpb = NULL' here.

>>  
>>          if ( offset >= bufsize )
>>              break;
>> @@ -593,9 +548,25 @@ static struct microcode_patch *cpu_request_microcode(const void *buf,
>>               *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>>              break;
>>      }
>> -    xfree(mc_amd->mpb);
>> -    xfree(mc_amd->equiv_cpu_table);
>> -    xfree(mc_amd);
>> +
>> +    if ( saved )
>> +    {
>> +        mc_amd->mpb = saved;
>> +        mc_amd->mpb_size = saved_size;
>> +        patch = xmalloc(struct microcode_patch);
>> +        if ( patch )
>> +            patch->mc_amd = mc_amd;
>> +        else
>> +        {
>> +            free_patch(mc_amd);
>> +            error = -ENOMEM;
>> +        }
>> +    }
>> +    else
>> +    {
>> +        mc_amd->mpb = NULL;
>
>What's the point in setting mpb to NULL if you are just going to free
>mc_amd below?

To avoid double free here. mc_amd->mpb is always freed or saved.
And here we want to free mc_amd itself and mc_amd->equiv_cpu_table.

>
>Also, I'm not sure I understand why you need to free mc_amd, isn't
>this buff memory that should be freed by the caller?

But mc_amd is allocated in this function.

>
>ie: in the Intel counterpart below you don't seem to free the mc
>cursor used for the get_next_ucode_from_buffer loop.

'mc' is saved if it is newer than current patch stored in 'saved'.
Otherwise 'mc' is freed immediately. So we don't need to free it
again after the while loop.

Thanks
Chao

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