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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/05/2024 4:29 pm, Fouad Hilly wrote:
> Pass ucode_force to common micorocde checks and utilize it to allow for 
> microcode downgrade
> or reapply the same version of the microcode.
> Update low level Intel and AMD to check for valid signature only. Any version 
> checks is done
> at core.c.
> While adding ucode_force, opt_ucode_allow_same was removed.
> Remove opt_ucode_allow_same from documentation.
>
> Signed-off-by: Fouad Hilly <fouad.hilly@xxxxxxxxx>
> ---
> [4]
> 1- As opt_ucode_allow_same is not required anymore, it has been removed while 
> introducing ucode_force.
> 2- Apply the changes for both AMD and Intel.
> 3- Remove the mention of opt_ucode_allow_same from documentation.
> ---
>  docs/misc/xen-command-line.pandoc    | 7 +------
>  xen/arch/x86/cpu/microcode/amd.c     | 7 -------
>  xen/arch/x86/cpu/microcode/core.c    | 9 +++------
>  xen/arch/x86/cpu/microcode/intel.c   | 4 ----
>  xen/arch/x86/cpu/microcode/private.h | 2 --
>  5 files changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/docs/misc/xen-command-line.pandoc 
> b/docs/misc/xen-command-line.pandoc
> index 1dea7431fab6..a42ce1039fed 100644
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2648,7 +2648,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`
> @@ -2680,11 +2680,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.
> -
>  ### unrestricted_guest (Intel)
>  > `= <boolean>`
>  
> diff --git a/xen/arch/x86/cpu/microcode/amd.c 
> b/xen/arch/x86/cpu/microcode/amd.c
> index f76a563c8b84..4bcc79f1ab2d 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -225,13 +225,6 @@ static int cf_check apply_microcode(const struct 
> microcode_patch *patch)
>      if ( result == MIS_UCODE )
>          return -EINVAL;
>  
> -    /*
> -     * Allow application of the same revision to pick up SMT-specific changes
> -     * even if the revision of the other SMT thread is already up-to-date.
> -     */
> -    if ( result == OLD_UCODE )
> -        return -EEXIST;
> -

I'm afraid that because of the observation leading to 977d98e67c2e, I
see no option other than to plumb the force flag down into
apply_microcode().

This check cannot be deleted unconditionally, or we'll try downgrading
microcode even without the force flag being passed.

Unless we can fix the cacheing layer to not treat "I didn't load ucode
at boot" as "no idea of the symmetry of the system".

I'm unsure which of these two is going to be less ugly...

>      if ( check_final_patch_levels(sig) )
>      {
>          printk(XENLOG_ERR
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index 8a9e744489b9..fc8ad8cfdd76 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -100,8 +100,6 @@ static bool __initdata ucode_scan;
>  /* By default, ucode loading is done in NMI handler */
>  static bool ucode_in_nmi = true;
>  
> -bool __read_mostly opt_ucode_allow_same;
> -
>  /* Protected by microcode_mutex */
>  static struct microcode_patch *microcode_cache;
>  
> @@ -128,8 +126,6 @@ static int __init cf_check parse_ucode(const char *s)
>  
>          if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
>              ucode_in_nmi = val;
> -        else if ( (val = parse_boolean("allow-same", s, ss)) >= 0 )
> -            opt_ucode_allow_same = val;
>          else if ( !ucode_mod_forced ) /* Not forced by EFI */
>          {
>              if ( (val = parse_boolean("scan", s, ss)) >= 0 )
> @@ -583,6 +579,7 @@ static long cf_check microcode_update_helper(void *data)
>      struct ucode_buf *buffer = data;
>      unsigned int cpu, updated;
>      struct microcode_patch *patch;
> +    bool ucode_force = buffer->flags == XENPF_UCODE_FORCE;
>  
>      /* cpu_online_map must not change during update */
>      if ( !get_cpu_maps() )
> @@ -636,12 +633,12 @@ static long cf_check microcode_update_helper(void *data)
>                                    microcode_cache);
>  
>          if ( result != NEW_UCODE &&
> -             !(opt_ucode_allow_same && result == SAME_UCODE) )
> +             (!ucode_force || (result & ~SAME_UCODE)) )

What is "result & ~SAME_UCODE" trying to do?

~Andrew