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

[Mario Limonciello <mario.limonciello@amd.com>]

On 11/5/26 13:19, Zhang, Yidong (David) wrote:
> 
> On 11/5/2026 7:51 AM, Mario Limonciello wrote:
>> 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>
>> ---
>> Changes in v2:
>> - Initialize val in aie4_psp_fw_variant() so a failed SMN read can't 
>> leave
>>    it uninitialized
>> - Only read the PSP firmware variant after the npufw/certfw path 
>> check, so
>>    VFs (which have no firmware paths) skip the SMN access
>> - Drop the redundant all-ones check, amd_smn_read() already returns - 
>> ENODEV
>> - Link to v1: https://patch.msgid.link/20261104-superm1-lms-npu-v1-1- 
>> dbffcc5abc23@amd.com
>> ---
>>   drivers/accel/amdxdna/Kconfig           |   1 +
>>   drivers/accel/amdxdna/aie4_pci.c        | 162 ++++++++++++++++++++++ 
>> ++++------
>>   drivers/accel/amdxdna/amdxdna_pci_drv.c |   8 ++
>>   3 files changed, 141 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..26c6162893760 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,124 @@ 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 = 0;
>>       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;
>> +    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;
>> -release_npufw:
>> -    release_firmware(*npufw);
>> +    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;
>> +
>> +    ret = aie4_request_one(ndev, certfw, ndev->priv->certfw_path, 
>> variant);
>> +    if (ret)
>> +        release_firmware(*npufw);
>>       return ret;
>>   }
>> @@ -645,18 +725,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;
>>       const struct firmware *npufw, *certfw;
>> -    int ret;
>> +    enum aie4_fw_variant order[3], hint;
>> +    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)
>> +    hint = aie4_psp_fw_variant(xdna);
>> +    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");
> 
> 
> I tested against both _l, _p and default sbins. All passed.
> 
> Reviewed-by: David Zhang <yidong.zhang@amd.com>
> 
> Tested-by: David Zhang <yidong.zhang@amd.com>
> 
> /David
> 
>>   /*
>>    * 0.0: Initial version
>>
>> ---
>> base-commit: e32e83a6c5ec031a7f59d4794113d8484b220254
>> change-id: 20261104-superm1-lms-npu-6bc3bbe38c29
>>
>> Best regards,
>> -- 
>> Mario Limonciello <mario.limonciello@amd.com>

applied to drm-misc-next