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

[Lizhi Hou <lizhi.hou@amd.com>]

On 11/5/26 09:52, Mario Limonciello wrote:
>
>
> On 11/5/26 11:47, Lizhi Hou wrote:
>>
>> On 11/5/26 07:51, 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");
>> need to add entries for "_p" images.
>
> Let me tell you my thought process in excluding them.
>
> They are only for pre-prod hardware, and so *most people* who use this 
> driver after the product launches shouldn't need them in their 
> initramfs; they'll just bloat the initramfs.
>
> If you think they should still be there; I'll spin a v3 an add them.

Ok, sounds good to me.

Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>

Lizhi

>
>>>   /*
>>>    * 0.0: Initial version
>>>
>>> ---
>>> base-commit: e32e83a6c5ec031a7f59d4794113d8484b220254
>>> change-id: 20261104-superm1-lms-npu-6bc3bbe38c29
>>>
>>> Best regards,
>>> -- 
>>> Mario Limonciello <mario.limonciello@amd.com>
>>>
>