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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 31.03.2020 17:47, Andrew Cooper wrote:
> 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.

Maybe; to me "exit" suggests more like being done with all processing /
looping. I'm not going to insist - you're the native speaker.

> 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.

Of course.

Jan