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

[Chao Gao <chao.gao@xxxxxxxxx>]

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

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

Will do.

Thanks
Chao

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