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

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

On Tue, Jan 29, 2019 at 12:41:42PM +0800, Chao Gao wrote:
> On Mon, Jan 28, 2019 at 06:39:43PM +0100, Roger Pau Monné wrote:
> >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.
> 
> Will do.
> 
> >
> >> +{
> >> +    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?
> 
> No. As Jan pointed out in [1], we still want to support mixed systems
> which are allowed by Intel and AMD.
> 
> [1] https://lists.xenproject.org/archives/html/xen-devel/2018-11/msg03479.html

Oh thanks, I've missed that reply while reading the previous series.
Then with the comments fixed you can add:

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks, Roger.

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