| Message ID | ced5b600ea66af84a9d53c467f99ec32ed6af742.1579727989.git.elnikety@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/microcode: Improve documentation and code | expand |
On 22.01.2020 23:30, Eslam Elnikety wrote: > When using `ucode=scan` and if a matching module is found, the microcode > payload is maintained in an xmalloc()'d region. This is unnecessary since > the bootmap would just do. Remove the xmalloc and xfree on the microcode > module scan path. > > This commit also does away with the restriction on the microcode module > size limit. The concern that a large microcode module would consume too > much memory preventing guests launch is misplaced since this is all the > init path. While having such safeguards is valuable, this should apply > across the board for all early/late microcode loading. Having it just on > the `scan` path is confusing. > > Looking forward, we are a bit closer (i.e., one xmalloc down) to pulling > the early microcode loading of the BSP a bit earlier in the early boot > process. This commit is the low hanging fruit. There is still a sizable > amount of work to get there as there are still a handful of xmalloc in > microcode_{amd,intel}.c. > > First, there are xmallocs on the path of finding a matching microcode > update. Similar to the commit at hand, searching through the microcode > blob can be done on the already present buffer with no need to xmalloc > any further. Even better, do the filtering in microcode.c before > requesting the microcode update on all CPUs. The latter requires careful > restructuring and exposing the arch-specific logic for iterating over > patches and declaring a match. > > Second, there are xmallocs for the microcode cache. Here, we would need > to ensure that the cache corresponding to the BSP gets xmalloc()'d and > populated after the fact. > > Signed-off-by: Eslam Elnikety <elnikety@amazon.com> > Acked-by: Jan Beulich <jbeulich@suse.com> Btw, if you could confirm (also for patch 4) that this is independent of patches 1 and 2 (it looks like so to me at least), then surely the two ones could go in independent and ahead of patch 2. Jan
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index e1d98fa55e..a662a7f438 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -141,11 +141,6 @@ static int __init parse_ucode(const char *s) } custom_param("ucode", parse_ucode); -/* - * 8MB ought to be enough. - */ -#define MAX_EARLY_CPIO_MICROCODE (8 << 20) - void __init microcode_scan_module( unsigned long *module_map, const multiboot_info_t *mbi) @@ -190,31 +185,12 @@ void __init microcode_scan_module( cd = find_cpio_data(p, _blob_start, _blob_size, &offset /* ignore */); if ( cd.data ) { - /* - * This is an arbitrary check - it would be sad if the blob - * consumed most of the memory and did not allow guests - * to launch. - */ - if ( cd.size > MAX_EARLY_CPIO_MICROCODE ) - { - printk("Multiboot %d microcode payload too big! (%ld, we can do %d)\n", - i, cd.size, MAX_EARLY_CPIO_MICROCODE); - goto err; - } - ucode_blob.size = cd.size; - ucode_blob.data = xmalloc_bytes(cd.size); - if ( !ucode_blob.data ) - cd.data = NULL; - else - memcpy(ucode_blob.data, cd.data, cd.size); + ucode_blob.size = cd.size; + ucode_blob.data = cd.data; + break; } bootstrap_map(NULL); - if ( cd.data ) - break; } - return; -err: - bootstrap_map(NULL); } void __init microcode_grab_module( unsigned long *module_map, @@ -734,7 +710,7 @@ static int __init microcode_init(void) */ if ( ucode_blob.size ) { - xfree(ucode_blob.data); + bootstrap_map(NULL); ucode_blob.size = 0; ucode_blob.data = NULL; }
When using `ucode=scan` and if a matching module is found, the microcode payload is maintained in an xmalloc()'d region. This is unnecessary since the bootmap would just do. Remove the xmalloc and xfree on the microcode module scan path. This commit also does away with the restriction on the microcode module size limit. The concern that a large microcode module would consume too much memory preventing guests launch is misplaced since this is all the init path. While having such safeguards is valuable, this should apply across the board for all early/late microcode loading. Having it just on the `scan` path is confusing. Looking forward, we are a bit closer (i.e., one xmalloc down) to pulling the early microcode loading of the BSP a bit earlier in the early boot process. This commit is the low hanging fruit. There is still a sizable amount of work to get there as there are still a handful of xmalloc in microcode_{amd,intel}.c. First, there are xmallocs on the path of finding a matching microcode update. Similar to the commit at hand, searching through the microcode blob can be done on the already present buffer with no need to xmalloc any further. Even better, do the filtering in microcode.c before requesting the microcode update on all CPUs. The latter requires careful restructuring and exposing the arch-specific logic for iterating over patches and declaring a match. Second, there are xmallocs for the microcode cache. Here, we would need to ensure that the cache corresponding to the BSP gets xmalloc()'d and populated after the fact. Signed-off-by: Eslam Elnikety <elnikety@amazon.com> Acked-by: Jan Beulich <jbeulich@suse.com> --- xen/arch/x86/microcode.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-)