Re: [PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Jan Beulich <jbeulich@xxxxxxxx>]

On 25.07.2024 10:27, Fouad Hilly wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2650,7 +2650,7 @@ performance.
>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>  
>  ### ucode
> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
> +> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
>  
>      Applicability: x86
>      Default: `nmi`
> @@ -2682,11 +2682,6 @@ precedence over `scan`.
>  stop_machine context. In NMI handler, even NMIs are blocked, which is
>  considered safer. The default value is `true`.
>  
> -'allow-same' alters the default acceptance policy for new microcode to permit
> -trying to reload the same version.  Many CPUs will actually reload microcode
> -of the same version, and this allows for easy testing of the late microcode
> -loading path.

The removal of a command line (sub)option should come with a CHANGELOG.md
entry.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -90,6 +90,11 @@ struct ucode_mod_blob {
>      size_t size;
>  };
>  
> +struct patch_with_flags {
> +    unsigned int flags;
> +    struct microcode_patch *patch;

Pointer-to-const? If the const was omitted here just because of
microcode_free_patch(), then I think the issue should be taken care
of there.

> @@ -237,7 +238,11 @@ static DEFINE_PER_CPU(int, loading_err);
>   */
>  static cpumask_t cpu_callin_map;
>  static atomic_t cpu_out, cpu_updated;
> -static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
> +static struct patch_with_flags nmi_patch_with_flags =

Could the variable name perhaps continue to be "nmi_patch"? Or be
simply "nmi_arg" or some such?

> +{
> +    .flags  = 0,

Nit: This isn't really needed.

> @@ -379,7 +386,8 @@ static int secondary_nmi_work(void)
>      return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
>  }
>  
> -static int primary_thread_work(const struct microcode_patch *patch)
> +static int primary_thread_work(struct microcode_patch *patch,
> +                               unsigned int flags)
>  {

Why is this change needed?

> @@ -446,7 +455,8 @@ static int secondary_thread_fn(void)
>      return this_cpu(loading_err);
>  }
>  
> -static int primary_thread_fn(const struct microcode_patch *patch)
> +static int primary_thread_fn(struct microcode_patch *patch,
> +                             unsigned int flags)

Same here.

Jan