Re: [PATCH 5/6] x86/microcode/intel_staging: Support mailbox data transfer

[Dave Hansen <dave.hansen@intel.com>]

On 12/10/24 17:42, Chang S. Bae wrote:
> The staging architecture features a narrowed interface for data transfer.
> Instead of allocating MMIO space based on data chunk size, it utilizes
> two data registers: one for reading and one for writing, enforcing the
> serialization of read and write operations. Additionally, it defines a
> mailbox data format.

So I'm going back and reading this _after_ all of the code. I don't
think I comprehended what "narrowed interface" or "serialization" really
meant when I read this. I was thinking "serializing instructions".

Maybe something like this would help:

Let's say you want to write 2 bytes of data to a device. Typically, the
device would expose 2 bytes of "wide" MMIO space. To send the data to
the device, you could do:

	writeb(buf[0], io_addr + 0);
	writeb(buf[1], io_addr + 1);

But this mailbox is a bit different. Instead of having a "wide"
interface where there is separate MMIO space for each word in a
transaction, it has a "narrow" interface where several words are written
to the same spot in MMIO space:

	writeb(buf[0], io_addr);
	writeb(buf[1], io_addr);

The same goes for the read side.

To me, it's much more akin to talking over a serial port than how I
think of devices attached via MMIO.

> To facilitate data transfer, implement helper functions in line with this
> specified format for reading and writing staging data. This mailbox
> format is a customized version and is not compatible with the existing
> mailbox code, so reuse is not feasible.

This is getting a bit too flowery.

	Implement helper functions that send and receive data to and
	from the device.

	Note: the kernel has support for similar mailboxes. But none of
	them are compatible with this one. Trying to share code resulted
	in a bloated mess, so this code is standalone.

> diff --git a/arch/x86/kernel/cpu/microcode/intel_staging.c b/arch/x86/kernel/cpu/microcode/intel_staging.c
> index 2fc8667cab45..eab6e891db9c 100644
> --- a/arch/x86/kernel/cpu/microcode/intel_staging.c
> +++ b/arch/x86/kernel/cpu/microcode/intel_staging.c
> @@ -3,6 +3,7 @@
>  #define pr_fmt(fmt) "microcode: " fmt
>  #include <linux/delay.h>
>  #include <linux/io.h>
> +#include <linux/pci_ids.h>
>  
>  #include "internal.h"
>  
> @@ -11,17 +12,44 @@
>  
>  #define MBOX_CONTROL_OFFSET	0x0
>  #define MBOX_STATUS_OFFSET	0x4
> +#define MBOX_WRDATA_OFFSET	0x8
> +#define MBOX_RDDATA_OFFSET	0xc
>  
>  #define MASK_MBOX_CTRL_ABORT	BIT(0)
> +#define MASK_MBOX_CTRL_GO	BIT(31)
>  
>  #define MASK_MBOX_STATUS_ERROR	BIT(2)
>  #define MASK_MBOX_STATUS_READY	BIT(31)
>  
> +#define MASK_MBOX_RESP_SUCCESS	BIT(0)
> +#define MASK_MBOX_RESP_PROGRESS	BIT(1)
> +#define MASK_MBOX_RESP_ERROR	BIT(2)
> +
> +#define MBOX_CMD_LOAD		0x3
> +#define MBOX_OBJ_STAGING	0xb
> +#define MBOX_HDR		(PCI_VENDOR_ID_INTEL | (MBOX_OBJ_STAGING << 16))
> +#define MBOX_HDR_SIZE		16
> +
>  #define MBOX_XACTION_LEN	PAGE_SIZE
>  #define MBOX_XACTION_MAX(imgsz)	((imgsz) * 2)
>  #define MBOX_XACTION_TIMEOUT	(10 * MSEC_PER_SEC)
>  
>  #define STAGING_OFFSET_END	0xffffffff
> +#define DWORD_SIZE(s)		((s) / sizeof(u32))
> +
> +static inline u32 read_mbox_dword(void __iomem *base)
> +{
> +	u32 dword = readl(base + MBOX_RDDATA_OFFSET);
> +
> +	/* Inform the read completion to the staging firmware */
> +	writel(0, base + MBOX_RDDATA_OFFSET);
> +	return dword;
> +}

That comment doesn't quite parse for me.  Maybe:

	/* Inform the staging firmware that the read is complete: */

BTW, is this kind of read/write normal for MMIO reads? It looks really
goofy to me, but I don't deal with devices much.

> +static inline void write_mbox_dword(void __iomem *base, u32 dword)
> +{
> +	writel(dword, base + MBOX_WRDATA_OFFSET);
> +}
>  
>  static inline void abort_xaction(void __iomem *base)
>  {
> @@ -30,7 +58,18 @@ static inline void abort_xaction(void __iomem *base)
>  
>  static void request_xaction(void __iomem *base, u32 *chunk, unsigned int chunksize)
>  {
> -	pr_debug_once("Need to implement staging mailbox loading code.\n");
> +	unsigned int i, dwsize = DWORD_SIZE(chunksize);
> +
> +	write_mbox_dword(base, MBOX_HDR);
> +	write_mbox_dword(base, dwsize + DWORD_SIZE(MBOX_HDR_SIZE));
> +
> +	write_mbox_dword(base, MBOX_CMD_LOAD);
> +	write_mbox_dword(base, 0);

This last write is a mystery. Why is it here?

> +
> +	for (i = 0; i < dwsize; i++)
> +		write_mbox_dword(base, chunk[i]);

So, again, I'm a device dummy here. But this _looks_ nonsensical like
the code is just scribbling over itself repeatedly by rewriting data at
'base' over and over.

Would a comment like this help?

/*
 * 'base' is mapped UnCached (UC). Each write shows up at the device
 * as a separate "packet" in program order. That property allows the
 * device to reassemble everything in order on its side.
 */

> +	writel(MASK_MBOX_CTRL_GO, base + MBOX_CONTROL_OFFSET);
>  }

Can we comment the MASK_MBOX_CTRL_GO?  Maybe:

	/*
	 * Tell the device that this chunk is
	 * complete and can be processed.
	 */

>  static enum ucode_state wait_for_xaction(void __iomem *base)
> @@ -55,8 +94,18 @@ static enum ucode_state wait_for_xaction(void __iomem *base)
>  
>  static enum ucode_state read_xaction_response(void __iomem *base, unsigned int *offset)
>  {
> -	pr_debug_once("Need to implement staging response handler.\n");
> -	return UCODE_ERROR;
> +	u32 flag;

	/*
	 * All responses begin with the same header
	 * word and are the same length: 4 dwords.
	 */
	
> +	WARN_ON_ONCE(read_mbox_dword(base) != MBOX_HDR);
> +	WARN_ON_ONCE(read_mbox_dword(base) != DWORD_SIZE(MBOX_HDR_SIZE));
> +
> +	*offset = read_mbox_dword(base);
> +
> +	flag = read_mbox_dword(base);
> +	if (flag & MASK_MBOX_RESP_ERROR)
> +		return UCODE_ERROR;
> +
> +	return UCODE_OK;
>  }
>  
>  static inline unsigned int get_chunksize(unsigned int totalsize, unsigned int offset)