Re: [PATCH V2 2/5] accel/amd_vpci: Add new driver for AMD Versal PCI accelerator

[Yidong Zhang <yidong.zhang@amd.com>]

Hi Mario,

On 1/26/26 11:30, Mario Limonciello wrote:
> 
> 
> On 1/26/2026 1:27 PM, David Zhang wrote:
>> This patch introduces a new PCI driver for AMD Versal-based accelerator
>> cards.
>>
>> The driver provides basic module and PCI device initialization, based on
>> BAR resources used to establish a hardware queue-based ring buffer 
>> between
>> the PCIe host and the Versal Management Runtime (VMR) service running on
>> the embedded SoC. This interface enables firmware management and board
>> health monitoring.
>>
>> Key features:
>>     - PCI probe and BAR resource initialization.
>>     - Integration with configfs for firmware management
>>     - Compatibility check using firmware-reported UUIDs
>>
>> The base firmware image is expected under /lib/firmware/xilinx/<fw_name>
>> and can be programmed to the device through the configfs interface.
>> Firmware transfer is handled via a remote queue service (added in a later
>> patch).
>>
>> Co-developed-by: DMG Karthik <Karthik.DMG@amd.com>
>> Signed-off-by: DMG Karthik <Karthik.DMG@amd.com>
>> Co-developed-by: Nishad Saraf <nishads@amd.com>
>> Signed-off-by: Nishad Saraf <nishads@amd.com>
>> Signed-off-by: David Zhang <yidong.zhang@amd.com>
>> ---
>>   MAINTAINERS                              |   5 +
>>   drivers/accel/Kconfig                    |   1 +
>>   drivers/accel/Makefile                   |   3 +-
>>   drivers/accel/amd_vpci/Kconfig           |  15 ++
>>   drivers/accel/amd_vpci/Makefile          |   6 +
>>   drivers/accel/amd_vpci/versal-pci-main.c | 280 +++++++++++++++++++++++
>>   drivers/accel/amd_vpci/versal-pci.h      |  62 +++++
>>   7 files changed, 371 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/accel/amd_vpci/Kconfig
>>   create mode 100644 drivers/accel/amd_vpci/Makefile
>>   create mode 100644 drivers/accel/amd_vpci/versal-pci-main.c
>>   create mode 100644 drivers/accel/amd_vpci/versal-pci.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 6863d5fa07a1..8fb7276eb7c1 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -1271,6 +1271,11 @@ F:    drivers/spi/spi-amd-pci.c
>>   F:    drivers/spi/spi-amd.c
>>   F:    drivers/spi/spi-amd.h
>> +AMD VERSAL PCI DRIVER
>> +M:    Yidong Zhang <yidong.zhang@amd.com>
>> +S:    Supported
>> +F:    drivers/accel/amd_vpci/
>> +
>>   AMD XDNA DRIVER
>>   M:    Min Ma <mamin506@gmail.com>
>>   M:    Lizhi Hou <lizhi.hou@amd.com>
>> diff --git a/drivers/accel/Kconfig b/drivers/accel/Kconfig
>> index bdf48ccafcf2..f80998fdb3fc 100644
>> --- a/drivers/accel/Kconfig
>> +++ b/drivers/accel/Kconfig
>> @@ -25,6 +25,7 @@ menuconfig DRM_ACCEL
>>         and debugfs).
>>   source "drivers/accel/amdxdna/Kconfig"
>> +source "drivers/accel/amd_vpci/Kconfig"
>>   source "drivers/accel/ethosu/Kconfig"
>>   source "drivers/accel/habanalabs/Kconfig"
>>   source "drivers/accel/ivpu/Kconfig"
>> diff --git a/drivers/accel/Makefile b/drivers/accel/Makefile
>> index 1d3a7251b950..be62e08bbef1 100644
>> --- a/drivers/accel/Makefile
>> +++ b/drivers/accel/Makefile
>> @@ -1,8 +1,9 @@
>>   # SPDX-License-Identifier: GPL-2.0-only
>>   obj-$(CONFIG_DRM_ACCEL_AMDXDNA)        += amdxdna/
>> +obj-$(CONFIG_DRM_ACCEL_AMD_VPCI)    += amd_vpci/
>>   obj-$(CONFIG_DRM_ACCEL_ARM_ETHOSU)    += ethosu/
>>   obj-$(CONFIG_DRM_ACCEL_HABANALABS)    += habanalabs/
>>   obj-$(CONFIG_DRM_ACCEL_IVPU)        += ivpu/
>>   obj-$(CONFIG_DRM_ACCEL_QAIC)        += qaic/
>> -obj-$(CONFIG_DRM_ACCEL_ROCKET)        += rocket/
>> \ No newline at end of file
>> +obj-$(CONFIG_DRM_ACCEL_ROCKET)        += rocket/
>> diff --git a/drivers/accel/amd_vpci/Kconfig 
>> b/drivers/accel/amd_vpci/Kconfig
>> new file mode 100644
>> index 000000000000..dcf83bf3d8e6
>> --- /dev/null
>> +++ b/drivers/accel/amd_vpci/Kconfig
>> @@ -0,0 +1,15 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +config DRM_ACCEL_AMD_VPCI
>> +    tristate "AMD Versal PCIe Management Driver"
>> +    depends on DRM_ACCEL
>> +    depends on PCI && HAS_IOMEM
>> +    select FW_LOADER
>> +    select CONFIGFS_FS
>> +    default m
>> +    help
>> +      AMD Versal PCIe Management Driver provides management services,
>> +      including download firmware, program bitstream, and communicate 
>> with
>> +      the User function.
>> +
>> +      If "M" is selected, the driver module will be versal-pci
>> diff --git a/drivers/accel/amd_vpci/Makefile 
>> b/drivers/accel/amd_vpci/Makefile
>> new file mode 100644
>> index 000000000000..03849875ad0b
>> --- /dev/null
>> +++ b/drivers/accel/amd_vpci/Makefile
>> @@ -0,0 +1,6 @@
>> +# SPDX-License-Identifier: GPL-2.0+
>> +
>> +obj-$(CONFIG_DRM_ACCEL_AMD_VPCI) := versal-pci.o
>> +
>> +versal-pci-y := \
>> +    versal-pci-main.o
>> diff --git a/drivers/accel/amd_vpci/versal-pci-main.c 
>> b/drivers/accel/amd_vpci/versal-pci-main.c
>> new file mode 100644
>> index 000000000000..36f88d5aee95
>> --- /dev/null
>> +++ b/drivers/accel/amd_vpci/versal-pci-main.c
>> @@ -0,0 +1,280 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Driver for Versal PCIe device
>> + *
>> + * Copyright (C) 2026 Advanced Micro Devices, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/pci.h>
>> +
>> +#include "versal-pci.h"
>> +
>> +#define DRV_NAME            "amd-versal-pci"
>> +
>> +#define PCI_DEVICE_ID_V70PQ2        0x50B0
>> +#define PCI_DEVICE_ID_RAVE        0x5700
>> +#define VERSAL_XCLBIN_MAGIC_ID        "xclbin2"
>> +
>> +static inline u32 versal_pci_devid(struct versal_pci_device *vdev)
>> +{
>> +    return ((pci_domain_nr(vdev->pdev->bus) << 16) |
>> +        PCI_DEVID(vdev->pdev->bus->number, vdev->pdev->devfn));
>> +}
>> +
>> +static int versal_pci_load_shell(struct versal_pci_device *vdev, char 
>> *fw_name)
>> +{
>> +    const struct firmware *fw;
>> +    struct axlf *xsabin;
>> +    int ret;
>> +
>> +    strim(fw_name);
>> +
>> +    ret = request_firmware(&fw, fw_name, &vdev->pdev->dev);
>> +    if (ret) {
>> +        vdev_warn(vdev, "request xsabin fw %s failed %d", fw_name, ret);
>> +        return ret;
>> +    }
>> +
>> +    xsabin = (struct axlf *)fw->data;
>> +    if (memcmp(xsabin->magic, VERSAL_XCLBIN_MAGIC_ID, 
>> strlen(VERSAL_XCLBIN_MAGIC_ID))) {
>> +        vdev_err(vdev, "Invalid device firmware");
>> +        ret = -EINVAL;
>> +        goto release_firmware;
>> +    }
>> +
>> +    if (!fw->size ||
>> +        fw->size != xsabin->header.length ||
>> +        fw->size < sizeof(*xsabin) ||
>> +        fw->size > SZ_1G) {
>> +        vdev_err(vdev, "Invalid device firmware size %zu", fw->size);
>> +        ret = -EINVAL;
>> +        goto release_firmware;
>> +    }
>> +
>> +    if (!uuid_equal(&vdev->intf_uuid, &xsabin->header.rom_uuid)) {
>> +        vdev_err(vdev, "base shell doesn't match uuid %pUb", 
>> &xsabin->header.rom_uuid);
>> +        ret = -EINVAL;
>> +        goto release_firmware;
>> +    }
>> +
>> +    /* TODO upload fw to card */
>> +    if (ret) {
>> +        vdev_err(vdev, "failed to load xsabin %s : %d", fw_name, ret);
>> +        goto release_firmware;
>> +    }
>> +
>> +    vdev_info(vdev, "Downloaded xsabin %pUb of size %lld Bytes",
>> +          &xsabin->header.uuid, xsabin->header.length);
>> +
>> +release_firmware:
>> +    release_firmware(fw);
>> +
>> +    return ret;
>> +}
>> +
>> +static inline struct versal_pci_device *item_to_vdev(struct 
>> config_item *item)
>> +{
>> +    return container_of(to_configfs_subsystem(to_config_group(item)),
>> +                struct versal_pci_device, cfs_subsys);
>> +}
>> +
>> +static ssize_t versal_pci_cfs_config_store(struct config_item *item,
>> +                       const char *page, size_t count)
>> +{
>> +    struct versal_pci_device *vdev = item_to_vdev(item);
>> +    u32 config;
>> +    int ret;
>> +
>> +    ret = kstrtou32(page, 0, &config);
>> +    if (ret)
>> +        return -EINVAL;
>> +
>> +    if (config)
>> +        ret = versal_pci_load_shell(vdev, vdev->fw.name);
>> +
>> +    if (ret)
>> +        return -EFAULT;
>> +
>> +    return count;
>> +}
>> +CONFIGFS_ATTR_WO(versal_pci_cfs_, config);
>> +
>> +static ssize_t versal_pci_cfs_image_show(struct config_item *item, 
>> char *page)
>> +{
>> +    struct versal_pci_device *vdev = item_to_vdev(item);
>> +
>> +    vdev_info(vdev, "fw name: %s", vdev->fw.name);
>> +
>> +    return 0;
>> +}
>> +
>> +static ssize_t versal_pci_cfs_image_store(struct config_item *item,
>> +                      const char *page, size_t count)
>> +{
>> +    struct versal_pci_device *vdev = item_to_vdev(item);
>> +
>> +    count = snprintf(vdev->fw.name, sizeof(vdev->fw.name), "%s", page);
>> +
>> +    vdev_info(vdev, "fw name: %s", vdev->fw.name);
>> +    return count;
>> +}
>> +CONFIGFS_ATTR(versal_pci_cfs_, image);
>> +
>> +static struct configfs_attribute *versal_pci_cfs_attrs[] = {
>> +    &versal_pci_cfs_attr_config,
>> +    &versal_pci_cfs_attr_image,
>> +    NULL,
>> +};
>> +
>> +static const struct config_item_type versal_pci_cfs_table = {
>> +    .ct_owner = THIS_MODULE,
>> +    .ct_attrs = versal_pci_cfs_attrs,
>> +};
>> +
>> +static int versal_pci_cfs_init(struct versal_pci_device *vdev)
>> +{
>> +    struct configfs_subsystem *subsys = &vdev->cfs_subsys;
>> +
>> +    snprintf(subsys->su_group.cg_item.ci_namebuf,
>> +         sizeof(subsys->su_group.cg_item.ci_namebuf),
>> +         "%s%x", DRV_NAME, versal_pci_devid(vdev));
>> +
>> +    subsys->su_group.cg_item.ci_type = &versal_pci_cfs_table;
>> +
>> +    config_group_init(&subsys->su_group);
>> +    return configfs_register_subsystem(subsys);
>> +}
>> +
>> +static void versal_pci_fw_fini(struct versal_pci_device *vdev)
>> +{
>> +    uuid_copy(&vdev->intf_uuid, &uuid_null);
>> +}
>> +
>> +static void versal_pci_cfs_fini(struct configfs_subsystem *subsys)
>> +{
>> +    configfs_unregister_subsystem(subsys);
>> +}
>> +
>> +static void versal_pci_device_teardown(struct versal_pci_device *vdev)
>> +{
>> +    versal_pci_cfs_fini(&vdev->cfs_subsys);
>> +    versal_pci_fw_fini(vdev);
>> +}
>> +
>> +static int versal_pci_uuid_parse(struct versal_pci_device *vdev, 
>> uuid_t *uuid)
>> +{
>> +    char str[UUID_STRING_LEN];
>> +    u8 i, j;
>> +    int len = strlen(vdev->fw_id);
>> +
>> +    /* parse uuid into a valid uuid string format */
>> +    for (i = 0, j = 0; i < len && i < sizeof(str); i++) {
>> +        str[j++] = vdev->fw_id[i];
>> +        if (j == 8 || j == 13 || j == 18 || j == 23)
>> +            str[j++] = '-';
>> +    }
>> +
>> +    if (uuid_parse(str, uuid)) {
>> +        vdev_warn(vdev, "Invalid fw_id format");
>> +        return -EINVAL;
>> +    }
>> +
>> +    vdev_info(vdev, "Interface uuid %pU", uuid);
> 
> Is the UUID from the ROM not considered sensitive?  The system UUID is.
> 
> ❮ cat /sys/class/dmi/id/product_uuid
> cat: /sys/class/dmi/id/product_uuid: Permission denied
> 
> Besides this comment some errors are showing it too AFAICT.

Thanks for catching this out. I will remove all UUID messages from the 
debug message.

Thanks,
David

> 
>> +    return 0;
>> +}
>> +
>> +static int versal_pci_fw_init(struct versal_pci_device *vdev)
>> +{
>> +    int ret;
>> +
>> +    /* TODO request compatible fw_id from card */
>> +
>> +    ret = versal_pci_uuid_parse(vdev, &vdev->intf_uuid);
>> +
>> +    return ret;
>> +}
>> +
>> +static int versal_pci_device_setup(struct versal_pci_device *vdev)
>> +{
>> +    int ret;
>> +
>> +    ret = versal_pci_fw_init(vdev);
>> +    if (ret) {
>> +        vdev_err(vdev, "Failed to init fw, err %d", ret);
>> +        goto comm_chan_fini;
>> +    }
>> +
>> +    ret = versal_pci_cfs_init(vdev);
>> +    if (ret) {
>> +        vdev_err(vdev, "Failed to init configfs subsys, err %d", ret);
>> +        goto comm_chan_fini;
>> +    }
>> +
>> +    return 0;
>> +
>> +comm_chan_fini:
>> +    versal_pci_fw_fini(vdev);
>> +
>> +    return ret;
>> +}
>> +
>> +static void versal_pci_remove(struct pci_dev *pdev)
>> +{
>> +    struct versal_pci_device *vdev = pci_get_drvdata(pdev);
>> +
>> +    versal_pci_device_teardown(vdev);
>> +}
>> +
>> +static int versal_pci_probe(struct pci_dev *pdev, const struct 
>> pci_device_id *pdev_id)
>> +{
>> +    struct versal_pci_device *vdev;
>> +    int ret;
>> +
>> +    vdev = devm_kzalloc(&pdev->dev, sizeof(*vdev), GFP_KERNEL);
>> +    if (!vdev)
>> +        return -ENOMEM;
>> +
>> +    pci_set_drvdata(pdev, vdev);
>> +    vdev->pdev = pdev;
>> +
>> +    ret = pcim_enable_device(pdev);
>> +    if (ret) {
>> +        vdev_err(vdev, "Failed to enable device %d", ret);
>> +        return ret;
>> +    }
>> +
>> +    vdev->io_regs = pcim_iomap_region(vdev->pdev, MGMT_BAR, DRV_NAME);
>> +    if (IS_ERR(vdev->io_regs)) {
>> +        vdev_err(vdev, "Failed to map RM shared memory BAR%d", 
>> MGMT_BAR);
>> +        return PTR_ERR(vdev->io_regs);
>> +    }
>> +
>> +    ret = versal_pci_device_setup(vdev);
>> +    if (ret) {
>> +        vdev_err(vdev, "Failed to setup Versal device %d", ret);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct pci_device_id versal_pci_ids[] = {
>> +    { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_V70PQ2), },
>> +    { PCI_DEVICE(PCI_VENDOR_ID_XILINX, PCI_DEVICE_ID_RAVE), },
>> +    { 0 }
>> +};
>> +
>> +MODULE_DEVICE_TABLE(pci, versal_pci_ids);
>> +
>> +static struct pci_driver versal_pci_driver = {
>> +    .name = DRV_NAME,
>> +    .id_table = versal_pci_ids,
>> +    .probe = versal_pci_probe,
>> +    .remove = versal_pci_remove,
>> +};
>> +
>> +module_pci_driver(versal_pci_driver);
>> +
>> +MODULE_DESCRIPTION("AMD Versal PCIe Management Driver");
>> +MODULE_AUTHOR("XRT Team <runtimeca39d@amd.com>");
>> +MODULE_LICENSE("GPL");
>> diff --git a/drivers/accel/amd_vpci/versal-pci.h 
>> b/drivers/accel/amd_vpci/versal-pci.h
>> new file mode 100644
>> index 000000000000..890da1d6bcc9
>> --- /dev/null
>> +++ b/drivers/accel/amd_vpci/versal-pci.h
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/*
>> + * Driver for Versal PCIe device
>> + *
>> + * Copyright (C) 2026 Advanced Micro Devices, Inc. All rights reserved.
>> + */
>> +
>> +#ifndef __VERSAL_PCI_H
>> +#define __VERSAL_PCI_H
>> +
>> +#include <linux/configfs.h>
>> +#include <linux/firmware.h>
>> +
>> +#define MGMT_BAR        0
>> +
>> +#define vdev_info(vdev, fmt, args...)                    \
>> +    dev_info(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
>> +
>> +#define vdev_warn(vdev, fmt, args...)                    \
>> +    dev_warn(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
>> +
>> +#define vdev_err(vdev, fmt, args...)                    \
>> +    dev_err(&(vdev)->pdev->dev, "%s: "fmt, __func__, ##args)
>> +
>> +#define vdev_dbg(vdev, fmt, args...)                    \
>> +    dev_dbg(&(vdev)->pdev->dev, fmt, ##args)
>> +
>> +struct versal_pci_device;
>> +
>> +struct axlf_header {
>> +    __u64                length;
>> +    __u8                reserved1[24];
>> +    uuid_t                rom_uuid;
>> +    __u8                reserved2[64];
>> +    uuid_t                uuid;
>> +    __u8                reserved3[24];
>> +} __packed;
>> +
>> +struct axlf {
>> +    __u8                magic[8];
>> +    __u8                reserved[296];
>> +    struct axlf_header        header;
>> +} __packed;
>> +
>> +struct fw_info {
>> +    __u32                opcode;
>> +    char                name[128];
>> +};
>> +
>> +struct versal_pci_device {
>> +    struct pci_dev            *pdev;
>> +
>> +    struct fw_info            fw;
>> +
>> +    void __iomem            *io_regs;
>> +    uuid_t                intf_uuid;
>> +    __u8                fw_id[UUID_STRING_LEN + 1];
>> +
>> +    struct configfs_subsystem    cfs_subsys;
>> +};
>> +
>> +#endif    /* __VERSAL_PCI_H */
>