[PATCH] accel/amdxdna: Load the aie4 firmware variant the PSP reports

[PATCH] accel/amdxdna: Load the aie4 firmware variant the PSP reports

[Mario Limonciello]

Some Platforms ship BIOS with either a default or an alternative trust
authority. The PSP validates the NPU firmware, so it must be signed with
the matching scheme. Firmware for the alternative scheme is named with
an "_l" suffix before the extension. A preproduction part needs firmware
of its own, named with a "_p" suffix.

The PSP reports the signing type in bit 6 of C2PMSG_63, and in bit 28
whether this is a preproduction part. A preproduction part that also
reports the alternative scheme is not expected, so warn about it and
use the default firmware.

Add MODULE_FIRMWARE() entries for the "_l" variants of the npu and cert
images of the aie4 devices so that initramfs generators include them.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/accel/amdxdna/Kconfig           |   1 +
 drivers/accel/amdxdna/aie4_pci.c        | 166 ++++++++++++++++++++++++++------
 drivers/accel/amdxdna/amdxdna_pci_drv.c |   8 ++
 3 files changed, 145 insertions(+), 30 deletions(-)

diff --git a/drivers/accel/amdxdna/Kconfig b/drivers/accel/amdxdna/Kconfig
index eddff490b4f75..cd17a89cf4146 100644
--- a/drivers/accel/amdxdna/Kconfig
+++ b/drivers/accel/amdxdna/Kconfig
@@ -6,6 +6,7 @@ config DRM_ACCEL_AMDXDNA
 	depends on DRM_ACCEL
 	depends on PCI && HAS_IOMEM
 	depends on X86_64
+	depends on AMD_NODE
 	depends on AMD_PMF || !AMD_PMF
 	select DRM_SCHED
 	select DRM_GEM_SHMEM_HELPER
diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
index 02dcb2f813d45..fa8f99d1be361 100644
--- a/drivers/accel/amdxdna/aie4_pci.c
+++ b/drivers/accel/amdxdna/aie4_pci.c
@@ -11,6 +11,8 @@
 #include <linux/debugfs.h>
 #include <linux/firmware.h>
 #include <linux/sizes.h>
+#include <linux/string.h>
+#include <asm/amd/node.h>
 
 #include "aie.h"
 #include "aie4_msg_priv.h"
@@ -558,46 +560,128 @@ static void aie4_classic_hw_stop(struct amdxdna_dev_hdl *ndev)
 	aie4_fw_stop(ndev);
 }
 
-static int aie4_request_firmware(struct amdxdna_dev_hdl *ndev,
-				 const struct firmware **npufw,
-				 const struct firmware **certfw)
+/*
+ * PSP C2PMSG_63 (SMN) bit 6: 0 = default firmware signing, 1 = alternative
+ * PSP C2PMSG_63 (SMN) bit 28: 0 = production part, 1 = preproduction part
+ */
+#define AIE4_PSP_C2PMSG_63_SMN_ADDR	0x38109fc
+#define AIE4_PSP_FW_SIGN_ALT		BIT(6)
+#define AIE4_PSP_FW_PREPROD		BIT(28)
+
+enum aie4_fw_variant {
+	AIE4_FW_DEFAULT,
+	AIE4_FW_ALT,		/* "_l" images */
+	AIE4_FW_PREPROD,	/* "_p" images */
+};
+
+/*
+ * The PSP only trusts firmware signed with the scheme the platform firmware
+ * uses and a preproduction part needs firmware of its own. Reading which one is
+ * in use is optional and only used as a hint for which images to try first:
+ * anything but a clear answer is treated as default.
+ */
+static enum aie4_fw_variant aie4_psp_fw_variant(struct amdxdna_dev *xdna)
 {
-	struct amdxdna_dev *xdna = ndev->aie.xdna;
-	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
-	char fw_name[128];
+	u32 val;
 	int ret;
 
-	ret = snprintf(fw_name, sizeof(fw_name), "amdnpu/%04x_%02x/%s",
-		       pdev->device, pdev->revision, ndev->priv->npufw_path);
-	if (ret >= sizeof(fw_name)) {
-		XDNA_ERR(xdna, "npu firmware path is truncated");
-		return -EINVAL;
+	ret = amd_smn_read(0, AIE4_PSP_C2PMSG_63_SMN_ADDR, &val);
+	if (ret) {
+		XDNA_DBG(xdna, "PSP firmware variant not available, ret %d", ret);
+		return AIE4_FW_DEFAULT;
 	}
 
-	ret = request_firmware(npufw, fw_name, &pdev->dev);
-	if (ret) {
-		XDNA_ERR(xdna, "failed to request_firmware %s, ret %d", fw_name, ret);
-		return ret;
+	/* all ones means the register isn't accessible */
+	if (val == 0xffffffff)
+		return AIE4_FW_DEFAULT;
+
+	if ((val & AIE4_PSP_FW_SIGN_ALT) && (val & AIE4_PSP_FW_PREPROD)) {
+		XDNA_WARN(xdna,
+			  "Preproduction part with alternative signing, using default firmware");
+		return AIE4_FW_DEFAULT;
 	}
 
-	ret = snprintf(fw_name, sizeof(fw_name), "amdnpu/%04x_%02x/%s",
-		       pdev->device, pdev->revision, ndev->priv->certfw_path);
-	if (ret >= sizeof(fw_name)) {
-		XDNA_ERR(xdna, "cert firmware path is truncated");
-		ret = -EINVAL;
-		goto release_npufw;
+	if (val & AIE4_PSP_FW_PREPROD)
+		return AIE4_FW_PREPROD;
+
+	return (val & AIE4_PSP_FW_SIGN_ALT) ? AIE4_FW_ALT : AIE4_FW_DEFAULT;
+}
+
+/*
+ * Images for the other variants have a suffix before the ".sbin" extension.
+ * Returns -ENOENT if @path has no such variant.
+ */
+static int aie4_fw_name(struct amdxdna_dev *xdna, char *name, size_t size,
+			const char *path, enum aie4_fw_variant variant)
+{
+	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
+	const char *suffix;
+	size_t len, base;
+	int ret;
+
+	ret = snprintf(name, size, "amdnpu/%04x_%02x/%s",
+		       pdev->device, pdev->revision, path);
+	if (ret >= size) {
+		XDNA_ERR(xdna, "firmware path %s is truncated", path);
+		return -EINVAL;
 	}
 
-	ret = request_firmware(certfw, fw_name, &pdev->dev);
-	if (ret) {
-		XDNA_ERR(xdna, "failed to request_firmware %s, ret %d", fw_name, ret);
-		goto release_npufw;
+	switch (variant) {
+	case AIE4_FW_ALT:
+		suffix = "_l.sbin";
+		break;
+	case AIE4_FW_PREPROD:
+		suffix = "_p.sbin";
+		break;
+	default:
+		return 0;
 	}
 
+	len = ret;
+	if (len < strlen(".sbin") || strcmp(name + len - strlen(".sbin"), ".sbin"))
+		return -ENOENT;
+
+	base = len - strlen(".sbin");
+	if (strscpy(name + base, suffix, size - base) < 0)
+		return -EOVERFLOW;
+
 	return 0;
+}
+
+static int aie4_request_one(struct amdxdna_dev_hdl *ndev, const struct firmware **fw,
+			    const char *path, enum aie4_fw_variant variant)
+{
+	struct amdxdna_dev *xdna = ndev->aie.xdna;
+	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
+	char name[128];
+	int ret;
+
+	ret = aie4_fw_name(xdna, name, sizeof(name), path, variant);
+	if (ret)
+		return ret;
+
+	ret = firmware_request_nowarn(fw, name, &pdev->dev);
+	if (ret)
+		XDNA_DBG(xdna, "firmware %s not loaded, ret %d", name, ret);
+
+	return ret;
+}
+
+/* The NPU and cert images are validated together, so both use the same variant */
+static int aie4_request_firmware(struct amdxdna_dev_hdl *ndev,
+				 const struct firmware **npufw,
+				 const struct firmware **certfw,
+				 enum aie4_fw_variant variant)
+{
+	int ret;
+
+	ret = aie4_request_one(ndev, npufw, ndev->priv->npufw_path, variant);
+	if (ret)
+		return ret;
 
-release_npufw:
-	release_firmware(*npufw);
+	ret = aie4_request_one(ndev, certfw, ndev->priv->certfw_path, variant);
+	if (ret)
+		release_firmware(*npufw);
 
 	return ret;
 }
@@ -645,18 +729,40 @@ static int aie4_prepare_firmware(struct amdxdna_dev_hdl *ndev,
 	return 0;
 }
 
+/*
+ * The firmware can only be started once. It is the PSP that decides whether it
+ * accepts the images, and it does that when the firmware is started, so there
+ * is no trying a variant and falling back. Pick the variant that the PSP
+ * reported (the default one if it can't be read) and use the others only if
+ * its images are missing.
+ */
 static int aie4_load_fw(struct amdxdna_dev_hdl *ndev,
 			void __iomem *tbl[PCI_NUM_RESOURCES])
 {
+	struct amdxdna_dev *xdna = ndev->aie.xdna;
+	enum aie4_fw_variant hint = aie4_psp_fw_variant(xdna);
 	const struct firmware *npufw, *certfw;
-	int ret;
+	enum aie4_fw_variant order[3];
+	int i, n = 0, ret = -ENOENT;
 
 	if (!ndev->priv->npufw_path && !ndev->priv->certfw_path)
 		return 0;
 
-	ret = aie4_request_firmware(ndev, &npufw, &certfw);
-	if (ret)
+	order[n++] = hint;
+	if (hint != AIE4_FW_DEFAULT)
+		order[n++] = AIE4_FW_DEFAULT;
+	if (hint != AIE4_FW_ALT)
+		order[n++] = AIE4_FW_ALT;
+
+	for (i = 0; i < n; i++) {
+		ret = aie4_request_firmware(ndev, &npufw, &certfw, order[i]);
+		if (!ret)
+			break;
+	}
+	if (ret) {
+		XDNA_ERR(xdna, "failed to load firmware, ret %d", ret);
 		return ret;
+	}
 
 	ret = aie4_prepare_firmware(ndev, npufw, certfw, tbl);
 	aie4_release_firmware(ndev, npufw, certfw);
diff --git a/drivers/accel/amdxdna/amdxdna_pci_drv.c b/drivers/accel/amdxdna/amdxdna_pci_drv.c
index f233eb70a2957..dab8603e2381f 100644
--- a/drivers/accel/amdxdna/amdxdna_pci_drv.c
+++ b/drivers/accel/amdxdna/amdxdna_pci_drv.c
@@ -37,6 +37,14 @@ MODULE_FIRMWARE("amdnpu/17f1_13/npu.sbin");
 MODULE_FIRMWARE("amdnpu/17f1_13/cert.sbin");
 MODULE_FIRMWARE("amdnpu/17f2_13/npu.sbin");
 MODULE_FIRMWARE("amdnpu/17f2_13/cert.sbin");
+MODULE_FIRMWARE("amdnpu/17f1_10/npu_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f1_10/cert_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f2_10/npu_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f2_10/cert_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f1_13/npu_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f1_13/cert_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f2_13/npu_l.sbin");
+MODULE_FIRMWARE("amdnpu/17f2_13/cert_l.sbin");
 
 /*
  * 0.0: Initial version

---
base-commit: e32e83a6c5ec031a7f59d4794113d8484b220254
change-id: 20261104-superm1-lms-npu-6bc3bbe38c29

Best regards,
--  
Mario Limonciello <mario.limonciello@amd.com>

Re: [PATCH] accel/amdxdna: Load the aie4 firmware variant the PSP reports

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] accel/amdxdna: uninitialized read in aie4_psp_fw_variant() on SMN failure
--

commit 8b85be20f84a0e02e0435160129ed37c6585968f
Author: Mario Limonciello <mario.limonciello@amd.com>

accel/amdxdna: Load the aie4 firmware variant the PSP reports

This commit reads PSP C2PMSG_63 over SMN to determine whether an AIE4 NPU
requires default, alternative ("_l"), or preproduction ("_p") signed
firmware, and adds MODULE_FIRMWARE() entries for the "_l" images.

> diff --git a/drivers/accel/amdxdna/aie4_pci.c b/drivers/accel/amdxdna/aie4_pci.c
> index 02dcb2f813d45..fa8f99d1be361 100644
> --- a/drivers/accel/amdxdna/aie4_pci.c
> +++ b/drivers/accel/amdxdna/aie4_pci.c

[ ... ]

> @@ -558,46 +560,128 @@ static void aie4_classic_hw_stop(struct amdxdna_dev_hdl *ndev)
>  	aie4_fw_stop(ndev);
>  }
>  
> -static int aie4_request_firmware(struct amdxdna_dev_hdl *ndev,
> -				 const struct firmware **npufw,
> -				 const struct firmware **certfw)
> +/*
> + * PSP C2PMSG_63 (SMN) bit 6: 0 = default firmware signing, 1 = alternative
> + * PSP C2PMSG_63 (SMN) bit 28: 0 = production part, 1 = preproduction part
> + */
> +#define AIE4_PSP_C2PMSG_63_SMN_ADDR	0x38109fc
> +#define AIE4_PSP_FW_SIGN_ALT		BIT(6)
> +#define AIE4_PSP_FW_PREPROD		BIT(28)
> +
> +enum aie4_fw_variant {
> +	AIE4_FW_DEFAULT,
> +	AIE4_FW_ALT,		/* "_l" images */
> +	AIE4_FW_PREPROD,	/* "_p" images */
> +};
> +
> +/*
> + * The PSP only trusts firmware signed with the scheme the platform firmware
> + * uses and a preproduction part needs firmware of its own. Reading which one is
> + * in use is optional and only used as a hint for which images to try first:
> + * anything but a clear answer is treated as default.
> + */
> +static enum aie4_fw_variant aie4_psp_fw_variant(struct amdxdna_dev *xdna)
>  {
> -	struct amdxdna_dev *xdna = ndev->aie.xdna;
> -	struct pci_dev *pdev = to_pci_dev(xdna->ddev.dev);
> -	char fw_name[128];
> +	u32 val;
>  	int ret;
>  
> -	ret = snprintf(fw_name, sizeof(fw_name), "amdnpu/%04x_%02x/%s",
> -		       pdev->device, pdev->revision, ndev->priv->npufw_path);
> -	if (ret >= sizeof(fw_name)) {
> -		XDNA_ERR(xdna, "npu firmware path is truncated");
> -		return -EINVAL;
> +	ret = amd_smn_read(0, AIE4_PSP_C2PMSG_63_SMN_ADDR, &val);
> +	if (ret) {
> +		XDNA_DBG(xdna, "PSP firmware variant not available, ret %d", ret);
> +		return AIE4_FW_DEFAULT;
>  	}

[ ... ]

> @@ -645,18 +729,40 @@ static int aie4_prepare_firmware(struct amdxdna_dev_hdl *ndev,
>  	return 0;
>  }
>  
> +/*
> + * The firmware can only be started once. It is the PSP that decides whether it
> + * accepts the images, and it does that when the firmware is started, so there
> + * is no trying a variant and falling back. Pick the variant that the PSP
> + * reported (the default one if it can't be read) and use the others only if
> + * its images are missing.
> + */
>  static int aie4_load_fw(struct amdxdna_dev_hdl *ndev,
>  			void __iomem *tbl[PCI_NUM_RESOURCES])
>  {
> +	struct amdxdna_dev *xdna = ndev->aie.xdna;
> +	enum aie4_fw_variant hint = aie4_psp_fw_variant(xdna);
>  	const struct firmware *npufw, *certfw;
> -	int ret;
> +	enum aie4_fw_variant order[3];
> +	int i, n = 0, ret = -ENOENT;
>  
>  	if (!ndev->priv->npufw_path && !ndev->priv->certfw_path)
>  		return 0;

[Severity: Medium]
Can calling aie4_psp_fw_variant() here read uninitialized stack memory when
amd_smn_read() fails early?

In aie4_psp_fw_variant(), val is declared uninitialized on the stack and
passed to amd_smn_read(). In arch/x86/kernel/amd_node.c:amd_smn_read(),
*value is unconditionally inspected even when __amd_smn_rw() returns an
error before pci_read_config_dword() writes to *value:

arch/x86/kernel/amd_node.c:amd_smn_read() {
	int err = __amd_smn_rw(SMN_INDEX_OFFSET, SMN_DATA_OFFSET, node, address, value, false);

	if (PCI_POSSIBLE_ERROR(*value)) {
		err = -ENODEV;
		*value = 0;
	}

	return err;
}

When __amd_smn_rw() returns early with -ENODEV (for example when !amd_roots
in a guest VM, or when pci_request_config_region_exclusive() failed during
amd_smn_init()), PCI_POSSIBLE_ERROR(*value) reads the uninitialized stack
variable val.

Additionally, because aie4_load_fw() calls aie4_psp_fw_variant(xdna) in its
variable declarations before checking:

	if (!ndev->priv->npufw_path && !ndev->priv->certfw_path)
		return 0;

probing a VF device (aie4_vf_init() -> aie4m_pcidev_init() -> aie4_load_fw(),
where npu3_dev_vf_priv has both paths set to NULL) will always perform this
SMN read and hit the uninitialized read in a guest VM before immediately
returning 0.

Should val be initialized in aie4_psp_fw_variant() (e.g., u32 val = 0;), and
should the aie4_psp_fw_variant(xdna) call in aie4_load_fw() be moved after
the npufw_path/certfw_path check?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104-superm1-lms-npu-v1-1-dbffcc5abc23@amd.com?part=1