[PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[Liu, Jinsong]

[-- Attachment #1: Type: text/plain, Size: 230 bytes --]

X86 microcode: cancel redundant input parameter of microcode functions

Cancel redundant input parameter 'uci', since it can get from another input parameter 'cpu' as index.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

[-- Attachment #2: microcode-1-remove-redundant.patch --]
[-- Type: application/octet-stream, Size: 5971 bytes --]

X86 microcode: cancel redundant input parameter of microcode functions

Cancel redundant input parameter 'uci', since it can get from another input parameter 'cpu' as index.

Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

diff -r a0dddcbd9e32 xen/arch/x86/microcode.c
--- a/xen/arch/x86/microcode.c	Tue Jan 20 16:41:29 2009 +0000
+++ b/xen/arch/x86/microcode.c	Wed Jan 21 15:31:51 2009 +0800
@@ -49,23 +49,28 @@ struct microcode_info {
     char buffer[1];
 };
 
-static void microcode_fini_cpu(struct ucode_cpu_info *uci, int cpu)
+static void microcode_fini_cpu(int cpu)
 {
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
     spin_lock(&microcode_mutex);
     xfree(uci->mc.mc_valid);
     uci->mc.mc_valid = NULL;
     spin_unlock(&microcode_mutex);
 }
 
-static int collect_cpu_info(struct ucode_cpu_info *uci, int cpu)
+static int collect_cpu_info(int cpu)
 {
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
+
     memset(uci, 0, sizeof(*uci));
     return microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
 }
 
-static int microcode_resume_cpu(struct ucode_cpu_info *uci, int cpu)
+static int microcode_resume_cpu(int cpu)
 {
     int err = 0;
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
     struct cpu_signature nsig;
 
     gdprintk(XENLOG_INFO, "microcode: CPU%d resumed\n", cpu);
@@ -80,25 +85,25 @@ static int microcode_resume_cpu(struct u
     err = microcode_ops->collect_cpu_info(cpu, &nsig);
     if ( err )
     {
-        microcode_fini_cpu(uci, cpu);
+        microcode_fini_cpu(cpu);
         return err;
     }
 
     if ( memcmp(&nsig, &uci->cpu_sig, sizeof(nsig)) )
     {
-        microcode_fini_cpu(uci, cpu);
+        microcode_fini_cpu(cpu);
         /* Should we look for a new ucode here? */
         return -EIO;
     }
 
-    return microcode_ops->apply_microcode(uci, cpu);
+    return microcode_ops->apply_microcode(cpu);
 }
 
 static int microcode_update_cpu(const void *buf, size_t size)
 {
     int err;
     unsigned int cpu = smp_processor_id();
-    struct ucode_cpu_info *uci = &ucode_cpu_info[cpu];
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
 
     spin_lock(&microcode_mutex);
 
@@ -107,11 +112,11 @@ static int microcode_update_cpu(const vo
      * otherwise just request a firmware:
      */
     if ( uci->mc.mc_valid ) {
-        err = microcode_resume_cpu(uci, cpu);
+        err = microcode_resume_cpu(cpu);
     } else {
-        err = collect_cpu_info(uci, cpu);
+        err = collect_cpu_info(cpu);
         if ( !err )
-            err = microcode_ops->cpu_request_microcode(uci, cpu, buf, size);
+            err = microcode_ops->cpu_request_microcode(cpu, buf, size);
     }
 
     spin_unlock(&microcode_mutex);
diff -r a0dddcbd9e32 xen/arch/x86/microcode_amd.c
--- a/xen/arch/x86/microcode_amd.c	Tue Jan 20 16:41:29 2009 +0000
+++ b/xen/arch/x86/microcode_amd.c	Wed Jan 21 15:31:51 2009 +0800
@@ -122,9 +122,10 @@ out:
     return 0;
 }
 
-static int apply_microcode(struct ucode_cpu_info *uci, int cpu)
+static int apply_microcode(int cpu)
 {
     unsigned long flags;
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
     uint32_t rev, dummy;
     struct microcode_amd *mc_amd = uci->mc.mc_amd;
 
@@ -246,18 +247,17 @@ static int install_equiv_cpu_table(const
     return 0;
 }
 
-static int cpu_request_microcode(struct ucode_cpu_info *uci,
-				int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(int cpu, const void *buf, size_t size)
 {
     const uint32_t *buf_pos;
     unsigned long offset = 0;
     int error = 0;
     int ret;
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
     void *mc;
 
     /* We should bind the task to the CPU */
     BUG_ON(cpu != raw_smp_processor_id());
-    BUG_ON(uci != &ucode_cpu_info[cpu]);
 
     buf_pos = (const uint32_t *)buf;
 
@@ -297,7 +297,7 @@ static int cpu_request_microcode(struct 
         if (error != 0)
             continue;
 
-        error = apply_microcode(uci, cpu);
+        error = apply_microcode(cpu);
         if (error == 0)
             break;
     }
diff -r a0dddcbd9e32 xen/arch/x86/microcode_intel.c
--- a/xen/arch/x86/microcode_intel.c	Tue Jan 20 16:41:29 2009 +0000
+++ b/xen/arch/x86/microcode_intel.c	Wed Jan 21 15:31:51 2009 +0800
@@ -244,11 +244,12 @@ static int get_matching_microcode(void *
     return 1;
 }
 
-static int apply_microcode(struct ucode_cpu_info *uci, int cpu)
+static int apply_microcode(int cpu)
 {
     unsigned long flags;
     unsigned int val[2];
     int cpu_num = raw_smp_processor_id();
+    struct ucode_cpu_info *uci = ucode_cpu_info + cpu_num;
 
     /* We should bind the task to the CPU */
     BUG_ON(cpu_num != cpu);
@@ -317,8 +318,7 @@ static long get_next_ucode_from_buffer(v
     return offset + total_size;
 }
 
-static int cpu_request_microcode(struct ucode_cpu_info *uci,
-                                int cpu, const void *buf, size_t size)
+static int cpu_request_microcode(int cpu, const void *buf, size_t size)
 {
     long offset = 0;
     int error = 0;
@@ -341,7 +341,7 @@ static int cpu_request_microcode(struct 
          */
         if ( error == 1 )
         {
-            apply_microcode(uci, cpu);
+            apply_microcode(cpu);
             error = 0;
         }
         xfree(mc);
diff -r a0dddcbd9e32 xen/include/asm-x86/microcode.h
--- a/xen/include/asm-x86/microcode.h	Tue Jan 20 16:41:29 2009 +0000
+++ b/xen/include/asm-x86/microcode.h	Wed Jan 21 15:31:51 2009 +0800
@@ -5,10 +5,9 @@ struct ucode_cpu_info;
 struct ucode_cpu_info;
 
 struct microcode_ops {
-    int (*cpu_request_microcode)(struct ucode_cpu_info *uci,
-                                 int cpu, const void *buf, size_t size);
+    int (*cpu_request_microcode)(int cpu, const void *buf, size_t size);
     int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
-    int (*apply_microcode)(struct ucode_cpu_info *uci, int cpu);
+    int (*apply_microcode)(int cpu);
 };
 
 struct microcode_header_intel {

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[Christoph Egger]

On Thursday 22 January 2009 04:08:21 Liu, Jinsong wrote:
> X86 microcode: cancel redundant input parameter of microcode functions
>
> Cancel redundant input parameter 'uci', since it can get from another input
> parameter 'cpu' as index.
>
> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>

I added the parameter 'uci' to reduce dependency on a global variable.
That improves reentrancy and cache locality.

If you want to go a step forward rather back, then remove the 'cpu' parameter
instead.

Christoph

-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

RE: [PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[Liu, Jinsong]

Christoph Egger wrote:
> On Thursday 22 January 2009 04:08:21 Liu, Jinsong wrote:
>> X86 microcode: cancel redundant input parameter of microcode
>> functions 
>> 
>> Cancel redundant input parameter 'uci', since it can get from
>> another input parameter 'cpu' as index. 
>> 
>> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> 
> I added the parameter 'uci' to reduce dependency on a global variable.
> That improves reentrancy and cache locality.
> 
> If you want to go a step forward rather back, then remove the 'cpu'
> parameter instead.
> 
> Christoph

I think the input parameter 'cpu' is better than 'uci' since 'cpu' is much clear to developer with explicit meaning, so between the 2 redundant parameters 'uci' and 'cpu', we should remove 'uci', just like what native linux microcode functions do.

As for cache locality, it's not important for microcode functions since it's not in key path, seldom be called. BTW, we'd better keep consistent with native linux code for the sake of futher porting and upgrade, you know, the latest linux kernel (2.6.28) still has some issues for microcode ...

Thanks,
Jinsong

Re: [PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[Christoph Egger]

On Thursday 22 January 2009 11:54:15 Liu, Jinsong wrote:
> Christoph Egger wrote:
> > On Thursday 22 January 2009 04:08:21 Liu, Jinsong wrote:
> >> X86 microcode: cancel redundant input parameter of microcode
> >> functions
> >>
> >> Cancel redundant input parameter 'uci', since it can get from
> >> another input parameter 'cpu' as index.
> >>
> >> Signed-off-by: Liu, Jinsong <jinsong.liu@intel.com>
> >
> > I added the parameter 'uci' to reduce dependency on a global variable.
> > That improves reentrancy and cache locality.
> >
> > If you want to go a step forward rather back, then remove the 'cpu'
> > parameter instead.
> >
> > Christoph
>
> I think the input parameter 'cpu' is better than 'uci' since 'cpu' is much
> clear to developer with explicit meaning, so between the 2 redundant
> parameters 'uci' and 'cpu', we should remove 'uci', just like what native
> linux microcode functions do.

What linux does, doesn't matter here. Dom0 just has to make the hypercall.
This is nothing linux specific.

> As for cache locality, it's not important for microcode functions since
> it's not in key path, seldom be called.

Right, but better reentrancy is always good.

> BTW, we'd better keep consistent 
> with native linux code for the sake of futher porting and upgrade, you
> know, the latest linux kernel (2.6.28) still has some issues for microcode
> ...

You should have hear yourself.

Someone else can say:

BTW, we'd better keep consistent
with native Solaris code for the sake of further porting and upgrade. It 
doesn't suffer on the Linux issues.

And another guy can say:

BTW, we'd better keep consistent with native BSD code for the sake
of further porting and upgrade.


You see what I mean?


Christoph


-- 
---to satisfy European Law for business letters:
Advanced Micro Devices GmbH
Karl-Hammerschmidt-Str. 34, 85609 Dornach b. Muenchen
Geschaeftsfuehrer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
Sitz: Dornach, Gemeinde Aschheim, Landkreis Muenchen
Registergericht Muenchen, HRB Nr. 43632

Re: [PATCH 1/4]X86 microcode: cancel redundant input parameter of microcode functions

[Keir Fraser]

On 22/01/2009 11:07, "Christoph Egger" <Christoph.Egger@amd.com> wrote:

>> I think the input parameter 'cpu' is better than 'uci' since 'cpu' is much
>> clear to developer with explicit meaning, so between the 2 redundant
>> parameters 'uci' and 'cpu', we should remove 'uci', just like what native
>> linux microcode functions do.
> 
> What linux does, doesn't matter here. Dom0 just has to make the hypercall.
> This is nothing linux specific.

Like much of Xen's low-level platform code, our microcode mechanism has
direct Linux heritage. Hence keeping as close as possible to those roots
makes upgrades from kernel.org upstream much easier. Hence there is good
reason for caring about closeness to Linux code compared with
solaris/bsd/...

 -- Keir