Re: [Xen-devel] [PATCH v6 04/12] microcode: introduce a global cache of ucode patch

[Chao Gao <chao.gao@xxxxxxxxx>]

On Wed, Mar 13, 2019 at 04:36:50PM +0000, Sergey Dyasli wrote:
>On 11/03/2019 07:57, 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 is done once on one CPU. Other CPUs needn't parse ucode file.
>> Instead, they can find out and load the newest patch from the global
>> cache.
>> 2. It reduces the memory consumption on a system with many CPU cores.
>> 
>> Two functions, microcode_save_patch() and microcode_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.
>> All operations on the cache is expected to be done with the
>> 'microcode_mutex' hold.
>> 
>> 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 v6:
>>  - constify local variables and function parameters if possible
>>  - comment that the global cache is protected by 'microcode_mutex'.
>>    and add assertions to catch violations in microcode_{save/find}_patch()
>> 
>> 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        | 60 +++++++++++++++++++++++++++
>>  xen/arch/x86/microcode_amd.c    | 91 
>> +++++++++++++++++++++++++++++++++++++----
>>  xen/arch/x86/microcode_intel.c  | 66 ++++++++++++++++++++++++++----
>>  xen/include/asm-x86/microcode.h | 13 ++++++
>>  4 files changed, 215 insertions(+), 15 deletions(-)

Hi Sergey,

Thanks for your testing.

>
>This needs the following change for the AMD part:
>
>diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>index 4aa8fdca38..358f3c44e3 100644
>--- a/xen/arch/x86/microcode_amd.c
>+++ b/xen/arch/x86/microcode_amd.c
>@@ -249,6 +249,7 @@ static enum microcode_match_result compare_patch(
> static int apply_microcode(void)
> {
>     uint32_t rev;
>+    struct microcode_amd *mc_amd;
>     const struct microcode_header_amd *hdr;
>     const struct microcode_patch *patch;
>     int hw_err;
>@@ -259,7 +260,8 @@ static int apply_microcode(void)
>     if ( patch == NULL )
>         return -EINVAL;
>
>-    hdr = patch->data;
>+    mc_amd = patch->data;
>+    hdr = mc_amd->mpb;
>
>     BUG_ON(local_irq_is_enabled());

Yes. Indeed we need this fix.

>
>
>Another thing to mention is that even with this fix applied, I get
>early ucode update only on CPU0.

In this patch, I checked the return value of microcode_save_patch().
If it failed, the CPU wouldn't update ucode. Here, each cpu will
parse microcode itself and add found patches to cache. Only the first
saving would succeed and I believe it causes the issue you found.

>With the whole series applied, secondary
>CPUs also start doing early ucode update.

I assume that you mean secondary CPUs also invoke .apply_microcode
callback.

This is an expected behavior. On early ucode update, the sibling info isn't
initialized. We don't do anything to prevent secondary cpus (for we haven't
identified them) from updating CPU. Secondary CPUs wouldn't find any
newer patch when calling microcode_find_patch() in apply_microcode().

Thanks
Chao

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