linux-kernel - Re: [PATCH RFC 4/7] x86/microcode/intel: Prepare for microcode staging

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241104111630.GSZyitDuXnBYmEFxvo@fat_crate.local>
Date: Mon, 4 Nov 2024 12:16:30 +0100
From: Borislav Petkov <bp@...en8.de>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
Cc: linux-kernel@...r.kernel.org, x86@...nel.org, tglx@...utronix.de,
	mingo@...hat.com, dave.hansen@...ux.intel.com
Subject: Re: [PATCH RFC 4/7] x86/microcode/intel: Prepare for microcode
 staging

On Tue, Oct 01, 2024 at 09:10:39AM -0700, Chang S. Bae wrote:
> +static inline u64 staging_addr(u32 cpu)
> +{
> +	u32 lo, hi;
> +
> +	rdmsr_on_cpu(cpu, MSR_IA32_MCU_STAGING_MBOX_ADDR, &lo, &hi);
> +	return lo | ((u64)hi << 32);
> +}

A single usage site. Move its code there and get rid of the function.

> +
> +static bool need_staging(u64 *mmio_addrs, u64 pa)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; mmio_addrs[i] != 0; i++) {
> +		if (mmio_addrs[i] == pa)
> +			return false;
> +	}
> +	mmio_addrs[i] = pa;

This is a weird function - its name is supposed to mean it queries something
but then it has side effects too.

> +	return true;
> +}
> +
> +static void staging_microcode(void)

stage_microcode().

Functions should have verbs in their name and have the meaning of
a "do-something".

> +{
> +	u64 *mmio_addrs, mmio_pa;
> +	unsigned int totalsize;
> +	int cpu;
> +
> +	totalsize = get_totalsize(&ucode_patch_late->hdr);
> +	if (!IS_ALIGNED(totalsize, sizeof(u32)))
> +		return;
> +
> +	mmio_addrs = kcalloc(nr_cpu_ids, sizeof(*mmio_addrs), GFP_KERNEL);
> +	if (WARN_ON_ONCE(!mmio_addrs))
> +		return;
> +
> +	for_each_cpu(cpu, cpu_online_mask) {

Oh great, and someone went and offlined one of those CPUs right here. Fun.

> +		mmio_pa = staging_addr(cpu);
> +
> +		if (need_staging(mmio_addrs, mmio_pa) &&
> +		    !staging_work(mmio_pa, ucode_patch_late, totalsize)) {

do_stage()

> +			pr_err("Error: staging failed.\n");

							... on CPU%d, err_val: 0x%x"\n"

perhaps?

For more info debugging something like that?

> +			goto out;
> +		}
> +	}
> +
> +	pr_info("Staging succeeded.\n");

		"Staging of patch revision 0x%x succeeded.\n"...

more user-friendly.

> +out:
> +	kfree(mmio_addrs);
> +}
> +
>  static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
>  					  struct microcode_intel *mc,
>  					  u32 *cur_rev)
> @@ -627,6 +676,7 @@ static struct microcode_ops microcode_intel_ops = {
>  	.collect_cpu_info	= collect_cpu_info,
>  	.apply_microcode	= apply_microcode_late,
>  	.finalize_late_load	= finalize_late_load,
> +	.staging_microcode	= staging_microcode,
>  	.use_nmi		= IS_ENABLED(CONFIG_X86_64),
>  };
>  
> diff --git a/arch/x86/kernel/cpu/microcode/internal.h b/arch/x86/kernel/cpu/microcode/internal.h
> index cb58e83e4934..06c3c8db4ceb 100644
> --- a/arch/x86/kernel/cpu/microcode/internal.h
> +++ b/arch/x86/kernel/cpu/microcode/internal.h
> @@ -120,6 +120,11 @@ void load_ucode_intel_bsp(struct early_load_data *ed);
>  void load_ucode_intel_ap(void);
>  void reload_ucode_intel(void);
>  struct microcode_ops *init_intel_microcode(void);
> +static inline bool staging_work(u64 mmio_pa, void *ucode_ptr, unsigned int totalsize)
> +{
> +	pr_debug_once("Need to implement the Staging code.\n");
> +	return false;
> +}
>  #else /* CONFIG_CPU_SUP_INTEL */

You better have an empty stub for that work function here too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette