| Message ID | d3fb2800517d79a422acc62628ad362f919824ea.1576630344.git.elnikety@amazon.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/microcode: Support builtin CPU microcode | expand |
On 18.12.2019 02:32, Eslam Elnikety wrote: > @@ -725,7 +701,7 @@ static int __init microcode_init(void) > */ > if ( ucode_blob.size ) > { > - xfree(ucode_blob.data); > + bootstrap_map(NULL); As much as I like the change, I wholeheartedly disagree with this aspect of it: You make it largely unpredictable when the boot mappings will go away - it becomes entirely dependent upon link order. And of course we really want these mappings to be gone, the very latest (I think), by the time we start bringing up APs (but generally the sooner the better). This is (one of?) the main reason(s) why it hadn't been done this way to begin with. The alternative is more complicated (set up a proper, long term mapping), but it's going to be more clean (including the mapping then also being suitable to post-boot CPU onlining). Jan
On 18.12.19 13:05, Jan Beulich wrote: > On 18.12.2019 02:32, Eslam Elnikety wrote: >> @@ -725,7 +701,7 @@ static int __init microcode_init(void) >> */ >> if ( ucode_blob.size ) >> { >> - xfree(ucode_blob.data); >> + bootstrap_map(NULL); > > As much as I like the change, I wholeheartedly disagree with this > aspect of it: You make it largely unpredictable when the boot > mappings will go away - it becomes entirely dependent upon link > order. And of course we really want these mappings to be gone, > the very latest (I think), by the time we start bringing up APs > (but generally the sooner the better). This is (one of?) the main > reason(s) why it hadn't been done this way to begin with. The > alternative is more complicated (set up a proper, long term > mapping), but it's going to be more clean (including the mapping > then also being suitable to post-boot CPU onlining). > This change is an improvement on the current status. We get to avoid xmalloc/memcpy in the case of a successful ucode=scan. The problematic aspect you highlight is anyway there regardless of this patch (ref. to the "else if ( ucode_mod.mod_end )" in microcode_init). The proper, long term mapping would be a welcome change, but is otherwise independent of this patch imo. Thanks, Eslam
On 19.12.2019 22:25, Eslam Elnikety wrote: > On 18.12.19 13:05, Jan Beulich wrote: >> On 18.12.2019 02:32, Eslam Elnikety wrote: >>> @@ -725,7 +701,7 @@ static int __init microcode_init(void) >>> */ >>> if ( ucode_blob.size ) >>> { >>> - xfree(ucode_blob.data); >>> + bootstrap_map(NULL); >> >> As much as I like the change, I wholeheartedly disagree with this >> aspect of it: You make it largely unpredictable when the boot >> mappings will go away - it becomes entirely dependent upon link >> order. And of course we really want these mappings to be gone, >> the very latest (I think), by the time we start bringing up APs >> (but generally the sooner the better). This is (one of?) the main >> reason(s) why it hadn't been done this way to begin with. The >> alternative is more complicated (set up a proper, long term >> mapping), but it's going to be more clean (including the mapping >> then also being suitable to post-boot CPU onlining). >> > > This change is an improvement on the current status. We get to avoid > xmalloc/memcpy in the case of a successful ucode=scan. The problematic > aspect you highlight is anyway there regardless of this patch (ref. to > the "else if ( ucode_mod.mod_end )" in microcode_init). Hmm, fair point. I'm not a fan of making a bad situation worse, but I think it's acceptable here: Acked-by: Jan Beulich <jbeulich@suse.com> Jan
On 20/12/2019 09:57, Jan Beulich wrote: > On 19.12.2019 22:25, Eslam Elnikety wrote: >> On 18.12.19 13:05, Jan Beulich wrote: >>> On 18.12.2019 02:32, Eslam Elnikety wrote: >>>> @@ -725,7 +701,7 @@ static int __init microcode_init(void) >>>> */ >>>> if ( ucode_blob.size ) >>>> { >>>> - xfree(ucode_blob.data); >>>> + bootstrap_map(NULL); >>> As much as I like the change, I wholeheartedly disagree with this >>> aspect of it: You make it largely unpredictable when the boot >>> mappings will go away - it becomes entirely dependent upon link >>> order. And of course we really want these mappings to be gone, >>> the very latest (I think), by the time we start bringing up APs >>> (but generally the sooner the better). This is (one of?) the main >>> reason(s) why it hadn't been done this way to begin with. The >>> alternative is more complicated (set up a proper, long term >>> mapping), but it's going to be more clean (including the mapping >>> then also being suitable to post-boot CPU onlining). >>> >> This change is an improvement on the current status. We get to avoid >> xmalloc/memcpy in the case of a successful ucode=scan. The problematic >> aspect you highlight is anyway there regardless of this patch (ref. to >> the "else if ( ucode_mod.mod_end )" in microcode_init). > Hmm, fair point. I'm not a fan of making a bad situation worse, > but I think it's acceptable here: > Acked-by: Jan Beulich <jbeulich@suse.com> Specifically relevant to this conversation is point 2 of https://lore.kernel.org/xen-devel/20200109193241.14542-1-andrew.cooper3@citrix.com/ where having dynamic bootmap mappings seems pointless when all we're doing is mapping RAM below 4G. ~Andrew
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 8b4d87782c..c878fc71ff 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -138,11 +138,6 @@ static int __init parse_ucode_param(const char *s) } custom_param("ucode", parse_ucode_param); -/* - * 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) @@ -187,31 +182,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, @@ -725,7 +701,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> --- xen/arch/x86/microcode.c | 32 ++++---------------------------- 1 file changed, 4 insertions(+), 28 deletions(-)