Re: [PATCH v5 5/7] x86/microcode/intel: Implement staging handler

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

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

On 9/10/2025 11:33 AM, Borislav Petkov wrote:
> On Sat, Aug 23, 2025 at 08:52:08AM -0700, Chang S. Bae wrote:
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
>> index 3ca22457d839..a1b13202330d 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -20,6 +20,8 @@
>>   #include <linux/cpu.h>
>>   #include <linux/uio.h>
>>   #include <linux/mm.h>
>> +#include <linux/delay.h>
>> +#include <linux/io.h>
> 
> You do see those are sorted by header name length in a reverse order, right?

Okay, fixed -- and I adjusted patch6 as well:

  #include <linux/initrd.h>
  #include <linux/io.h>
  #include <linux/kernel.h>
+#include <linux/pci_ids.h>
  #include <linux/slab.h>
  #include <linux/cpu.h>
  #include <linux/uio.h>

>> +/* Defines for the microcode staging mailbox interface */
>> +
> 
> ^ Superfluous newline.

Dropped.

>> +/*
>> + * Return PAGE_SIZE, or remaining bytes if this is the final chunk
>> + */
>> +static inline unsigned int calc_next_chunk_size(unsigned int ucode_len, unsigned int offset)
>> +{
>> +	return min(PAGE_SIZE, ucode_len - offset);
>> +}
> 
> That oneliner looks useless - sticking a comment over tne min() and putting it
> at the single callsite below is good enough.

Agreed -- removed the helper and moved them.

>> +/*
>> + * 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)
>> +{
>> +	ss->chunk_size = calc_next_chunk_size(ss->ucode_len, ss->offset);
>> +	/*
>> +	 * Each microcode image is divided into chunks, each at most
>> +	 * one page size. A 10-chunk  image would typically require 10
> 				   ^^^^

Fixed.

Just to make sure, include the diff here.

Thanks for the careful review and for sticking with this set.

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

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c7a75afd2b9a..4d663cab4f48 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -12,16 +12,16 @@
  */
 #define pr_fmt(fmt) "microcode: " fmt
 #include <linux/earlycpio.h>
+#include <linux/delay.h>
 #include <linux/firmware.h>
 #include <linux/uaccess.h>
 #include <linux/initrd.h>
+#include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/cpu.h>
 #include <linux/uio.h>
 #include <linux/mm.h>
-#include <linux/delay.h>
-#include <linux/io.h>
 
 #include <asm/cpu_device_id.h>
 #include <asm/processor.h>
@@ -36,7 +36,6 @@ static const char ucode_path[] = "kernel/x86/microcode/GenuineIntel.bin";
 #define UCODE_BSP_LOADED	((struct microcode_intel *)0x1UL)
 
 /* Defines for the microcode staging mailbox interface */
-
 #define MBOX_REG_NUM		4
 #define MBOX_REG_SIZE		sizeof(u32)
 
@@ -344,24 +343,18 @@ static void init_stage(struct staging_state *ss)
 	writel(MASK_MBOX_CTRL_ABORT, ss->mmio_base + MBOX_CONTROL_OFFSET);
 }
 
-/*
- * Return PAGE_SIZE, or remaining bytes if this is the final chunk
- */
-static inline unsigned int calc_next_chunk_size(unsigned int ucode_len, unsigned int offset)
-{
-	return min(PAGE_SIZE, ucode_len - offset);
-}
-
 /*
  * 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, int *err)
 {
-	ss->chunk_size = calc_next_chunk_size(ss->ucode_len, ss->offset);
+	/* a page size or remaining bytes if this is the final chunk */
+	ss->chunk_size = min(PAGE_SIZE, ss->ucode_len - ss->offset);
+
 	/*
 	 * Each microcode image is divided into chunks, each at most
-	 * one page size. A 10-chunk  image would typically require 10
+	 * one page size. A 10-chunk image would typically require 10
 	 * transactions.
 	 *
 	 * However, the hardware managing the mailbox has limited