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

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Mon, Jul 29, 2024 at 12:30 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:

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.

Will be fixed in v7 

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

This struct is required as is, I initially added a similar struct with const (which was removed in v6).

updated control_thread_fn()

-static int control_thread_fn(const struct microcode_patch *patch)
+static int control_thread_fn(struct microcode_patch *patch,
+                             unsigned int flags)

> @@ -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?

Sure, will keep it as nmi_patch 

> +{
> +    .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?

This will be reverted in v7 

> @@ -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.

 This will be reverted in v7

Jan

Thanks,

Fouad