| 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
| ||
|
Message-ID: <fac46937-e0a5-42c1-96ee-65fec4e17551@intel.com>
Date: Tue, 18 Feb 2025 12:54:19 -0800
From: Dave Hansen <dave.hansen@...el.com>
To: "Chang S. Bae" <chang.seok.bae@...el.com>, linux-kernel@...r.kernel.org
Cc: x86@...nel.org, tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
dave.hansen@...ux.intel.com
Subject: Re: [PATCH 5/6] x86/microcode/intel_staging: Support mailbox data
transfer
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)
Powered by blists - more mailing lists