RE: [PATCH for-4.14] x86/ucode: Fix errors with start/end_update()

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> Sent: 01 June 2020 15:58
> To: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>; Jan Beulich 
> <JBeulich@xxxxxxxx>; Wei Liu <wl@xxxxxxx>;
> Roger Pau Monné <roger.pau@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>
> Subject: [PATCH for-4.14] x86/ucode: Fix errors with start/end_update()
> 
> c/s 9267a439c "x86/ucode: Document the behaviour of the microcode_ops hooks"
> identified several poor behaviours of the start_update()/end_update_percpu()
> hooks.
> 
> AMD have subsequently confirmed that OSVW don't, and are not expected to,
> change across a microcode load, rendering all of this complexity unecessary.
> 
> Instead of fixing up the logic to not leave the OSVW state reset in a number
> of corner cases, delete the logic entirely.  This in turn allows for the
> removal of the poorly-named 'start_update' parameter to
> microcode_update_one().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Paul Durrant <paul@xxxxxxx>
> 
> This wants backporting to 4.13, hence considering for 4.14 at this point.

Release-acked-by: Paul Durrant <paul@xxxxxxx>

> ---
>  xen/arch/x86/acpi/power.c            |  2 +-
>  xen/arch/x86/cpu/microcode/amd.c     | 17 -----------------
>  xen/arch/x86/cpu/microcode/core.c    | 33 +++------------------------------
>  xen/arch/x86/cpu/microcode/private.h | 14 --------------
>  xen/arch/x86/smpboot.c               |  2 +-
>  xen/include/asm-x86/microcode.h      |  2 +-
>  6 files changed, 6 insertions(+), 64 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 0cda362045..dfffe08e18 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -287,7 +287,7 @@ static int enter_state(u32 state)
>      console_end_sync();
>      watchdog_enable();
> 
> -    microcode_update_one(true);
> +    microcode_update_one();
> 
>      if ( !recheck_cpu_features(0) )
>          panic("Missing previously available feature(s)\n");
> diff --git a/xen/arch/x86/cpu/microcode/amd.c 
> b/xen/arch/x86/cpu/microcode/amd.c
> index 3f0969e70d..11e24637e7 100644
> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -395,26 +395,9 @@ static struct microcode_patch 
> *cpu_request_microcode(const void *buf, size_t siz
>      return patch;
>  }
> 
> -#ifdef CONFIG_HVM
> -static int start_update(void)
> -{
> -    /*
> -     * svm_host_osvw_init() will be called on each cpu by calling 
> '.end_update'
> -     * in common code.
> -     */
> -    svm_host_osvw_reset();
> -
> -    return 0;
> -}
> -#endif
> -
>  const struct microcode_ops amd_ucode_ops = {
>      .cpu_request_microcode            = cpu_request_microcode,
>      .collect_cpu_info                 = collect_cpu_info,
>      .apply_microcode                  = apply_microcode,
> -#ifdef CONFIG_HVM
> -    .start_update                     = start_update,
> -    .end_update_percpu                = svm_host_osvw_init,
> -#endif
>      .compare_patch                    = compare_patch,
>  };
> diff --git a/xen/arch/x86/cpu/microcode/core.c 
> b/xen/arch/x86/cpu/microcode/core.c
> index d879d28787..18ebc07b13 100644
> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -546,9 +546,6 @@ static int do_microcode_update(void *patch)
>      else
>          ret = secondary_thread_fn();
> 
> -    if ( microcode_ops->end_update_percpu )
> -        microcode_ops->end_update_percpu();
> -
>      return ret;
>  }
> 
> @@ -620,16 +617,6 @@ static long microcode_update_helper(void *data)
>      }
>      spin_unlock(&microcode_mutex);
> 
> -    if ( microcode_ops->start_update )
> -    {
> -        ret = microcode_ops->start_update();
> -        if ( ret )
> -        {
> -            microcode_free_patch(patch);
> -            goto put;
> -        }
> -    }
> -
>      cpumask_clear(&cpu_callin_map);
>      atomic_set(&cpu_out, 0);
>      atomic_set(&cpu_updated, 0);
> @@ -728,28 +715,14 @@ static int __init microcode_init(void)
>  __initcall(microcode_init);
> 
>  /* Load a cached update to current cpu */
> -int microcode_update_one(bool start_update)
> +int microcode_update_one(void)
>  {
> -    int err;
> -
>      if ( !microcode_ops )
>          return -EOPNOTSUPP;
> 
>      microcode_ops->collect_cpu_info();
> 
> -    if ( start_update && microcode_ops->start_update )
> -    {
> -        err = microcode_ops->start_update();
> -        if ( err )
> -            return err;
> -    }
> -
> -    err = microcode_update_cpu(NULL);
> -
> -    if ( microcode_ops->end_update_percpu )
> -        microcode_ops->end_update_percpu();
> -
> -    return err;
> +    return microcode_update_cpu(NULL);
>  }
> 
>  /* BSP calls this function to parse ucode blob and then apply an update. */
> @@ -790,7 +763,7 @@ static int __init early_microcode_update_cpu(void)
>      spin_unlock(&microcode_mutex);
>      ASSERT(rc);
> 
> -    return microcode_update_one(true);
> +    return microcode_update_one();
>  }
> 
>  int __init early_microcode_init(void)
> diff --git a/xen/arch/x86/cpu/microcode/private.h 
> b/xen/arch/x86/cpu/microcode/private.h
> index dc5c7a81ae..c00ba590d1 100644
> --- a/xen/arch/x86/cpu/microcode/private.h
> +++ b/xen/arch/x86/cpu/microcode/private.h
> @@ -46,20 +46,6 @@ struct microcode_ops {
>      int (*apply_microcode)(const struct microcode_patch *patch);
> 
>      /*
> -     * Optional.  If provided and applicable to the specific update attempt,
> -     * is run once by the initiating CPU.  Returning an error will abort the
> -     * load attempt.
> -     */
> -    int (*start_update)(void);
> -
> -    /*
> -     * Optional.  If provided, called on every CPU which completes a 
> microcode
> -     * load.  May be called in the case of some errors, and not others.  May
> -     * be called even if start_update() wasn't.
> -     */
> -    void (*end_update_percpu)(void);
> -
> -    /*
>       * Given two patches, are they both applicable to the current CPU, and is
>       * new a higher revision than old?
>       */
> diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
> index 13b3dade9c..f878a00760 100644
> --- a/xen/arch/x86/smpboot.c
> +++ b/xen/arch/x86/smpboot.c
> @@ -369,7 +369,7 @@ void start_secondary(void *unused)
> 
>      initialize_cpu_data(cpu);
> 
> -    microcode_update_one(false);
> +    microcode_update_one();
> 
>      /*
>       * If MSR_SPEC_CTRL is available, apply Xen's default setting and discard
> diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
> index 9da63f992e..3b0234e9fa 100644
> --- a/xen/include/asm-x86/microcode.h
> +++ b/xen/include/asm-x86/microcode.h
> @@ -22,6 +22,6 @@ DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
>  void microcode_set_module(unsigned int idx);
>  int microcode_update(XEN_GUEST_HANDLE(const_void), unsigned long len);
>  int early_microcode_init(void);
> -int microcode_update_one(bool start_update);
> +int microcode_update_one(void);
> 
>  #endif /* ASM_X86__MICROCODE_H */
> --
> 2.11.0