| Message ID | f997f1b3a7d515392c15f518ce886710068bb4ef.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: > Decouple the microcode indexing mechanism when using GRUB to that > when using EFI. This allows us to avoid the "unspecified effect" of > using `<integer>` when booting via EFI. Personally I'd prefer us to continue call this unspecified. It simply shouldn't be used. People shouldn't rely on it getting ignored. (Iirc, despite this being v1, you had previously submitted a patch to this effect, with me replaying about the same.) > With that, Xen can explicitly ignore that option when using EFI. Don't we do so already, by way of the "if ( !ucode_mod_forced )" you delete? > --- a/docs/misc/xen-command-line.pandoc > +++ b/docs/misc/xen-command-line.pandoc > @@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts at the end of > the modules in the GrUB entry (so with the blob commonly being last, > one could specify `ucode=-1`). Note that the value of zero is not valid > here (entry zero, i.e. the first module, is always the Dom0 kernel > -image). Note further that use of this option has an unspecified effect > -when used with xen.efi (there the concept of modules doesn't exist, and > -the blob gets specified via the `ucode=<filename>` config file/section > +image). This option should be used only with legacy boot, as it is explicitly > +ignored in EFI boot. When booting via EFI, the microcode update blob for > +early loading can be specified via the `ucode=<filename>` config file/section > entry; see [EFI configuration file description](efi.html)). Just like in patch 1, please distinguish "EFI boot" in general from "use of xen.efi" (relevant here of course only if indeed we went with this patch). Jan
Thanks for getting the other patches in the series onto master, Jan. This is the only patch out of this series that did not make it through, so I keeping my comments here. On 23.01.20 11:26, Jan Beulich wrote: > On 22.01.2020 23:30, Eslam Elnikety wrote: >> Decouple the microcode indexing mechanism when using GRUB to that >> when using EFI. This allows us to avoid the "unspecified effect" of >> using `<integer>` when booting via EFI. > > Personally I'd prefer us to continue call this unspecified. It > simply shouldn't be used. People shouldn't rely on it getting > ignored. (Iirc, despite this being v1, you had previously > submitted a patch to this effect, with me replaying about the > same.) > >> With that, Xen can explicitly ignore that option when using EFI. > > Don't we do so already, by way of the "if ( !ucode_mod_forced )" > you delete? > Not quite. If cfg.efi does not specify "ucode=<filename>", then a `ucode=<integer>` from the commandline gets parsed, resulting in the "unspecified" behaviour. Here, the ucode_mod_idx will hold the <integer> which will be used as index into the modules prepared in whatever order the efi/boot.c defines. In any case, let me know (and others too) if, later on, you may want to favor the explicitly ignore (opposed to unspecified) semantic and I will be happy to send another revision of this patch while addressing your comment below. >> --- a/docs/misc/xen-command-line.pandoc >> +++ b/docs/misc/xen-command-line.pandoc >> @@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts at the end of >> the modules in the GrUB entry (so with the blob commonly being last, >> one could specify `ucode=-1`). Note that the value of zero is not valid >> here (entry zero, i.e. the first module, is always the Dom0 kernel >> -image). Note further that use of this option has an unspecified effect >> -when used with xen.efi (there the concept of modules doesn't exist, and >> -the blob gets specified via the `ucode=<filename>` config file/section >> +image). This option should be used only with legacy boot, as it is explicitly >> +ignored in EFI boot. When booting via EFI, the microcode update blob for >> +early loading can be specified via the `ucode=<filename>` config file/section >> entry; see [EFI configuration file description](efi.html)). > > Just like in patch 1, please distinguish "EFI boot" in general from > "use of xen.efi" (relevant here of course only if indeed we went > with this patch). > > Jan > You have mentioned this very same remark on the other patch too. My understanding is that "EFI boot" may be ambiguous between (a) we are actually booting from xen.efi or (b) a system with EFI support (and the latter may still be falling onto grub for booting). Let me know if that's not actually what your remark is about. Cheers, Eslam
On 27.01.2020 20:34, Eslam Elnikety wrote: > On 23.01.20 11:26, Jan Beulich wrote: >> On 22.01.2020 23:30, Eslam Elnikety wrote: >>> Decouple the microcode indexing mechanism when using GRUB to that >>> when using EFI. This allows us to avoid the "unspecified effect" of >>> using `<integer>` when booting via EFI. >> >> Personally I'd prefer us to continue call this unspecified. It >> simply shouldn't be used. People shouldn't rely on it getting >> ignored. (Iirc, despite this being v1, you had previously >> submitted a patch to this effect, with me replaying about the >> same.) >> >>> With that, Xen can explicitly ignore that option when using EFI. >> >> Don't we do so already, by way of the "if ( !ucode_mod_forced )" >> you delete? >> > > Not quite. If cfg.efi does not specify "ucode=<filename>", then a > `ucode=<integer>` from the commandline gets parsed, resulting in the > "unspecified" behaviour. Here, the ucode_mod_idx will hold the <integer> > which will be used as index into the modules prepared in whatever order > the efi/boot.c defines. I guess I see now what you mean, but I'm still unconvinced it needs "fixing". After all - how is this different from "ucode=<N>" used with the wrong number in a xen.gz boot? In fact I'm also unconvinced the behavior of microcode_grab_module() to fall back as if "ucode=scan" was specified, in case something bogus was found with the specified number. >>> --- a/docs/misc/xen-command-line.pandoc >>> +++ b/docs/misc/xen-command-line.pandoc >>> @@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts at the end of >>> the modules in the GrUB entry (so with the blob commonly being last, >>> one could specify `ucode=-1`). Note that the value of zero is not valid >>> here (entry zero, i.e. the first module, is always the Dom0 kernel >>> -image). Note further that use of this option has an unspecified effect >>> -when used with xen.efi (there the concept of modules doesn't exist, and >>> -the blob gets specified via the `ucode=<filename>` config file/section >>> +image). This option should be used only with legacy boot, as it is explicitly >>> +ignored in EFI boot. When booting via EFI, the microcode update blob for >>> +early loading can be specified via the `ucode=<filename>` config file/section >>> entry; see [EFI configuration file description](efi.html)). >> >> Just like in patch 1, please distinguish "EFI boot" in general from >> "use of xen.efi" (relevant here of course only if indeed we went >> with this patch). >> > You have mentioned this very same remark on the other patch too. My > understanding is that "EFI boot" may be ambiguous between (a) we are > actually booting from xen.efi or (b) a system with EFI support (and the > latter may still be falling onto grub for booting). Let me know if > that's not actually what your remark is about. Well, booting from EFI can be either via xen.efi, or via xen.gz's efi_multiboot2() kind-of-entry-point. Yet the distinction discussed is strictly between xen.efi and xen.gz. Jan
diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc index ebec6d387e..821b9281a1 100644 --- a/docs/misc/xen-command-line.pandoc +++ b/docs/misc/xen-command-line.pandoc @@ -2147,9 +2147,9 @@ for updating CPU micrcode. When negative, counting starts at the end of the modules in the GrUB entry (so with the blob commonly being last, one could specify `ucode=-1`). Note that the value of zero is not valid here (entry zero, i.e. the first module, is always the Dom0 kernel -image). Note further that use of this option has an unspecified effect -when used with xen.efi (there the concept of modules doesn't exist, and -the blob gets specified via the `ucode=<filename>` config file/section +image). This option should be used only with legacy boot, as it is explicitly +ignored in EFI boot. When booting via EFI, the microcode update blob for +early loading can be specified via the `ucode=<filename>` config file/section entry; see [EFI configuration file description](efi.html)). 'scan' instructs the hypervisor to scan the multiboot images for an cpio diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c index 6ced293d88..e1d98fa55e 100644 --- a/xen/arch/x86/microcode.c +++ b/xen/arch/x86/microcode.c @@ -35,6 +35,7 @@ #include <xen/guest_access.h> #include <xen/earlycpio.h> #include <xen/watchdog.h> +#include <xen/efi.h> #include <asm/apic.h> #include <asm/delay.h> @@ -61,6 +62,7 @@ static module_t __initdata ucode_mod; static signed int __initdata ucode_mod_idx; static bool_t __initdata ucode_mod_forced; +static unsigned int __initdata ucode_mod_efi_idx; static unsigned int nr_cores; /* @@ -105,15 +107,10 @@ static struct microcode_patch *microcode_cache; void __init microcode_set_module(unsigned int idx) { - ucode_mod_idx = idx; + ucode_mod_efi_idx = idx; ucode_mod_forced = 1; } -/* - * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are - * optional. If the EFI has forced which of the multiboot payloads is to be - * used, only nmi=<bool> is parsed. - */ static int __init parse_ucode(const char *s) { const char *ss; @@ -126,18 +123,15 @@ static int __init parse_ucode(const char *s) if ( (val = parse_boolean("nmi", s, ss)) >= 0 ) ucode_in_nmi = val; - else if ( !ucode_mod_forced ) /* Not forced by EFI */ + else if ( (val = parse_boolean("scan", s, ss)) >= 0 ) + ucode_scan = val; + else { - if ( (val = parse_boolean("scan", s, ss)) >= 0 ) - ucode_scan = val; - else - { - const char *q; - - ucode_mod_idx = simple_strtol(s, &q, 0); - if ( q != ss ) - rc = -EINVAL; - } + const char *q; + + ucode_mod_idx = simple_strtol(s, &q, 0); + if ( q != ss ) + rc = -EINVAL; } s = ss + 1; @@ -228,6 +222,16 @@ void __init microcode_grab_module( { module_t *mod = (module_t *)__va(mbi->mods_addr); + if ( efi_enabled(EFI_BOOT) ) + { + if ( ucode_mod_forced ) /* Microcode specified by EFI */ + { + ucode_mod = mod[ucode_mod_efi_idx]; + return; + } + goto scan; + } + if ( ucode_mod_idx < 0 ) ucode_mod_idx += mbi->mods_count; if ( ucode_mod_idx <= 0 || ucode_mod_idx >= mbi->mods_count ||
Decouple the microcode indexing mechanism when using GRUB to that when using EFI. This allows us to avoid the "unspecified effect" of using `<integer>` when booting via EFI. With that, Xen can explicitly ignore that option when using EFI. This is the only functinal change this commit introduces. Update the command line documentation for consistency. As an added benefit, the 'parse_ucode' logic becomes independent of GRUB vs. EFI. While at it, drop the leading comment for parse_ucode. No practical use for it given this commit. Signed-off-by: Eslam Elnikety <elnikety@amazon.com> --- docs/misc/xen-command-line.pandoc | 6 ++--- xen/arch/x86/microcode.c | 38 +++++++++++++++++-------------- 2 files changed, 24 insertions(+), 20 deletions(-)