linux-kernel - Re: [PATCH v2a 4/6] x86/microcode/intel: Implement staging handler

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <Z+O8DK5NZJL43Nt6@intel.com>
Date: Wed, 26 Mar 2025 16:34:20 +0800
From: Chao Gao <chao.gao@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>
CC: <linux-kernel@...r.kernel.org>, <x86@...nel.org>, <tglx@...utronix.de>,
	<mingo@...hat.com>, <bp@...en8.de>, <dave.hansen@...ux.intel.com>,
	<colinmitchell@...gle.com>
Subject: Re: [PATCH v2a 4/6] x86/microcode/intel: Implement staging handler

> /*
>- * Handle the staging process using the mailbox MMIO interface.
>- * Return the result state.
>+ * Prepare for a new microcode transfer by resetting hardware and
>+ * initializing software states.
>+ */
>+static void init_stage(struct staging_state *ss)
>+{
>+	ss->ucode_ptr = ucode_patch_late;
>+	ss->ucode_len = get_totalsize(&ucode_patch_late->hdr);
>+
>+	/* Reset tracking variables */
>+	ss->offset = 0;
>+	ss->bytes_sent = 0;

Nit: no need to reset them, as

>+	struct staging_state ss = {};

in do_stage() will zero the whole structure.

>+/*
>+ * Handle the staging process using the mailbox MMIO interface. The
>+ * microcode image is transferred in chunks until completion. Return the
>+ * result state.
>  */
> static enum ucode_state do_stage(u64 mmio_pa)
> {
>-	pr_debug_once("Staging implementation is pending.\n");
>-	return UCODE_ERROR;
>+	struct staging_state ss = {};
>+
>+	ss.mmio_base = ioremap(mmio_pa, MBOX_REG_NUM * MBOX_REG_SIZE);
>+	if (WARN_ON_ONCE(!ss.mmio_base))
>+		return UCODE_ERROR;
>+
>+	init_stage(&ss);
>+
>+	/* Perform the staging process while within the retry limit */
>+	while (!is_stage_complete(ss.offset) && can_send_next_chunk(&ss)) {
>+		/* Send a chunk of microcode each time: */
>+		if (!send_data_chunk(&ss))
>+			break;
>+		/*
>+		 * Then, ask the hardware which piece of the image it
>+		 * needs next. The same piece may be sent more than once.
>+		 */
>+		if (!fetch_next_offset(&ss))
>+			break;

why send_data_chunk() and fetch_next_offset() return a boolean instead of
an error or ucode_state?

Using the return value to indicate just success or failure, while relying
on another variable to report detailed error/state, seems a bit clumsy to
me.

>+	}
>+
>+	iounmap(ss.mmio_base);
>+	return ss.state;
> }
> 
> static void stage_microcode(void)
>-- 
>2.45.2
>