[Xen-devel] [PATCH v1 0/4] x86/microcode: Improve documentation and code

[Xen-devel] [PATCH v1 0/4] x86/microcode: Improve documentation and code

[Eslam Elnikety]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 1/4] x86/microcode: Improve documentation for ucode=

[Eslam Elnikety]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Eslam Elnikety]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 3/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Eslam Elnikety]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 4/4] x86/microcode: use const qualifier for microcode buffer

[Eslam Elnikety]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 0/4] x86/microcode: Improve documentation and code

[Eslam Elnikety]

This patch series introduces improvements to the existing documentation
and code of x86/microcode. Patches 1 and 2 improve the documentation and
parsing for `ucode=`. Patches 3 and 4 introduce nits/improvements to the
microcode early loading code.

Some (variant of the) patches have been sent earlier under "Support builtin CPU
microcode" as those patches were motivated by discussions following the initial
submission of the builtin microcode. On a second thought, such improvements
should have gone independently. So here it goes. (Those improvements will be
dropped from the builtin microcode series as I submit its v3).

Changes since submitted under [v2] x86/microcode: Support builtin CPU microcode
- Patch 1: New / explicitly document the current behaviour of ucode=scan with EFI
- Patch 2: Fix index data type, drop unwelcomed function rename
- Patch 3 and 4: Added Acked-by, otherwise as before

Eslam Elnikety (4):
  x86/microcode: Improve documentation for ucode=
  x86/microcode: Improve parsing for ucode=
  x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data
  x86/microcode: use const qualifier for microcode buffer

 docs/misc/xen-command-line.pandoc | 14 ++++--
 xen/arch/x86/microcode.c          | 74 +++++++++++--------------------
 2 files changed, 37 insertions(+), 51 deletions(-)

-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 1/4] x86/microcode: Improve documentation for ucode=

[Eslam Elnikety]

Specify applicability and the default value. Also state that, in case of
EFI, the microcode update blob specified in the EFI cfg takes precedence
over `ucode=scan`, if the latter is specified on Xen commend line.

No functional changes.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
---
 docs/misc/xen-command-line.pandoc | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 981a5e2381..ebec6d387e 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2134,7 +2134,12 @@ logic applies:
 ### ucode (x86)
 > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
 
-Specify how and where to find CPU microcode update blob.
+    Applicability: x86
+    Default: `nmi`
+
+Controls for CPU microcode loading. For early loading, this parameter can
+specify how and where to find the microcode update blob. For late loading,
+this parameter specifies if the update happens within a NMI handler.
 
 'integer' specifies the CPU microcode update blob module index. When positive,
 this specifies the n-th module (in the GrUB entry, zero based) to be used
@@ -2152,6 +2157,7 @@ image that contains microcode. Depending on the platform the blob with the
 microcode in the cpio name space must be:
   - on Intel: kernel/x86/microcode/GenuineIntel.bin
   - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
+If EFI boot, the `ucode=<filename>` config takes precendence over `scan`.
 
 'nmi' determines late loading is performed in NMI handler or just in
 stop_machine context. In NMI handler, even NMIs are blocked, which is
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Eslam Elnikety]

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(-)

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 ||
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 3/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Eslam Elnikety]

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(-)

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;
     }
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

[Xen-devel] [PATCH v1 4/4] x86/microcode: use const qualifier for microcode buffer

[Eslam Elnikety]

The buffer holding the microcode bits should be marked as const.

Signed-off-by: Eslam Elnikety <elnikety@amazon.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/arch/x86/microcode.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index a662a7f438..0639551173 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -88,7 +88,7 @@ static enum {
  * memory.
  */
 struct ucode_mod_blob {
-    void *data;
+    const void *data;
     size_t size;
 };
 
@@ -753,7 +753,7 @@ int microcode_update_one(bool start_update)
 int __init early_microcode_update_cpu(void)
 {
     int rc = 0;
-    void *data = NULL;
+    const void *data = NULL;
     size_t len;
     struct microcode_patch *patch;
 
-- 
2.17.1


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 1/4] x86/microcode: Improve documentation for ucode=

[Jan Beulich]

On 22.01.2020 23:30, Eslam Elnikety wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2134,7 +2134,12 @@ logic applies:
>  ### ucode (x86)
>  > `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
>  
> -Specify how and where to find CPU microcode update blob.
> +    Applicability: x86

With this, the (x86) in the section title should go away, I think.

> +    Default: `nmi`
> +
> +Controls for CPU microcode loading. For early loading, this parameter can
> +specify how and where to find the microcode update blob. For late loading,
> +this parameter specifies if the update happens within a NMI handler.
>  
>  'integer' specifies the CPU microcode update blob module index. When positive,
>  this specifies the n-th module (in the GrUB entry, zero based) to be used
> @@ -2152,6 +2157,7 @@ image that contains microcode. Depending on the platform the blob with the
>  microcode in the cpio name space must be:
>    - on Intel: kernel/x86/microcode/GenuineIntel.bin
>    - on AMD  : kernel/x86/microcode/AuthenticAMD.bin
> +If EFI boot, the `ucode=<filename>` config takes precendence over `scan`.

"In EFI boot" is wrong, isn't it? This is for xen.efi only aiui.
Also there's a stray 'n' in "precedence".

All could be taken care of while committing, I guess, so with
this
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 3/4] x86/microcode: avoid unnecessary xmalloc/memcpy of ucode data

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Eslam Elnikety]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [Xen-devel] [PATCH v1 2/4] x86/microcode: Improve parsing for ucode=

[Jan Beulich]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel