Re: [PATCH v5 3/7] x86/microcode/intel: Establish staging control logic

["Chang S. Bae" <chang.seok.bae@intel.com>]

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

On 9/4/2025 5:13 AM, Borislav Petkov wrote:
> On Sat, Aug 23, 2025 at 08:52:06AM -0700, Chang S. Bae wrote:
>>
>> Also, define cpu_primary_thread_mask for the CONFIG_SMP=n case, allowing
>> consistent use when narrowing down primary threads to locate the
>> per-package interface.
> 
> This paragraph is stale now and can go.

Ah, right. Sorry, I had to chop it out.

>> diff --git a/arch/x86/include/asm/msr-index.h b/arch/x86/include/asm/msr-index.h
>> index b65c3ba5fa14..0356155f9264 100644
>> --- a/arch/x86/include/asm/msr-index.h
>> +++ b/arch/x86/include/asm/msr-index.h
>> @@ -913,6 +913,8 @@
>>   #define MSR_IA32_UCODE_WRITE		0x00000079
>>   #define MSR_IA32_UCODE_REV		0x0000008b
>>   
>> +#define MSR_IA32_MCU_STAGING_MBOX_ADDR	0x000007a5
> 
> Doesn't look sorted to me.

Okay, I think this 'MCU' looks a bit fuzzy. Intel folks already use this 
acronym elsewhere (e.g. MSR_IA32_MCU_OPT_CTRL). It might be clearer to 
consolidate them under one section, like:

/* Intel microcode update MSRs */
#define MSR_IA32_MCU_OPT_CTRL           0x00000123
#define MSR_IA32_MCU_ENUMERATION        0x0000007b  <- patch7 adds this
#define MSR_IA32_MCU_STAGING_MBOX_ADDR  0x000007a5
...

>> +	/*
>> +	 * The MMIO address is unique per package, and all the SMT
>> +	 * primary threads are online here. Find each MMIO space by
>> +	 * their package ids to avoid duplicate staging.
>> +	 */
>> +	for_each_cpu(cpu, cpu_primary_thread_mask) {
>> +		if (topology_logical_package_id(cpu) == pkg_id)
>> +			continue;
> 
> <---- newline here.

Fixed. Thanks.

>> +		ret = do_stage(mmio_pa);
>> +		if (ret != UCODE_OK) {
>> +			pr_err("Error: staging failed with %s for CPU%d at package %u.\n",
>> +			       ret == UCODE_TIMEOUT ? "timeout" : "error state",
> 
> What does "error state" mean?
> 
> Are we going to dump additional error state so that it is clear why it failed?

Yeah, right. The wording "error state" is vague.

The next two patches in this series introduce helpers that return an 
error code and *also* update this ucode_state. The latter could go away. 
Instead, the error code could be just down through here and decoded like:

static const char *staging_errstr(int code)
{
   switch (code) {
   case -ETIMEDOUT:	return "timeout";
   ...
   default:		return "unknown error";
   }
}

static void stage_microcode(void)
{
   ...

   err = do_stage(mmio_pa);
   if (err) {
     pr_err("Error: staging failed with %s for CPU%d at package %u.\n",
            staging_errstr(err), cpu, pkg_id);
     return;
   }
}

I've attached a diff on top of V5 to picture what it will look like.

[-- Attachment #2: tmp.diff --]
[-- Type: text/plain, Size: 5703 bytes --]

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 455b50514d87..ae4b08e71e20 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -98,7 +98,6 @@ struct extended_sigtable {
  * @chunk_size:		Size of each data piece
  * @bytes_sent:		Total bytes transmitted so far
  * @offset:		Current offset in the microcode image
- * @state:		Current state of the staging process
  */
 struct staging_state {
 	void __iomem		*mmio_base;
@@ -106,7 +105,6 @@ struct staging_state {
 	unsigned int		chunk_size;
 	unsigned int		bytes_sent;
 	unsigned int		offset;
-	enum ucode_state	state;
 };
 
 #define DEFAULT_UCODE_TOTALSIZE (DEFAULT_UCODE_DATASIZE + MC_HEADER_SIZE)
@@ -425,7 +423,7 @@ static inline unsigned int calc_next_chunk_size(unsigned int ucode_len, unsigned
  * Update the chunk size and decide whether another chunk can be sent.
  * This accounts for remaining data and retry limits.
  */
-static bool can_send_next_chunk(struct staging_state *ss)
+static bool can_send_next_chunk(struct staging_state *ss, int *err)
 {
 	ss->chunk_size = calc_next_chunk_size(ss->ucode_len, ss->offset);
 	/*
@@ -445,10 +443,11 @@ static bool can_send_next_chunk(struct staging_state *ss)
 	 * likely stuck and mark the state as timeout.
 	 */
 	if (ss->bytes_sent + ss->chunk_size > ss->ucode_len * 2) {
-		ss->state = UCODE_TIMEOUT;
+		*err = -ETIMEDOUT;
 		return false;
 	}
 
+	*err = 0;
 	return true;
 }
 
@@ -465,9 +464,9 @@ static inline bool is_end_offset(u32 offset)
  * the end offset, or no more transactions are permitted (retry limit
  * reached).
  */
-static inline bool staging_is_complete(struct staging_state *ss)
+static inline bool staging_is_complete(struct staging_state *ss, int *err)
 {
-	return is_end_offset(ss->offset) || !can_send_next_chunk(ss);
+	return is_end_offset(ss->offset) || !can_send_next_chunk(ss, err);
 }
 
 /*
@@ -489,21 +488,16 @@ static int wait_for_transaction(struct staging_state *ss)
 	}
 
 	/* Check for explicit error response */
-	if (status & MASK_MBOX_STATUS_ERROR) {
-		ss->state = UCODE_ERROR;
-		return -EPROTO;
-	}
+	if (status & MASK_MBOX_STATUS_ERROR)
+		return -EIO;
 
 	/*
 	 * Hardware is neither responded to the action nor signaled any
 	 * error. Treat this as timeout.
 	 */
-	if (!(status & MASK_MBOX_STATUS_READY)) {
-		ss->state = UCODE_TIMEOUT;
+	if (!(status & MASK_MBOX_STATUS_READY))
 		return -ETIMEDOUT;
-	}
 
-	ss->state = UCODE_OK;
 	return 0;
 }
 
@@ -545,7 +539,6 @@ static int fetch_next_offset(struct staging_state *ss)
 	const u64 expected_header = MBOX_HEADER(MBOX_HEADER_SIZE + MBOX_RESPONSE_SIZE);
 	u32 offset, status;
 	u64 header;
-	int err;
 
 	/*
 	 * The 'response' mailbox returns three fields, in order:
@@ -558,55 +551,42 @@ static int fetch_next_offset(struct staging_state *ss)
 	status = read_mbox_dword(ss->mmio_base);
 
 	/* All valid responses must start with the expected header. */
-	if (header != expected_header) {
-		pr_err_once("staging: invalid response header\n");
-		err = -EINVAL;
-		goto err_out;
-	}
+	if (header != expected_header)
+		return -EBADR;
 
 	/*
 	 * Verify the offset: If not at the end marker, it must not
 	 * exceed the microcode image length
 	 */
-	if (!is_end_offset(offset) && offset > ss->ucode_len) {
-		pr_err_once("staging: invalid response offset\n");
-		err = -EINVAL;
-		goto err_out;
-	}
+	if (!is_end_offset(offset) && offset > ss->ucode_len)
+		return -EINVAL;
 
 	/* Hardware may report errors explicitly in the status field */
-	if (status & MASK_MBOX_RESP_ERROR) {
-		err = -EPROTO;
-		goto err_out;
-	}
+	if (status & MASK_MBOX_RESP_ERROR)
+		return -EPROTO;
 
 	ss->offset = offset;
-	ss->state  = UCODE_OK;
 	return 0;
-
-err_out:
-	ss->state = UCODE_ERROR;
-	return err;
 }
 
 /*
  * Handle the staging process using the mailbox MMIO interface. The
  * microcode image is transferred in chunks until completion. Return the
- * result state.
+ * error code.
  */
-static enum ucode_state do_stage(u64 mmio_pa)
+static int do_stage(u64 mmio_pa)
 {
 	struct staging_state ss = {};
 	int err;
 
 	ss.mmio_base = ioremap(mmio_pa, MBOX_REG_NUM * MBOX_REG_SIZE);
 	if (WARN_ON_ONCE(!ss.mmio_base))
-		return UCODE_ERROR;
+		return -EADDRNOTAVAIL;
 
 	init_stage(&ss);
 
 	/* Perform the staging process while within the retry limit */
-	while (!staging_is_complete(&ss)) {
+	while (!staging_is_complete(&ss, &err)) {
 		/* Send a chunk of microcode each time: */
 		err = send_data_chunk(&ss, ucode_patch_late);
 		if (err)
@@ -622,17 +602,25 @@ static enum ucode_state do_stage(u64 mmio_pa)
 
 	iounmap(ss.mmio_base);
 
-	/*
-	 * The helpers update ss.state on error. The final state is
-	 * returned to the caller.
-	 */
-	return ss.state;
+	return err;
+}
+
+static const char *staging_errstr(int code)
+{
+	switch (code) {
+	case -EBADR:		return "invalid response header";
+	case -EINVAL:		return "invalid next offset";
+	case -EIO:		return "transaction error";
+	case -ETIMEDOUT:	return "timeout";
+	case -EPROTO:		return "response error";
+	case -EADDRNOTAVAIL:	return "ioremap() failure";
+	default:		return "unknown error";
+	};
 }
 
 static void stage_microcode(void)
 {
 	unsigned int pkg_id = UINT_MAX;
-	enum ucode_state ret;
 	int cpu, err;
 	u64 mmio_pa;
 
@@ -656,11 +644,10 @@ static void stage_microcode(void)
 		if (WARN_ON_ONCE(err))
 			return;
 
-		ret = do_stage(mmio_pa);
-		if (ret != UCODE_OK) {
+		err = do_stage(mmio_pa);
+		if (err) {
 			pr_err("Error: staging failed with %s for CPU%d at package %u.\n",
-			       ret == UCODE_TIMEOUT ? "timeout" : "error state",
-			       cpu, pkg_id);
+			       staging_errstr(err), cpu, pkg_id);
 			return;
 		}
 	}