diff mbox series

[v7,05/10] microcode: remove pointless 'cpu' parameter

Message ID 1558945891-3015-6-git-send-email-chao.gao@intel.com (mailing list archive)
State Superseded
Headers show
Series improve late microcode loading | expand

Commit Message

Chao Gao May 27, 2019, 8:31 a.m. UTC
Some callbacks in microcode_ops or related functions take a cpu
id parameter. But at current call sites, the cpu id parameter is
always equal to current cpu id. Some of them even use an assertion
to guarantee this. Remove this redundent 'cpu' parameter.

Signed-off-by: Chao Gao <chao.gao@intel.com>
---
 xen/arch/x86/acpi/power.c       |  2 +-
 xen/arch/x86/microcode.c        | 12 ++++++------
 xen/arch/x86/microcode_amd.c    | 35 +++++++++++++----------------------
 xen/arch/x86/microcode_intel.c  | 25 +++++++++----------------
 xen/arch/x86/smpboot.c          |  2 +-
 xen/include/asm-x86/microcode.h |  7 +++----
 xen/include/asm-x86/processor.h |  2 +-
 7 files changed, 34 insertions(+), 51 deletions(-)

Comments

Jan Beulich June 4, 2019, 3:29 p.m. UTC | #1
>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -78,8 +78,9 @@ struct mpbhdr {
>  static DEFINE_SPINLOCK(microcode_update_lock);
>  
>  /* See comment in start_update() for cases when this routine fails */
> -static int collect_cpu_info(unsigned int cpu, struct cpu_signature *csig)
> +static int collect_cpu_info(struct cpu_signature *csig)
>  {
> +    unsigned int cpu = smp_processor_id();
>      struct cpuinfo_x86 *c = &cpu_data[cpu];

I think it would be more clear if you used current_cpu_data here.
The only other use of "cpu" is in a pr_debug(), which by default
expands to nothing anyway, and hence is cheap to change to
use smp_processor_id() instead.

> @@ -435,14 +429,14 @@ static const unsigned int final_levels[] = {
>      0x010000af
>  };
>  
> -static bool_t check_final_patch_levels(unsigned int cpu)
> +static bool check_final_patch_levels(void)
>  {
>      /*
>       * Check the current patch levels on the cpu. If they are equal to
>       * any of the 'final_levels', then we should not update the microcode
>       * patch on the cpu as system will hang otherwise.
>       */
> -    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
> +    const struct cpu_signature *sig = &this_cpu(cpu_sig);
>      unsigned int i;

I don't see any dependency of this function upon running on
the subject CPU.

> @@ -279,12 +278,13 @@ static enum microcode_match_result compare_patch(
>   * return 1 - found update
>   * return < 0 - error
>   */
> -static int get_matching_microcode(const void *mc, unsigned int cpu)
> +static int get_matching_microcode(const void *mc)
>  {
>      const struct microcode_header_intel *mc_header = mc;
>      unsigned long total_size = get_totalsize(mc_header);
>      void *new_mc = xmalloc_bytes(total_size);
>      struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
> +    unsigned int __maybe_unused cpu = smp_processor_id();

The __maybe_unused is for the sole use in pr_debug()? Please
instead use smp_processor_id() there, if so.

Jan
Chao Gao June 10, 2019, 7:31 a.m. UTC | #2
On Tue, Jun 04, 2019 at 09:29:34AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> wrote:
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -78,8 +78,9 @@ struct mpbhdr {
>>  static DEFINE_SPINLOCK(microcode_update_lock);
>>  
>>  /* See comment in start_update() for cases when this routine fails */
>> -static int collect_cpu_info(unsigned int cpu, struct cpu_signature *csig)
>> +static int collect_cpu_info(struct cpu_signature *csig)
>>  {
>> +    unsigned int cpu = smp_processor_id();
>>      struct cpuinfo_x86 *c = &cpu_data[cpu];
>
>I think it would be more clear if you used current_cpu_data here.
>The only other use of "cpu" is in a pr_debug(), which by default
>expands to nothing anyway, and hence is cheap to change to
>use smp_processor_id() instead.

Will do.

>
>> @@ -435,14 +429,14 @@ static const unsigned int final_levels[] = {
>>      0x010000af
>>  };
>>  
>> -static bool_t check_final_patch_levels(unsigned int cpu)
>> +static bool check_final_patch_levels(void)
>>  {
>>      /*
>>       * Check the current patch levels on the cpu. If they are equal to
>>       * any of the 'final_levels', then we should not update the microcode
>>       * patch on the cpu as system will hang otherwise.
>>       */
>> -    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
>> +    const struct cpu_signature *sig = &this_cpu(cpu_sig);
>>      unsigned int i;
>
>I don't see any dependency of this function upon running on
>the subject CPU.

Ok. I will drop this change.

>
>> @@ -279,12 +278,13 @@ static enum microcode_match_result compare_patch(
>>   * return 1 - found update
>>   * return < 0 - error
>>   */
>> -static int get_matching_microcode(const void *mc, unsigned int cpu)
>> +static int get_matching_microcode(const void *mc)
>>  {
>>      const struct microcode_header_intel *mc_header = mc;
>>      unsigned long total_size = get_totalsize(mc_header);
>>      void *new_mc = xmalloc_bytes(total_size);
>>      struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
>> +    unsigned int __maybe_unused cpu = smp_processor_id();
>
>The __maybe_unused is for the sole use in pr_debug()? Please
>instead use smp_processor_id() there, if so.

Will do.

Thanks
Chao
diff mbox series

Patch

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index aecc754..4f21903 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -253,7 +253,7 @@  static int enter_state(u32 state)
 
     console_end_sync();
 
-    microcode_resume_cpu(0);
+    microcode_resume_cpu();
 
     if ( !recheck_cpu_features(0) )
         panic("Missing previously available feature(s)\n");
diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
index 0c01dfa..16a6d50 100644
--- a/xen/arch/x86/microcode.c
+++ b/xen/arch/x86/microcode.c
@@ -196,19 +196,19 @@  struct microcode_info {
     char buffer[1];
 };
 
-int microcode_resume_cpu(unsigned int cpu)
+int microcode_resume_cpu(void)
 {
     int err;
-    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+    struct cpu_signature *sig = &this_cpu(cpu_sig);
 
     if ( !microcode_ops )
         return 0;
 
     spin_lock(&microcode_mutex);
 
-    err = microcode_ops->collect_cpu_info(cpu, sig);
+    err = microcode_ops->collect_cpu_info(sig);
     if ( likely(!err) )
-        err = microcode_ops->apply_microcode(cpu);
+        err = microcode_ops->apply_microcode();
     spin_unlock(&microcode_mutex);
 
     return err;
@@ -255,9 +255,9 @@  static int microcode_update_cpu(const void *buf, size_t size)
 
     spin_lock(&microcode_mutex);
 
-    err = microcode_ops->collect_cpu_info(cpu, sig);
+    err = microcode_ops->collect_cpu_info(sig);
     if ( likely(!err) )
-        err = microcode_ops->cpu_request_microcode(cpu, buf, size);
+        err = microcode_ops->cpu_request_microcode(buf, size);
     spin_unlock(&microcode_mutex);
 
     return err;
diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index 93af2c9..0144df1 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -78,8 +78,9 @@  struct mpbhdr {
 static DEFINE_SPINLOCK(microcode_update_lock);
 
 /* See comment in start_update() for cases when this routine fails */
-static int collect_cpu_info(unsigned int cpu, struct cpu_signature *csig)
+static int collect_cpu_info(struct cpu_signature *csig)
 {
+    unsigned int cpu = smp_processor_id();
     struct cpuinfo_x86 *c = &cpu_data[cpu];
 
     memset(csig, 0, sizeof(*csig));
@@ -152,18 +153,15 @@  static bool_t find_equiv_cpu_id(const struct equiv_cpu_entry *equiv_cpu_table,
     return 0;
 }
 
-static bool_t microcode_fits(const struct microcode_amd *mc_amd,
-                             unsigned int cpu)
+static bool microcode_fits(const struct microcode_amd *mc_amd)
 {
+    unsigned int cpu = smp_processor_id();
     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;
     unsigned int equiv_cpu_id;
 
-    /* We should bind the task to the CPU */
-    BUG_ON(cpu != raw_smp_processor_id());
-
     current_cpu_id = cpuid_eax(0x00000001);
 
     if ( !find_equiv_cpu_id(equiv_cpu_table, current_cpu_id, &equiv_cpu_id) )
@@ -192,9 +190,7 @@  static bool_t microcode_fits(const struct microcode_amd *mc_amd,
 
 static bool match_cpu(const struct microcode_patch *patch)
 {
-    if ( !patch )
-        return false;
-    return microcode_fits(patch->mc_amd, smp_processor_id());
+    return patch ? microcode_fits(patch->mc_amd) : false;
 }
 
 static struct microcode_patch *alloc_microcode_patch(
@@ -253,18 +249,16 @@  static enum microcode_match_result compare_patch(
     return MIS_UCODE;
 }
 
-static int apply_microcode(unsigned int cpu)
+static int apply_microcode(void)
 {
     unsigned long flags;
     uint32_t rev;
     int hw_err;
+    unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     const struct microcode_header_amd *hdr;
     const struct microcode_patch *patch = microcode_get_cache();
 
-    /* We should bind the task to the CPU */
-    BUG_ON(raw_smp_processor_id() != cpu);
-
     if ( !match_cpu(patch) )
         return -EINVAL;
 
@@ -435,14 +429,14 @@  static const unsigned int final_levels[] = {
     0x010000af
 };
 
-static bool_t check_final_patch_levels(unsigned int cpu)
+static bool check_final_patch_levels(void)
 {
     /*
      * Check the current patch levels on the cpu. If they are equal to
      * any of the 'final_levels', then we should not update the microcode
      * patch on the cpu as system will hang otherwise.
      */
-    const struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+    const struct cpu_signature *sig = &this_cpu(cpu_sig);
     unsigned int i;
 
     if ( boot_cpu_data.x86 != 0x10 )
@@ -455,19 +449,16 @@  static bool_t check_final_patch_levels(unsigned int cpu)
     return 0;
 }
 
-static int cpu_request_microcode(unsigned int cpu, const void *buf,
-                                 size_t bufsize)
+static int cpu_request_microcode(const void *buf, size_t bufsize)
 {
     struct microcode_amd *mc_amd;
     size_t offset = 0;
     int error = 0;
     unsigned int current_cpu_id;
     unsigned int equiv_cpu_id;
+    unsigned int cpu = smp_processor_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());
-
     current_cpu_id = cpuid_eax(0x00000001);
 
     if ( *(const uint32_t *)buf != UCODE_MAGIC )
@@ -477,7 +468,7 @@  static int cpu_request_microcode(unsigned int cpu, const void *buf,
         goto out;
     }
 
-    if ( check_final_patch_levels(cpu) )
+    if ( check_final_patch_levels() )
     {
         printk(XENLOG_INFO
                "microcode: Cannot update microcode patch on the cpu as we hit a final level\n");
@@ -567,7 +558,7 @@  static int cpu_request_microcode(unsigned int cpu, const void *buf,
 
         if ( match_cpu(microcode_get_cache()) )
         {
-            error = apply_microcode(cpu);
+            error = apply_microcode();
             if ( error )
                 break;
         }
diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index bf6497f..b66844d 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -96,13 +96,12 @@  struct extended_sigtable {
 /* serialize access to the physical write to MSR 0x79 */
 static DEFINE_SPINLOCK(microcode_update_lock);
 
-static int collect_cpu_info(unsigned int cpu_num, struct cpu_signature *csig)
+static int collect_cpu_info(struct cpu_signature *csig)
 {
+    unsigned int cpu_num = smp_processor_id();
     struct cpuinfo_x86 *c = &cpu_data[cpu_num];
     uint64_t msr_content;
 
-    BUG_ON(cpu_num != smp_processor_id());
-
     memset(csig, 0, sizeof(*csig));
 
     if ( (c->x86_vendor != X86_VENDOR_INTEL) || (c->x86 < 6) )
@@ -279,12 +278,13 @@  static enum microcode_match_result compare_patch(
  * return 1 - found update
  * return < 0 - error
  */
-static int get_matching_microcode(const void *mc, unsigned int cpu)
+static int get_matching_microcode(const void *mc)
 {
     const struct microcode_header_intel *mc_header = mc;
     unsigned long total_size = get_totalsize(mc_header);
     void *new_mc = xmalloc_bytes(total_size);
     struct microcode_patch *new_patch = xmalloc(struct microcode_patch);
+    unsigned int __maybe_unused cpu = smp_processor_id();
 
     if ( !new_patch || !new_mc )
     {
@@ -312,19 +312,16 @@  static int get_matching_microcode(const void *mc, unsigned int cpu)
     return 1;
 }
 
-static int apply_microcode(unsigned int cpu)
+static int apply_microcode(void)
 {
     unsigned long flags;
     uint64_t msr_content;
     unsigned int val[2];
     unsigned int cpu_num = raw_smp_processor_id();
-    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
+    struct cpu_signature *sig = &this_cpu(cpu_sig);
     const struct microcode_intel *mc_intel;
     const struct microcode_patch *patch = microcode_get_cache();
 
-    /* We should bind the task to the CPU */
-    BUG_ON(cpu_num != cpu);
-
     if ( !match_cpu(patch) )
         return -EINVAL;
 
@@ -391,22 +388,18 @@  static long get_next_ucode_from_buffer(void **mc, const u8 *buf,
     return offset + total_size;
 }
 
-static int cpu_request_microcode(unsigned int cpu, const void *buf,
-                                 size_t size)
+static int cpu_request_microcode(const void *buf, size_t size)
 {
     long offset = 0;
     int error = 0;
     void *mc;
 
-    /* We should bind the task to the CPU */
-    BUG_ON(cpu != raw_smp_processor_id());
-
     while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 0 )
     {
         error = microcode_sanity_check(mc);
         if ( error )
             break;
-        error = get_matching_microcode(mc, cpu);
+        error = get_matching_microcode(mc);
         if ( error < 0 )
             break;
         /*
@@ -424,7 +417,7 @@  static int cpu_request_microcode(unsigned int cpu, const void *buf,
         error = offset;
 
     if ( !error && match_cpu(microcode_get_cache()) )
-        error = apply_microcode(cpu);
+        error = apply_microcode();
 
     return error;
 }
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 274865a..de19b67 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -366,7 +366,7 @@  void start_secondary(void *unused)
     if ( system_state <= SYS_STATE_smp_boot )
         early_microcode_update_cpu(false);
     else
-        microcode_resume_cpu(cpu);
+        microcode_resume_cpu();
 
     /*
      * If MSR_SPEC_CTRL is available, apply Xen's default setting and discard
diff --git a/xen/include/asm-x86/microcode.h b/xen/include/asm-x86/microcode.h
index f2ac509..e6842d4 100644
--- a/xen/include/asm-x86/microcode.h
+++ b/xen/include/asm-x86/microcode.h
@@ -19,10 +19,9 @@  struct microcode_patch {
 };
 
 struct microcode_ops {
-    int (*cpu_request_microcode)(unsigned int cpu, const void *buf,
-                                 size_t size);
-    int (*collect_cpu_info)(unsigned int cpu, struct cpu_signature *csig);
-    int (*apply_microcode)(unsigned int cpu);
+    int (*cpu_request_microcode)(const void *buf, size_t size);
+    int (*collect_cpu_info)(struct cpu_signature *csig);
+    int (*apply_microcode)(void);
     int (*start_update)(void);
     void (*free_patch)(struct microcode_patch *patch);
     bool (*match_cpu)(const struct microcode_patch *patch);
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index cef3ffb..8b7e484 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -573,7 +573,7 @@  int guest_wrmsr_xen(struct vcpu *v, uint32_t idx, uint64_t val);
 
 void microcode_set_module(unsigned int);
 int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void), unsigned long len);
-int microcode_resume_cpu(unsigned int cpu);
+int microcode_resume_cpu(void);
 int early_microcode_update_cpu(bool start_update);
 int early_microcode_init(void);
 int microcode_init_intel(void);