| Message ID | 20200327122901.11569-4-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/ucode: Cleanup and fixes - Part 3/n (Intel) | expand |
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
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
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);
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(-)