No point for an almost duplicate function.
Signed-off-by: Thomas Gleixner <[email protected]>
---
V2: New patch
---
arch/x86/kernel/cpu/microcode/intel.c | 16 +---------------
1 file changed, 1 insertion(+), 15 deletions(-)
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -425,21 +425,7 @@ void reload_ucode_intel(void)
static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
{
- struct cpuinfo_x86 *c = &cpu_data(cpu_num);
- unsigned int val[2];
-
- memset(csig, 0, sizeof(*csig));
-
- csig->sig = cpuid_eax(0x00000001);
-
- if ((c->x86_model >= 5) || (c->x86 > 6)) {
- /* get processor flags from MSR 0x17 */
- rdmsr(MSR_IA32_PLATFORM_ID, val[0], val[1]);
- csig->pf = 1 << ((val[1] >> 18) & 7);
- }
-
- csig->rev = c->microcode;
-
+ intel_collect_cpu_info(csig);
return 0;
}
On Tue, Sep 12, 2023 at 09:58:02AM +0200, Thomas Gleixner wrote:
> static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
You can get rid of that silly wrapper too and use
intel_collect_cpu_info() in the function pointer assignment directly.
Diff ontop:
---
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 4066dd3734ba..581ecfbaf134 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -75,7 +75,7 @@ extern __noendbr void cet_disable(void);
struct cpu_signature;
-void intel_collect_cpu_info(struct cpu_signature *sig);
+void intel_collect_cpu_info(int unused, struct cpu_signature *sig);
static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
unsigned int s2, unsigned int p2)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 6c3b10e6b214..ebf0908fd91a 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -652,7 +652,7 @@ void reload_ucode_amd(unsigned int cpu)
}
}
-static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
+static void collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu);
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -670,8 +670,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
uci->mc = p->data;
pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
-
- return 0;
}
static enum ucode_state apply_microcode_amd(int cpu)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 6d67b92d7252..77e4120de641 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -66,7 +66,7 @@ static inline unsigned int exttable_size(struct extended_sigtable *et)
return et->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE;
}
-void intel_collect_cpu_info(struct cpu_signature *sig)
+void intel_collect_cpu_info(int unused, struct cpu_signature *sig)
{
sig->sig = cpuid_eax(1);
sig->pf = 0;
@@ -362,7 +362,7 @@ static __init struct microcode_intel *get_ucode_from_cpio(struct ucode_cpu_info
if (!(cp.data && cp.size))
return NULL;
- intel_collect_cpu_info(&uci->cpu_sig);
+ intel_collect_cpu_info(0, &uci->cpu_sig);
return scan_microcode(cp.data, cp.size, uci);
}
@@ -423,12 +423,6 @@ void reload_ucode_intel(void)
apply_microcode_early(&uci, false);
}
-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
-{
- intel_collect_cpu_info(csig);
- return 0;
-}
-
static enum ucode_state apply_microcode_late(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -591,7 +585,7 @@ static void finalize_late_load(int result)
static struct microcode_ops microcode_intel_ops = {
.request_microcode_fw = request_microcode_fw,
- .collect_cpu_info = collect_cpu_info,
+ .collect_cpu_info = intel_collect_cpu_info,
.apply_microcode = apply_microcode_late,
.finalize_late_load = finalize_late_load,
};
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 051b7956d4fd..b3753025cd4a 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -30,7 +30,7 @@ struct microcode_ops {
* See also the "Synchronization" section in microcode_core.c.
*/
enum ucode_state (*apply_microcode)(int cpu);
- int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
+ void (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
void (*finalize_late_load)(int result);
};
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Thu, Sep 21 2023 at 12:42, Borislav Petkov wrote:
> On Tue, Sep 12, 2023 at 09:58:02AM +0200, Thomas Gleixner wrote:
>> static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
>
> You can get rid of that silly wrapper too and use
> intel_collect_cpu_info() in the function pointer assignment directly.
>
> Diff ontop:
>
> ---
>
> diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
> index 4066dd3734ba..581ecfbaf134 100644
> --- a/arch/x86/include/asm/cpu.h
> +++ b/arch/x86/include/asm/cpu.h
> @@ -75,7 +75,7 @@ extern __noendbr void cet_disable(void);
>
> struct cpu_signature;
>
> -void intel_collect_cpu_info(struct cpu_signature *sig);
> +void intel_collect_cpu_info(int unused, struct cpu_signature *sig);
Eew. That's a function exposed to code outside of microcode and just
grows that unused argument for no value and you obviously forgot to
fixup the extern callsite :)
> diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
> index 051b7956d4fd..b3753025cd4a 100644
> --- a/arch/x86/kernel/cpu/microcode/internal.h
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -30,7 +30,7 @@ struct microcode_ops {
> * See also the "Synchronization" section in microcode_core.c.
> */
> enum ucode_state (*apply_microcode)(int cpu);
> - int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> + void (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
> void (*finalize_late_load)(int result);
Making this void makes sense, but that's a separate change.
Thanks,
tglx
On Mon, Sep 25, 2023 at 12:47:16PM +0200, Thomas Gleixner wrote:
> Eew. That's a function exposed to code outside of microcode and just
> grows that unused argument for no value and you obviously forgot to
> fixup the extern callsite :)
It's used on AMD. Adding the below to the pile.
---
From: "Borislav Petkov (AMD)" <[email protected]>
Date: Tue, 3 Oct 2023 16:12:01 +0200
Subject: [PATCH] x86/microcode: Make microcode_ops.collect_cpu_info() return
void
Simplify code flow a bit more in the process.
No functional changes.
Signed-off-by: Borislav Petkov (AMD) <[email protected]>
Link: https://lore.kernel.org/r/20230921104220.GHZQweDKyaJmkYdt4f@fat_crate.local
---
arch/x86/include/asm/cpu.h | 2 +-
arch/x86/kernel/cpu/microcode/amd.c | 4 +---
arch/x86/kernel/cpu/microcode/intel.c | 12 +++---------
arch/x86/kernel/cpu/microcode/internal.h | 2 +-
4 files changed, 6 insertions(+), 14 deletions(-)
diff --git a/arch/x86/include/asm/cpu.h b/arch/x86/include/asm/cpu.h
index 4066dd3734ba..581ecfbaf134 100644
--- a/arch/x86/include/asm/cpu.h
+++ b/arch/x86/include/asm/cpu.h
@@ -75,7 +75,7 @@ extern __noendbr void cet_disable(void);
struct cpu_signature;
-void intel_collect_cpu_info(struct cpu_signature *sig);
+void intel_collect_cpu_info(int unused, struct cpu_signature *sig);
static inline bool intel_cpu_signatures_match(unsigned int s1, unsigned int p1,
unsigned int s2, unsigned int p2)
diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 0f15e82a536c..5d1c2a716456 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -632,7 +632,7 @@ void reload_ucode_amd(unsigned int cpu)
}
}
-static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
+static void collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
{
struct cpuinfo_x86 *c = &cpu_data(cpu);
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -650,8 +650,6 @@ static int collect_cpu_info_amd(int cpu, struct cpu_signature *csig)
uci->mc = p->data;
pr_info("CPU%d: patch_level=0x%08x\n", cpu, csig->rev);
-
- return 0;
}
static enum ucode_state apply_microcode_amd(int cpu)
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 3817bb2ad6ac..0eff86a5ab8f 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -66,7 +66,7 @@ static inline unsigned int exttable_size(struct extended_sigtable *et)
return et->count * EXT_SIGNATURE_SIZE + EXT_HEADER_SIZE;
}
-void intel_collect_cpu_info(struct cpu_signature *sig)
+void intel_collect_cpu_info(int unused, struct cpu_signature *sig)
{
sig->sig = cpuid_eax(1);
sig->pf = 0;
@@ -363,7 +363,7 @@ static __init struct microcode_intel *get_microcode_blob(struct ucode_cpu_info *
if (!(cp.data && cp.size))
return NULL;
- intel_collect_cpu_info(&uci->cpu_sig);
+ intel_collect_cpu_info(0, &uci->cpu_sig);
return scan_microcode(cp.data, cp.size, uci);
}
@@ -424,12 +424,6 @@ void reload_ucode_intel(void)
apply_microcode_early(&uci);
}
-static int collect_cpu_info(int cpu_num, struct cpu_signature *csig)
-{
- intel_collect_cpu_info(csig);
- return 0;
-}
-
static enum ucode_state apply_microcode_late(int cpu)
{
struct ucode_cpu_info *uci = ucode_cpu_info + cpu;
@@ -592,7 +586,7 @@ static void finalize_late_load(int result)
static struct microcode_ops microcode_intel_ops = {
.request_microcode_fw = request_microcode_fw,
- .collect_cpu_info = collect_cpu_info,
+ .collect_cpu_info = intel_collect_cpu_info,
.apply_microcode = apply_microcode_late,
.finalize_late_load = finalize_late_load,
};
diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
index 051b7956d4fd..b3753025cd4a 100644
--- a/arch/x86/kernel/cpu/microcode/internal.h
+++ b/arch/x86/kernel/cpu/microcode/internal.h
@@ -30,7 +30,7 @@ struct microcode_ops {
* See also the "Synchronization" section in microcode_core.c.
*/
enum ucode_state (*apply_microcode)(int cpu);
- int (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
+ void (*collect_cpu_info)(int cpu, struct cpu_signature *csig);
void (*finalize_late_load)(int result);
};
--
2.42.0.rc0.25.ga82fb66fed25
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
On Tue, Oct 03, 2023 at 04:14:39PM +0200, Borislav Petkov wrote:
> On Mon, Sep 25, 2023 at 12:47:16PM +0200, Thomas Gleixner wrote:
> > Eew. That's a function exposed to code outside of microcode and just
> > grows that unused argument for no value and you obviously forgot to
> > fixup the extern callsite :)
>
> It's used on AMD. Adding the below to the pile.
And now that I look at it again, exposing that "unused" arg is uglier
than having the local wrapper in the loader code. Yeah, lemme zap that.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette