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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 31/03/2020 16:13, Jan Beulich wrote:
> 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@xxxxxxxx>
>> 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 ...

I'd still argue that "exit cleanly" is fine in context.

Without it, the end of buffer case happens fine as size becomes 0 and
terminates both loops, but in the case that there is a following
container, without it we fail because of the "!= UCODE_UCODE_TYPE" check.

~Andrew