[v2,3/7] x86/ucode/intel: Remove gratuitous memory allocations from cpu_request_microcode()
diff mbox series

Message ID 20200327122901.11569-4-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/ucode: Cleanup and fixes - Part 3/n (Intel)
Related show

Commit Message

Andrew Cooper March 27, 2020, 12:28 p.m. UTC
cpu_request_microcode() needs to scan its container and duplicate one blob,
but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
Furthermore, the length checking is only safe from overflow in 64bit builds.

Delete get_next_ucode_from_buffer() and alter the purpose of the saved
variable to simply point somewhere in buf until we're ready to return.

This is only a modest reduction in absolute code size (-144), but avoids
making memory allocations for every blob in the container.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

v2:
 * Rebase over struct microcode_patch re-work
 * Reinstate printk() for bad data
---
 xen/arch/x86/cpu/microcode/intel.c | 65 +++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 40 deletions(-)

Comments

Jan Beulich March 31, 2020, 2:09 p.m. UTC | #1
On 27.03.2020 13:28, Andrew Cooper wrote:
> cpu_request_microcode() needs to scan its container and duplicate one blob,
> but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
> Furthermore, the length checking is only safe from overflow in 64bit builds.
> 
> Delete get_next_ucode_from_buffer() and alter the purpose of the saved
> variable to simply point somewhere in buf until we're ready to return.
> 
> This is only a modest reduction in absolute code size (-144), but avoids
> making memory allocations for every blob in the container.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>

> v2:
>  * Rebase over struct microcode_patch re-work
>  * Reinstate printk() for bad data

Ooi, did the number mentioned above indeed no change with this?
(I don't mean you to adjust it, as it's precise value is not
really meaningful anyway without also knowing compiler version
etc.)

Jan
Andrew Cooper March 31, 2020, 2:17 p.m. UTC | #2
On 31/03/2020 15:09, Jan Beulich wrote:
> On 27.03.2020 13:28, Andrew Cooper wrote:
>> cpu_request_microcode() needs to scan its container and duplicate one blob,
>> but the get_next_ucode_from_buffer() helper duplicates every blob in turn.
>> Furthermore, the length checking is only safe from overflow in 64bit builds.
>>
>> Delete get_next_ucode_from_buffer() and alter the purpose of the saved
>> variable to simply point somewhere in buf until we're ready to return.
>>
>> This is only a modest reduction in absolute code size (-144), but avoids
>> making memory allocations for every blob in the container.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>
>> v2:
>>  * Rebase over struct microcode_patch re-work
>>  * Reinstate printk() for bad data
> Ooi, did the number mentioned above indeed no change with this?
> (I don't mean you to adjust it, as it's precise value is not
> really meaningful anyway without also knowing compiler version
> etc.)

I actually stripped the number after re-reading this on xen-devel.  I
didn't go back to check, but it almost certainly isn't the same.

~Andrew

Patch
diff mbox series

diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 77539a00be..2b48959573 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -315,67 +315,52 @@  static int apply_microcode(const struct microcode_patch *patch)
     return 0;
 }
 
-static long get_next_ucode_from_buffer(struct microcode_intel **mc,
-                                       const uint8_t *buf, unsigned long size,
-                                       unsigned long offset)
-{
-    struct microcode_header_intel *mc_header;
-    unsigned long total_size;
-
-    /* No more data */
-    if ( offset >= size )
-        return 0;
-    mc_header = (struct microcode_header_intel *)(buf + offset);
-    total_size = get_totalsize(mc_header);
-
-    if ( (offset + total_size) > size )
-    {
-        printk(KERN_ERR "microcode: error! Bad data in microcode data file\n");
-        return -EINVAL;
-    }
-
-    *mc = xmemdup_bytes(mc_header, total_size);
-    if ( *mc == NULL )
-        return -ENOMEM;
-
-    return offset + total_size;
-}
-
 static struct microcode_patch *cpu_request_microcode(const void *buf,
                                                      size_t size)
 {
-    long offset = 0;
     int error = 0;
-    struct microcode_intel *mc, *saved = NULL;
+    const struct microcode_patch *saved = NULL;
     struct microcode_patch *patch = NULL;
 
-    while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
+    while ( size )
     {
-        error = microcode_sanity_check(mc);
-        if ( error )
+        const struct microcode_patch *mc;
+        unsigned int blob_size;
+
+        if ( size < MC_HEADER_SIZE ||       /* Insufficient space for header? */
+             (mc = buf)->hdr.hdrver != 1 || /* Unrecognised header version?   */
+             mc->hdr.ldrver != 1 ||         /* Unrecognised loader version?   */
+             size < (blob_size =            /* Insufficient space for patch?  */
+                     get_totalsize(&mc->hdr)) )
         {
-            xfree(mc);
+            error = -EINVAL;
+            printk(XENLOG_WARNING "microcode: Bad data in container\n");
             break;
         }
 
+        error = microcode_sanity_check(mc);
+        if ( error )
+            break;
+
         /*
          * If the new update covers current CPU, compare updates and store the
          * one with higher revision.
          */
         if ( (microcode_update_match(mc) != MIS_UCODE) &&
              (!saved || (mc->hdr.rev > saved->hdr.rev)) )
-        {
-            xfree(saved);
             saved = mc;
-        }
-        else
-            xfree(mc);
+
+        buf  += blob_size;
+        size -= blob_size;
     }
-    if ( offset < 0 )
-        error = offset;
 
     if ( saved )
-        patch = saved;
+    {
+        patch = xmemdup_bytes(saved, get_totalsize(&saved->hdr));
+
+        if ( !patch )
+            error = -ENOMEM;
+    }
 
     if ( error && !patch )
         patch = ERR_PTR(error);