Re: [Xen-devel] [PATCH v5 3/8] microcode: introduce the global microcode cache

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Jan 28, 2019 at 03:06:45PM +0800, Chao Gao wrote:
> to replace the current per-cpu cache 'uci->mc'.
> 
> Compared to the current per-cpu cache, the benefits of the global
> microcode cache are:
> 1. It reduces the work that need to be done on each CPU. Parsing ucode
> file can be done once on one CPU. Other CPUs needn't parse ucode file.
> Instead, they can find out and load a patch with newer revision from
> the global cache.
> 2. It reduces the memory consumption on a system with many CPU cores.
> 
> Two functions, save_patch() and find_patch() are introduced. The
> former adds one given patch to the global cache. The latter gets
> a newer and matched ucode patch from the global cache.
> 
> Note that I deliberately avoid touching 'uci->mc' as I am going to
> remove it completely in the next patch.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> Changes in v5:
>  - reword the commit description
>  - find_patch() and save_patch() are abstracted into common functions
>    with some hooks for AMD and Intel
> ---
>  xen/arch/x86/microcode.c        | 54 +++++++++++++++++++++++
>  xen/arch/x86/microcode_amd.c    | 94 
> ++++++++++++++++++++++++++++++++++++++---
>  xen/arch/x86/microcode_intel.c  | 71 ++++++++++++++++++++++++++++---
>  xen/include/asm-x86/microcode.h | 13 ++++++
>  4 files changed, 219 insertions(+), 13 deletions(-)
> 
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 4163f50..7d5b769 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -61,6 +61,8 @@ static struct ucode_mod_blob __initdata ucode_blob;
>   */
>  static bool_t __initdata ucode_scan;
>  
> +static LIST_HEAD(microcode_cache);
> +
>  void __init microcode_set_module(unsigned int idx)
>  {
>      ucode_mod_idx = idx;
> @@ -208,6 +210,58 @@ static void microcode_fini_cpu(unsigned int cpu)
>      spin_unlock(&microcode_mutex);
>  }
>  
> +/* Save a ucode patch to the global cache list */
> +bool save_patch(struct microcode_patch *new_patch)

This being a global function likely requires some kind of prefix, I
would suggest microcode_save_patch, the same applies to the find_patch
function below.

> +{
> +    struct microcode_patch *microcode_patch;
> +
> +    list_for_each_entry(microcode_patch, &microcode_cache, list)

I think I'm missing something here, but given the conversation we had
in the previous version of the series [0] I assumed there was only a
single microcode patch that applies to the whole system, and that
there was no need to keep a list?

Because Xen doesn't support running on such mixed systems anyway?

[0] https://lists.xenproject.org/archives/html/xen-devel/2018-12/msg00381.html

> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index 1ed573a..fc35c8d 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -276,18 +324,24 @@ static int apply_microcode(unsigned int cpu)
>      unsigned int val[2];
>      unsigned int cpu_num = raw_smp_processor_id();
>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu_num);
> +    struct microcode_intel *mc_intel;
> +    struct microcode_patch *patch;
>  
>      /* We should bind the task to the CPU */
>      BUG_ON(cpu_num != cpu);
>  
> -    if ( uci->mc.mc_intel == NULL )
> +    patch = find_patch(cpu);
> +    if ( !patch )
>          return -EINVAL;
>  
> +    mc_intel = patch->data;
> +    BUG_ON(!mc_intel);
> +
>      /* serialize access to the physical write to MSR 0x79 */
>      spin_lock_irqsave(&microcode_update_lock, flags);
>  
>      /* write microcode via MSR 0x79 */
> -    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)uci->mc.mc_intel->bits);
> +    wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
>      wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);
>  
>      /* As documented in the SDM: Do a CPUID 1 here */
> @@ -298,19 +352,19 @@ static int apply_microcode(unsigned int cpu)
>      val[1] = (uint32_t)(msr_content >> 32);
>  
>      spin_unlock_irqrestore(&microcode_update_lock, flags);
> -    if ( val[1] != uci->mc.mc_intel->hdr.rev )
> +    if ( val[1] != mc_intel->hdr.rev )
>      {
>          printk(KERN_ERR "microcode: CPU%d update from revision "
>                 "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
> -               uci->cpu_sig.rev, uci->mc.mc_intel->hdr.rev, val[1]);
> +               uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]);
>          return -EIO;
>      }
>      printk(KERN_INFO "microcode: CPU%d updated from revision "
>             "%#x to %#x, date = %04x-%02x-%02x \n",
>             cpu_num, uci->cpu_sig.rev, val[1],
> -           uci->mc.mc_intel->hdr.date & 0xffff,
> -           uci->mc.mc_intel->hdr.date >> 24,
> -           (uci->mc.mc_intel->hdr.date >> 16) & 0xff);
> +           mc_intel->hdr.date & 0xffff,
> +           mc_intel->hdr.date >> 24,
> +           (mc_intel->hdr.date >> 16) & 0xff);

Nit: while here could you make an union of the date field with it's
format, so that you can print it without having to perform this
shifting and masking?

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel