[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 31 Mar 2020 15:55:08 +0100
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Tue, 31 Mar 2020 14:55:23 +0000
  • Ironport-sdr: f5B05I3hyPAjWgMhBIktpgTogNjUR/MgfTr+mjQcNw/9R6/eQfHivj04mflFGJD8ToQgcTn47o Rayry4oBW2rdLliKyZxHF12y+m9taXDXLwHMXDyRuVbVzJ23DpjkmdxS6jbqY0uvEZaFViS7p6 m983Ftw+HYVbmmsMjmfc3qphEFmLKOGBE7klWKM2ftGzl731IHqwQQKrYYgnTpBR126fYveFTO Ytfbi226xSBlUU+4tBk/X2JuWpeHOf3IudrcCI5iK0iQpV91BJBAoc8dJcW6/iyveHn0tPuzMT MOM=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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.

Its a much more obvious way to do things, even if the default case is to
only provide a single container applicable to a specific family.

~Andrew



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.