Message-ID: <3981d528-140c-42c7-94b7-007a84c1476f@amd.com>
Date: Mon, 26 Jan 2026 13:30:36 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: David Zhang <yidong.zhang@....com>, ogabbay@...nel.org,
 quic_jhugo@...cinc.com, maciej.falkowski@...ux.intel.com,
 dri-devel@...ts.freedesktop.org
Cc: linux-kernel@...r.kernel.org, sonal.santan@....com, lizhi.hou@....com,
 DMG Karthik <Karthik.DMG@....com>, Nishad Saraf <nishads@....com>
Subject: Re: [PATCH V2 2/5] accel/amd_vpci: Add new driver for AMD Versal PCI
 accelerator



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@....com>
> Signed-off-by: DMG Karthik <Karthik.DMG@....com>
> Co-developed-by: Nishad Saraf <nishads@....com>
> Signed-off-by: Nishad Saraf <nishads@....com>
> Signed-off-by: David Zhang <yidong.zhang@....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@....com>
> +S:	Supported
> +F:	drivers/accel/amd_vpci/
> +
>   AMD XDNA DRIVER
>   M:	Min Ma <mamin506@...il.com>
>   M:	Lizhi Hou <lizhi.hou@....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.

> +	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@....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 */

Powered by blists - more mailing lists