[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On 18.12.19 12:49, Jan Beulich wrote:
On 18.12.2019 02:32, Eslam Elnikety wrote:
Decouple the microcode referencing mechanism when using GRUB to that
when using EFI. This allows us to avoid the "unspecified effect" of
using `<integer> | scan` along xen.efi.

I guess "unspecified effect" in the doc was pretty pointless - such
options have been ignored before; in fact ...

With that, Xen can explicitly
ignore those named options when using EFI.

... I don't see things becoming any more explicit (not even parsing
the options was quite explicit to me).

I agree that those options have been ignored so far in the case of EFI. The documentation contradicts that however. The documentation:
* says <integer> has unspecified effect.
* does not mention anything about scan being ignored.

With this patch, it is explicit in code and in documentation that both options are ignored in case of EFI.

As an added benefit,
we get a straightfoward parsing of the ucode parameter.

It's a single if() you eliminate - for me this doesn't make it
meaningfully more straightforward.

As we decouple ucode_mod_idx and ucode_mod_efi_idx, the parsing of the ucode= just follows the pattern "[ <integer> | scan=<bool>, nmi=<bool> ]" and it does not have to cater for corner cases. In either case, if you strongly disagree with the wording, I can drop that sentence.

--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2128,7 +2128,13 @@ 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 or in
+a stop_machine context.

It's always stop_machine context, isn't it? I also don't think this
implementation detail belongs here.

Needs a better wording indeed. Let me know if you have particular suggestions, and I will incorporate in v3.

--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -60,7 +60,7 @@
static module_t __initdata ucode_mod;
  static signed int __initdata ucode_mod_idx;
-static bool_t __initdata ucode_mod_forced;
+static signed int __initdata ucode_mod_efi_idx;

I don't see anything negative be put into here - should be unsigned
int then.

Correct! The type just carried over from the coupling with ucode_mod_idx. Will adjust in v3.

@@ -105,16 +105,10 @@ static struct microcode_patch *microcode_cache;
void __init microcode_set_module(unsigned int idx)
-    ucode_mod_idx = idx;
-    ucode_mod_forced = 1;
+    ucode_mod_efi_idx = idx;

Is it guaranteed (now and forever) that the index passed in is
non-zero? You changes to microcode_grab_module() imply so, but
just looking at the call site of the function I can't convince
myself this is the case. _If_ it is (thought to be) guaranteed,
then I think you at least want to ASSERT() here, perhaps with
a comment.

For x86, it seems we have that guarantee (given that we must have a dom0). Right?

- * 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)
+static int __init parse_ucode_param(const char *s)

Any particular reason for the renaming? The function name was quite
fine imo.

To me, "parse_ucode" is a misnomer.

Thanks for your comments, Jan.

-- Eslam

Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.