We can remove the per-cpu cache field in struct ucode_cpu_info since
it has been replaced by a global cache. It would leads to only one field
remaining in ucode_cpu_info. Then, this struct is removed and the
remaining field (cpu signature) is stored in per-cpu area.

Also remove 'microcode_resume_match' from microcode_ops because the
check is done in find_patch(). The cpu status notifier is also
removed. It was used to free the "mc" field to avoid memory leak.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
Changes in v6:
 - remove the whole struct ucode_cpu_info instead of the per-cpu cache
 in it.
---
 xen/arch/x86/apic.c             |   2 +-
 xen/arch/x86/microcode.c        |  91 +++---------------------------------
 xen/arch/x86/microcode_amd.c    | 100 +++++-----------------------------------
 xen/arch/x86/microcode_intel.c  |  33 ++++---------
 xen/arch/x86/spec_ctrl.c        |   2 +-
 xen/include/asm-x86/microcode.h |  13 +-----
 6 files changed, 30 insertions(+), 211 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index fafc0bd..d216455 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1188,7 +1188,7 @@ static void __init check_deadline_errata(void)
     else
         rev = (unsigned long)m->driver_data;
 
-    if ( this_cpu(ucode_cpu_info).cpu_sig.rev >= rev )
+    if ( this_cpu(cpu_sig).rev >= rev )
         return;
 
     setup_clear_cpu_cap(X86_FEATURE_TSC_DEADLINE);
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index cff86a9..0c01dfa 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -187,7 +187,7 @@ const struct microcode_ops *microcode_ops;
 
 static DEFINE_SPINLOCK(microcode_mutex);
 
-DEFINE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
+DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
 
 struct microcode_info {
     unsigned int cpu;
@@ -196,70 +196,19 @@ struct microcode_info {
     char buffer[1];
 };
 
-static void __microcode_fini_cpu(unsigned int cpu)
-{
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-
-    xfree(uci->mc.mc_valid);
-    memset(uci, 0, sizeof(*uci));
-}
-
-static void microcode_fini_cpu(unsigned int cpu)
-{
-    spin_lock(&microcode_mutex);
-    __microcode_fini_cpu(cpu);
-    spin_unlock(&microcode_mutex);
-}
-
 int microcode_resume_cpu(unsigned int cpu)
 {
     int err;
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    struct cpu_signature nsig;
-    unsigned int cpu2;
+    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
 
     if ( !microcode_ops )
         return 0;
 
     spin_lock(&microcode_mutex);
 
-    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
-    if ( err )
-    {
-        __microcode_fini_cpu(cpu);
-        spin_unlock(&microcode_mutex);
-        return err;
-    }
-
-    if ( uci->mc.mc_valid )
-    {
-        err = microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid);
-        if ( err >= 0 )
-        {
-            if ( err )
-                err = microcode_ops->apply_microcode(cpu);
-            spin_unlock(&microcode_mutex);
-            return err;
-        }
-    }
-
-    nsig = uci->cpu_sig;
-    __microcode_fini_cpu(cpu);
-    uci->cpu_sig = nsig;
-
-    err = -EIO;
-    for_each_online_cpu ( cpu2 )
-    {
-        uci = &per_cpu(ucode_cpu_info, cpu2);
-        if ( uci->mc.mc_valid &&
-             microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid) > 0 )
-        {
-            err = microcode_ops->apply_microcode(cpu);
-            break;
-        }
-    }
-
-    __microcode_fini_cpu(cpu);
+    err = microcode_ops->collect_cpu_info(cpu, sig);
+    if ( likely(!err) )
+        err = microcode_ops->apply_microcode(cpu);
     spin_unlock(&microcode_mutex);
 
     return err;
@@ -302,16 +251,13 @@ static int microcode_update_cpu(const void *buf, size_t size)
 {
     int err;
     unsigned int cpu = smp_processor_id();
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
 
     spin_lock(&microcode_mutex);
 
-    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
+    err = microcode_ops->collect_cpu_info(cpu, sig);
     if ( likely(!err) )
         err = microcode_ops->cpu_request_microcode(cpu, buf, size);
-    else
-        __microcode_fini_cpu(cpu);
-
     spin_unlock(&microcode_mutex);
 
     return err;
@@ -398,25 +344,6 @@ static int __init microcode_init(void)
 }
 __initcall(microcode_init);
 
-static int microcode_percpu_callback(
-    struct notifier_block *nfb, unsigned long action, void *hcpu)
-{
-    unsigned int cpu = (unsigned long)hcpu;
-
-    switch ( action )
-    {
-    case CPU_DEAD:
-        microcode_fini_cpu(cpu);
-        break;
-    }
-
-    return NOTIFY_DONE;
-}
-
-static struct notifier_block microcode_percpu_nfb = {
-    .notifier_call = microcode_percpu_callback,
-};
-
 int __init early_microcode_update_cpu(bool start_update)
 {
     int rc = 0;
@@ -460,12 +387,8 @@ int __init early_microcode_init(void)
         return rc;
 
     if ( microcode_ops )
-    {
         if ( ucode_mod.mod_end || ucode_blob.size )
             rc = early_microcode_update_cpu(true);
 
-        register_cpu_notifier(&microcode_percpu_nfb);
-    }
-
     return rc;
 }
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 1f05899..93af2c9 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -155,7 +155,7 @@ static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
 static bool_t microcode_fits(const struct microcode_amd *mc_amd,
                              unsigned int cpu)
 {
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_header_amd *mc_header = mc_amd->mpb;
     const struct equiv_cpu_entry *equiv_cpu_table = mc_amd->equiv_cpu_table;
     unsigned int current_cpu_id;
@@ -178,14 +178,14 @@ static bool_t microcode_fits(const struct microcode_amd *mc_amd,
         return 0;
     }
 
-    if ( mc_header->patch_id <= uci->cpu_sig.rev )
+    if ( mc_header->patch_id <= sig->rev )
     {
         pr_debug("microcode: patch is already at required level or greater.\n");
         return 0;
     }
 
     pr_debug("microcode: CPU%d found a matching microcode update with version %#x (current=%#x)\n",
-             cpu, mc_header->patch_id, uci->cpu_sig.rev);
+             cpu, mc_header->patch_id, sig->rev);
 
     return 1;
 }
@@ -256,9 +256,9 @@ static enum microcode_match_result compare_patch(
 static int apply_microcode(unsigned int cpu)
 {
     unsigned long flags;
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     uint32_t rev;
     int hw_err;
+    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_header_amd *hdr;
     const struct microcode_patch *patch = microcode_get_cache();
 
@@ -294,9 +294,9 @@ static int apply_microcode(unsigned int cpu)
     }
 
     printk(KERN_WARNING "microcode: CPU%d updated from revision %#x to %#x\n",
-           cpu, uci->cpu_sig.rev, hdr->patch_id);
+           cpu, sig->rev, hdr->patch_id);
 
-    uci->cpu_sig.rev = rev;
+    sig->rev = rev;
 
     return 0;
 }
@@ -442,14 +442,14 @@ static bool_t check_final_patch_levels(unsigned int cpu)
      * any of the 'final_levels', then we should not update the microcode
      * patch on the cpu as system will hang otherwise.
      */
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     unsigned int i;
 
     if ( boot_cpu_data.x86 != 0x10 )
         return 0;
 
     for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
-        if ( uci->cpu_sig.rev == final_levels[i] )
+        if ( sig->rev == final_levels[i] )
             return 1;
 
     return 0;
@@ -458,13 +458,12 @@ static bool_t check_final_patch_levels(unsigned int cpu)
 static int cpu_request_microcode(unsigned int cpu, const void *buf,
                                  size_t bufsize)
 {
-    struct microcode_amd *mc_amd, *mc_old;
+    struct microcode_amd *mc_amd;
     size_t offset = 0;
-    size_t last_offset, applied_offset = 0;
-    int error = 0, save_error = 1;
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    int error = 0;
     unsigned int current_cpu_id;
     unsigned int equiv_cpu_id;
+    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
 
     /* We should bind the task to the CPU */
     BUG_ON(cpu != raw_smp_processor_id());
@@ -533,7 +532,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
         {
             printk(KERN_ERR "microcode: CPU%d incorrect or corrupt container file\n"
                    "microcode: Failed to update patch level. "
-                   "Current lvl:%#x\n", cpu, uci->cpu_sig.rev);
+                   "Current lvl:%#x\n", cpu, sig->rev);
             break;
         }
     }
@@ -544,17 +543,12 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
         goto out;
     }
 
-    mc_old = uci->mc.mc_amd;
-    /* implicitely validates uci->mc.mc_valid */
-    uci->mc.mc_amd = mc_amd;
-
     /*
      * It's possible the data file has multiple matching ucode,
      * lets keep searching till the latest version
      */
     mc_amd->mpb = NULL;
     mc_amd->mpb_size = 0;
-    last_offset = offset;
     while ( (error = get_ucode_from_buffer_amd(mc_amd, buf, bufsize,
                                                &offset)) == 0 )
     {
@@ -576,11 +570,8 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
             error = apply_microcode(cpu);
             if ( error )
                 break;
-            applied_offset = last_offset;
         }
 
-        last_offset = offset;
-
         if ( offset >= bufsize )
             break;
 
@@ -609,26 +600,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
             break;
     }
 
-    /* On success keep the microcode patch for
-     * re-apply on resume.
-     */
-    if ( applied_offset )
-    {
-        save_error = get_ucode_from_buffer_amd(
-            mc_amd, buf, bufsize, &applied_offset);
-
-        if ( save_error )
-            error = save_error;
-    }
-
-    if ( save_error )
-    {
-        xfree(mc_amd);
-        uci->mc.mc_amd = mc_old;
-    }
-    else
-        xfree(mc_old);
-
   out:
 #if CONFIG_HVM
     svm_host_osvw_init();
@@ -643,52 +614,6 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
     return error;
 }
 
-static int microcode_resume_match(unsigned int cpu, const void *mc)
-{
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
-    struct microcode_amd *mc_amd = uci->mc.mc_amd;
-    const struct microcode_amd *src = mc;
-
-    if ( !microcode_fits(src, cpu) )
-        return 0;
-
-    if ( src != mc_amd )
-    {
-        if ( mc_amd )
-        {
-            xfree(mc_amd->equiv_cpu_table);
-            xfree(mc_amd->mpb);
-            xfree(mc_amd);
-        }
-
-        mc_amd = xmalloc(struct microcode_amd);
-        uci->mc.mc_amd = mc_amd;
-        if ( !mc_amd )
-            return -ENOMEM;
-        mc_amd->equiv_cpu_table = xmalloc_bytes(src->equiv_cpu_table_size);
-        if ( !mc_amd->equiv_cpu_table )
-            goto err1;
-        mc_amd->mpb = xmalloc_bytes(src->mpb_size);
-        if ( !mc_amd->mpb )
-            goto err2;
-
-        mc_amd->equiv_cpu_table_size = src->equiv_cpu_table_size;
-        mc_amd->mpb_size = src->mpb_size;
-        memcpy(mc_amd->mpb, src->mpb, src->mpb_size);
-        memcpy(mc_amd->equiv_cpu_table, src->equiv_cpu_table,
-               src->equiv_cpu_table_size);
-    }
-
-    return 1;
-
-err2:
-    xfree(mc_amd->equiv_cpu_table);
-err1:
-    xfree(mc_amd);
-    uci->mc.mc_amd = NULL;
-    return -ENOMEM;
-}
-
 static int start_update(void)
 {
 #if CONFIG_HVM
@@ -708,7 +633,6 @@ static int start_update(void)
 }
 
 static const struct microcode_ops microcode_amd_ops = {
-    .microcode_resume_match           = microcode_resume_match,
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index d3405a0..bf6497f 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -250,13 +250,13 @@ static int microcode_sanity_check(void *mc)
 
 static bool match_cpu(const struct microcode_patch *patch)
 {
-    const struct ucode_cpu_info *uci = &this_cpu(ucode_cpu_info);
+    const struct cpu_signature *sig = &this_cpu(cpu_sig);
 
     if ( !patch )
         return false;
 
-    return microcode_update_match(&patch->mc_intel->hdr, uci->cpu_sig.sig,
-                                uci->cpu_sig.pf, uci->cpu_sig.rev) == NEW_UCODE;
+    return microcode_update_match(&patch->mc_intel->hdr,
+                                  sig->sig, sig->pf, sig->rev) == NEW_UCODE;
 }
 
 static void free_patch(struct microcode_patch *patch)
@@ -281,7 +281,6 @@ static enum microcode_match_result compare_patch(
  */
 static int get_matching_microcode(const void *mc, unsigned int cpu)
 {
-    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
     const struct microcode_header_intel *mc_header = mc;
     unsigned long total_size = get_totalsize(mc_header);
     void *new_mc = xmalloc_bytes(total_size);
@@ -308,17 +307,7 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
 
     pr_debug("microcode: CPU%d found a matching microcode update with"
              " version %#x (current=%#x)\n",
-             cpu, mc_header->rev, uci->cpu_sig.rev);
-    new_mc = xmalloc_bytes(total_size);
-    if ( new_mc == NULL )
-    {
-        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
-        return -ENOMEM;
-    }
-
-    memcpy(new_mc, mc, total_size);
-    xfree(uci->mc.mc_intel);
-    uci->mc.mc_intel = new_mc;
+             cpu, mc_header->rev, this_cpu(cpu_sig).rev);
 
     return 1;
 }
@@ -329,7 +318,7 @@ static int apply_microcode(unsigned int cpu)
     uint64_t msr_content;
     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 cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_intel *mc_intel;
     const struct microcode_patch *patch = microcode_get_cache();
 
@@ -360,16 +349,16 @@ static int apply_microcode(unsigned int cpu)
     {
         printk(KERN_ERR "microcode: CPU%d update from revision "
                "%#x to %#x failed. Resulting revision is %#x.\n", cpu_num,
-               uci->cpu_sig.rev, mc_intel->hdr.rev, val[1]);
+               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],
+           cpu_num, sig->rev, val[1],
            mc_intel->hdr.year,
            mc_intel->hdr.month,
            mc_intel->hdr.day);
-    uci->cpu_sig.rev = val[1];
+    sig->rev = val[1];
 
     return 0;
 }
@@ -440,13 +429,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
     return error;
 }
 
-static int microcode_resume_match(unsigned int cpu, const void *mc)
-{
-    return get_matching_microcode(mc, cpu);
-}
-
 static const struct microcode_ops microcode_intel_ops = {
-    .microcode_resume_match           = microcode_resume_match,
     .cpu_request_microcode            = cpu_request_microcode,
     .collect_cpu_info                 = collect_cpu_info,
     .apply_microcode                  = apply_microcode,
diff --git a/xen/arch/x86/spec_ctrl.c b/xen/arch/x86/spec_ctrl.c
index 5d98cac..43128c3 100644
--- a/xen/arch/x86/spec_ctrl.c
+++ b/xen/arch/x86/spec_ctrl.c
@@ -436,7 +436,7 @@ static bool __init check_smt_enabled(void)
 /* Calculate whether Retpoline is known-safe on this CPU. */
 static bool __init retpoline_safe(uint64_t caps)
 {
-    unsigned int ucode_rev = this_cpu(ucode_cpu_info).cpu_sig.rev;
+    unsigned int ucode_rev = this_cpu(cpu_sig).rev;
 
     if ( boot_cpu_data.x86_vendor == X86_VENDOR_AMD )
         return true;
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index 6541c58..f2ac509 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -10,7 +10,6 @@ enum microcode_match_result {
 };
 
 struct cpu_signature;
-struct ucode_cpu_info;
 
 struct microcode_patch {
     union {
@@ -20,7 +19,6 @@ struct microcode_patch {
 };
 
 struct microcode_ops {
-    int (*microcode_resume_match)(unsigned int cpu, const void *mc);
     int (*cpu_request_microcode)(unsigned int cpu, const void *buf,
                                  size_t size);
     int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig);
@@ -39,16 +37,7 @@ struct cpu_signature {
     unsigned int rev;
 };
 
-struct ucode_cpu_info {
-    struct cpu_signature cpu_sig;
-    union {
-        struct microcode_intel *mc_intel;
-        struct microcode_amd *mc_amd;
-        void *mc_valid;
-    } mc;
-};
-
-DECLARE_PER_CPU(struct ucode_cpu_info, ucode_cpu_info);
+DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 extern const struct microcode_ops *microcode_ops;
 
 const struct microcode_patch *microcode_get_cache(void);
-- 
1.8.3.1


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

>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
> We can remove the per-cpu cache field in struct ucode_cpu_info since
> it has been replaced by a global cache. It would leads to only one field
> remaining in ucode_cpu_info. Then, this struct is removed and the
> remaining field (cpu signature) is stored in per-cpu area.
> 
> Also remove 'microcode_resume_match' from microcode_ops because the
> check is done in find_patch(). The cpu status notifier is also
> removed. It was used to free the "mc" field to avoid memory leak.

There's no find_patch() function anymore afaics. And I also think this
should be a separate patch. The above isn't enough imo to justify ...

>  int microcode_resume_cpu(unsigned int cpu)
>  {
>      int err;
> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> -    struct cpu_signature nsig;
> -    unsigned int cpu2;
> +    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>  
>      if ( !microcode_ops )
>          return 0;
>  
>      spin_lock(&microcode_mutex);
>  
> -    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
> -    if ( err )
> -    {
> -        __microcode_fini_cpu(cpu);
> -        spin_unlock(&microcode_mutex);
> -        return err;
> -    }
> -
> -    if ( uci->mc.mc_valid )
> -    {
> -        err = microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid);
> -        if ( err >= 0 )
> -        {
> -            if ( err )
> -                err = microcode_ops->apply_microcode(cpu);
> -            spin_unlock(&microcode_mutex);
> -            return err;
> -        }
> -    }
> -
> -    nsig = uci->cpu_sig;
> -    __microcode_fini_cpu(cpu);
> -    uci->cpu_sig = nsig;
> -
> -    err = -EIO;
> -    for_each_online_cpu ( cpu2 )
> -    {
> -        uci = &per_cpu(ucode_cpu_info, cpu2);
> -        if ( uci->mc.mc_valid &&
> -             microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid) > 0 )
> -        {
> -            err = microcode_ops->apply_microcode(cpu);
> -            break;
> -        }
> -    }

... in particular the removal of this loop, the more that both the
loop and the code ahead of it also call ->apply_microcode().

> @@ -281,7 +281,6 @@ static enum microcode_match_result compare_patch(
>   */
>  static int get_matching_microcode(const void *mc, unsigned int cpu)
>  {
> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);

Note how this was using "cpu".

> @@ -308,17 +307,7 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
>  
>      pr_debug("microcode: CPU%d found a matching microcode update with"
>               " version %#x (current=%#x)\n",
> -             cpu, mc_header->rev, uci->cpu_sig.rev);
> -    new_mc = xmalloc_bytes(total_size);
> -    if ( new_mc == NULL )
> -    {
> -        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
> -        return -ENOMEM;
> -    }
> -
> -    memcpy(new_mc, mc, total_size);
> -    xfree(uci->mc.mc_intel);
> -    uci->mc.mc_intel = new_mc;
> +             cpu, mc_header->rev, this_cpu(cpu_sig).rev);

Why "this_cpu()" here?

Jan



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

On Tue, Jun 04, 2019 at 09:13:46AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
>> We can remove the per-cpu cache field in struct ucode_cpu_info since
>> it has been replaced by a global cache. It would leads to only one field
>> remaining in ucode_cpu_info. Then, this struct is removed and the
>> remaining field (cpu signature) is stored in per-cpu area.
>> 
>> Also remove 'microcode_resume_match' from microcode_ops because the
>> check is done in find_patch(). The cpu status notifier is also
>> removed. It was used to free the "mc" field to avoid memory leak.
>
>There's no find_patch() function anymore afaics. And I also think this
>should be a separate patch. The above isn't enough imo to justify ...
>
>>  int microcode_resume_cpu(unsigned int cpu)
>>  {
>>      int err;
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> -    struct cpu_signature nsig;
>> -    unsigned int cpu2;
>> +    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>>  
>>      if ( !microcode_ops )
>>          return 0;
>>  
>>      spin_lock(&microcode_mutex);
>>  
>> -    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> -    if ( err )
>> -    {
>> -        __microcode_fini_cpu(cpu);
>> -        spin_unlock(&microcode_mutex);
>> -        return err;
>> -    }
>> -
>> -    if ( uci->mc.mc_valid )
>> -    {
>> -        err = microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid);
>> -        if ( err >= 0 )
>> -        {
>> -            if ( err )
>> -                err = microcode_ops->apply_microcode(cpu);
>> -            spin_unlock(&microcode_mutex);
>> -            return err;
>> -        }
>> -    }
>> -
>> -    nsig = uci->cpu_sig;
>> -    __microcode_fini_cpu(cpu);
>> -    uci->cpu_sig = nsig;
>> -
>> -    err = -EIO;
>> -    for_each_online_cpu ( cpu2 )
>> -    {
>> -        uci = &per_cpu(ucode_cpu_info, cpu2);
>> -        if ( uci->mc.mc_valid &&
>> -             microcode_ops->microcode_resume_match(cpu, uci->mc.mc_valid) > 0 )
>> -        {
>> -            err = microcode_ops->apply_microcode(cpu);
>> -            break;
>> -        }
>> -    }
>
>... in particular the removal of this loop, the more that both the
>loop and the code ahead of it also call ->apply_microcode().

Ok. Will split it out from this patch and refine the patch description.

Basically, this function tries best to find a suitable patch from the
per-cpu cache and loads it. Currently, the per-cpu cache is replaced by
the global cache, and ->apply_microcode() loads the global cache rather
then the per-cpu cache. Hence, a simple invocation of ->apply_microcode()
is enough to apply the global cache during CPU hotplug or resuming from
hibernation.

>
>> @@ -281,7 +281,6 @@ static enum microcode_match_result compare_patch(
>>   */
>>  static int get_matching_microcode(const void *mc, unsigned int cpu)
>>  {
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>
>Note how this was using "cpu".
>
>> @@ -308,17 +307,7 @@ static int get_matching_microcode(const void *mc, unsigned int cpu)
>>  
>>      pr_debug("microcode: CPU%d found a matching microcode update with"
>>               " version %#x (current=%#x)\n",
>> -             cpu, mc_header->rev, uci->cpu_sig.rev);
>> -    new_mc = xmalloc_bytes(total_size);
>> -    if ( new_mc == NULL )
>> -    {
>> -        printk(KERN_ERR "microcode: error! Can not allocate memory\n");
>> -        return -ENOMEM;
>> -    }
>> -
>> -    memcpy(new_mc, mc, total_size);
>> -    xfree(uci->mc.mc_intel);
>> -    uci->mc.mc_intel = new_mc;
>> +             cpu, mc_header->rev, this_cpu(cpu_sig).rev);
>
>Why "this_cpu()" here?

It should be a part of next patch.

Thanks
Chao

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