Message-ID: <20250912163421.GBaMRLjTUr3bcG5fvJ@fat_crate.local>
Date: Fri, 12 Sep 2025 18:34:21 +0200
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, chao.gao@...el.com,
	abusse@...zon.de
Subject: Re: [PATCH v5 6/7] x86/microcode/intel: Support mailbox transfer

On Sat, Aug 23, 2025 at 08:52:09AM -0700, Chang S. Bae wrote:
> Previously, the functions for sending microcode data and retrieving the
> next offset were placeholders, as they required handling the specific
> mailbox format. Implement them as following:
> 
> == Mailbox Format ==
> 
> The staging mailbox consists of two primary sections: 'header' and
> 'data'. While the microcode must be transferred following this format,
> the actual data transfer mechanism involves reading and writing to
> specific MMIO registers.
> 
> == Mailbox Data Registers ==
> 
> Unlike conventional interfaces that allocate MMIO space for each data
> chunk, the staging interface features a "narrow" interface, using only
> two dword-sized registers for read and write operations.
> 
> For example, if writing 2 dwords of data to a device. Typically, the
						      ^
						      |

abrupt sentence end. Looks like the idea was to not have a fullstop there but
continue...

> device would expose 2 dwords of "wide" MMIO space. To send the data to
> the device:
> 
> 	writel(buf[0], io_addr + 0);
> 	writel(buf[1], io_addr + 1);
> 
> But, this interface 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:
> 
> 	writel(buf[0], io_addr);
> 	writel(buf[1], io_addr);
> 
> The same goes for the read side.

I don't understand what the point of this explanation is if all you end up
doing is writing to "io_addr". Why introduce the unnecessary confusion?

> 
> == Implementation Summary ==
> 
> Given that, introduce two layers of helper functions at first:
> 
>   * Low-level helpers for reading and writing to data registers directly.
>   * Wrapper functions for handling mailbox header and data sections.
> 
> Using them, implement send_data_chunk() and fetch_next_offset()
> functions. Add explicit error and timeout handling routine in
> wait_for_transaction(), finishing up the transfer.
>
> Both hardware error states and implicit errors -- invalid header or
> offset -- result in UCODE_ERROR. Emit a clear message for the latter.a

Can we pu-lease stop explaining what the code does?!

Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.
 
> 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.

Now that belongs in a commit message. Stuff which is non-obvious etc.

> +static void write_mbox_data(void __iomem *mmio_base, u32 *chunk, unsigned int chunk_bytes)
> +{
> +	int i;
> +
> +	/*
> +	 * The MMIO space is mapped as Uncached (UC). Each write arrives
> +	 * at the device as an individual transaction in program order.
> +	 * The device can then resemble the sequence accordingly.

reassemble?

> +	 */
> +	for (i = 0; i < chunk_bytes / sizeof(u32); i++)
> +		write_mbox_dword(mmio_base, chunk[i]);
> +}
> +
>  /*
>   * Prepare for a new microcode transfer: reset hardware and record the
>   * image size.

...

> +static int wait_for_transaction(struct staging_state *ss)
> +{
> +	u32 timeout, status;
> +
> +	/* Allow time for hardware to complete the operation: */
> +	for (timeout = 0; timeout < MBOX_XACTION_TIMEOUT_MS; timeout++) {
> +		msleep(1);
> +
> +		status = readl(ss->mmio_base + MBOX_STATUS_OFFSET);
> +		/* Break out early if the hardware is ready: */
> +		if (status & MASK_MBOX_STATUS_READY)
> +			break;
> +	}
> +
> +	/* Check for explicit error response */
> +	if (status & MASK_MBOX_STATUS_ERROR) {
> +		ss->state = UCODE_ERROR;
> +		return -EPROTO;
> +	}
> +
> +	/*
> +	 * Hardware is neither responded to the action nor signaled any

s/is/has/

> +	 * error. Treat this as timeout.
> +	 */
> +	if (!(status & MASK_MBOX_STATUS_READY)) {
> +		ss->state = UCODE_TIMEOUT;
> +		return -ETIMEDOUT;
> +	}
> +
> +	ss->state = UCODE_OK;
> +	return 0;
>  }

...

> @@ -412,9 +542,51 @@ static int send_data_chunk(struct staging_state *ss, void *ucode_ptr __maybe_unu
>   */
>  static int fetch_next_offset(struct staging_state *ss)
>  {
> -	pr_debug_once("Staging mailbox response handling code needs to be implemented.\n\n");
> +	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:
> +	 *  1. Header
> +	 *  2. Next offset in the microcode image
> +	 *  3. Status flags
> +	 */
> +	header = read_mbox_header(ss->mmio_base);
> +	offset = read_mbox_dword(ss->mmio_base);
> +	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;
> +	}
> +
> +	/*
> +	 * 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");

You might want to dump some of the actual values in those pr_* statements so
that it dumps more breadcrumbs and those prints are more useful when debugging
issues.

-- 
Regards/Gruss,
    Boris.

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

Powered by blists - more mailing lists