Re: [PATCH v2 1/4] x86/ucode: Enforce invariant about module selection

[Jan Beulich <jbeulich@xxxxxxxx>]

On 06.11.2024 01:39, Andrew Cooper wrote:
> The work to add the `ucode=nmi` cmdline option left a subtle corner case.
> Both scan and an explicit index could be chosen, and we could really find both
> a CPIO archive and a microcode file.
> 
> Worse, because the if/else chains for processing ucode_{blob,mod} are opposite
> ways around in early_microcode_load() and microcode_init_cache(), we can
> genuinely perform early microcode loading from the CPIO archive, then cache
> from the explicit file.
> 
> Therefore, enforce that only one selection method can be active.

While, following my v1 comment on this, I'm okay with it staying like this,
I'm afraid ...

> @@ -109,13 +103,28 @@ static bool ucode_in_nmi = true;
>  /* Protected by microcode_mutex */
>  static const struct microcode_patch *microcode_cache;
>  
> +/*
> + * opt_mod_idx and opt_scan have subtle semantics.
> + *
> + * The cmdline can either identify a module by number (inc -ve 
> back-reference)
> + * containing a raw microcode container, or select scan which instructs Xen 
> to
> + * search all modules for an uncompressed CPIO archive containing a file with
> + * a vendor-dependent name.
> + *
> + * These options do not make sense when combined, so for the benefit of 
> module
> + * location we require that they are not both active together.
> + */

... "do not make sense" is still wrong here. As per your subsequent v1 reply
to my comments, it is merely the case that we do not assign any meaning to
that combination. Yet as outlined we could. With the wording tweaked
accordingly:
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan