Re: [PATCH 09/11] x86/ucode/amd: Remove gratuitous memory allocations from cpu_request_microcode()

[Jan Beulich <jbeulich@suse.com>]

On 31.03.2020 16:55, Andrew Cooper wrote:
> On 31/03/2020 15:51, Jan Beulich wrote:
>> On 31.03.2020 12:05, Andrew Cooper wrote:
>>> @@ -497,57 +456,54 @@ static struct microcode_patch *cpu_request_microcode(const void *buf, size_t siz
>>>       * It's possible the data file has multiple matching ucode,
>>>       * lets keep searching till the latest version
>>>       */
>>> -    while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, size,
>>> -                                               &offset)) == 0 )
>>> +    buf  += offset;
>>> +    size -= offset;
>>>      {
>>> -        /*
>>> -         * If the new ucode covers current CPU, compare ucodes and store the
>>> -         * one with higher revision.
>>> -         */
>>> -        if ( (microcode_fits(mc_amd->mpb) != MIS_UCODE) &&
>>> -             (!saved || (compare_header(mc_amd->mpb, saved) == NEW_UCODE)) )
>>> +        while ( size )
>>>          {
>>> -            xfree(saved);
>>> -            saved = mc_amd->mpb;
>>> -        }
>>> -        else
>>> -        {
>>> -            xfree(mc_amd->mpb);
>>> -            mc_amd->mpb = NULL;
>>> -        }
>>> +            const struct container_microcode *mc;
>>> +
>>> +            if ( size < sizeof(*mc) ||
>>> +                 (mc = buf)->type != UCODE_UCODE_TYPE ||
>>> +                 size - sizeof(*mc) < mc->len ||
>>> +                 !verify_patch_size(mc->len) )
>>> +            {
>>> +                printk(XENLOG_ERR "microcode: Bad microcode data\n");
>>> +                error = -EINVAL;
>>> +                break;
>>> +            }
>>>  
>>> -        if ( offset >= size )
>>> -            break;
>>> +            /*
>>> +             * If the new ucode covers current CPU, compare ucodes and store the
>>> +             * one with higher revision.
>>> +             */
>>> +            if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
>>> +                 (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
>>> +            {
>>> +                saved = mc->patch;
>>> +                saved_size = mc->len;
>>> +            }
>>>  
>>> -        /*
>>> -         * 1. Given a situation where multiple containers exist and correct
>>> -         *    patch lives on a container that is not the last container.
>>> -         * 2. We match equivalent ids using find_equiv_cpu_id() from the
>>> -         *    earlier while() (On this case, matches on earlier container
>>> -         *    file and we break)
>>> -         * 3. Proceed to while ( (error = get_ucode_from_buffer_amd(mc_amd,
>>> -         *                                  buf, size, &offset)) == 0 )
>>> -         * 4. Find correct patch using microcode_fits() and apply the patch
>>> -         *    (Assume: apply_microcode() is successful)
>>> -         * 5. The while() loop from (3) continues to parse the binary as
>>> -         *    there is a subsequent container file, but...
>>> -         * 6. ...a correct patch can only be on one container and not on any
>>> -         *    subsequent ones. (Refer docs for more info) Therefore, we
>>> -         *    don't have to parse a subsequent container. So, we can abort
>>> -         *    the process here.
>>> -         * 7. This ensures that we retain a success value (= 0) to 'error'
>>> -         *    before if ( mpbuf->type != UCODE_UCODE_TYPE ) evaluates to
>>> -         *    false and returns -EINVAL.
>>> -         */
>>> -        if ( offset + SECTION_HDR_SIZE <= size &&
>>> -             *(const uint32_t *)(buf + offset) == UCODE_MAGIC )
>>> -            break;
>>> +            /* Move over the microcode blob. */
>>> +            buf  += sizeof(*mc) + mc->len;
>>> +            size -= sizeof(*mc) + mc->len;
>>> +
>>> +            /*
>>> +             * Peek ahead.  If we see the start of another container, we've
>>> +             * exhaused all microcode blobs in this container.  Exit cleanly.
>>> +             */
>>> +            if ( size >= 4 && *(const uint32_t *)buf == UCODE_MAGIC )
>>> +                break;
>> While, as already indicated, I agree with shrinking the big comment,
>> I think point 6 is what wants retaining in some form - it's not
>> obvious at all why a subsequent container couldn't contain a higher
>> rev ucode than what we've found. That comment refers us to docs, but
>> I couldn't find anything to this effect in PM Vol 2. Assuming this
>> indeed documented and true, with the comment extended accordingly
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> I think it is referring to the internal PPR, which isn't even the one we
> have access to.
> 
> As to the multiple containers aspect, I've deliberately "fixed" that in
> patch 11 so we do scan all the way to the end.

Right, meanwhile I've seen this. But shouldn't patch 11 then adjust at
least the "Exit cleanly" part of the comment? You're merely breaking
the inner loop then ...

Jan