diff mbox series

[V3,XRT,Alveo,02/18] fpga: xrt: driver metadata helper functions

Message ID 20210218064019.29189-3-lizhih@xilinx.com
State New
Headers show
Series XRT Alveo driver overview | expand

Commit Message

Lizhi Hou Feb. 18, 2021, 6:40 a.m. UTC
XRT drivers use device tree as metadata format to discover HW subsystems
behind PCIe BAR. Thus libfdt functions are called for driver to parse
device tree blob.

Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>
Signed-off-by: Max Zhen <max.zhen@xilinx.com>
Signed-off-by: Lizhi Hou <lizhih@xilinx.com>
---
 drivers/fpga/xrt/include/metadata.h  | 229 ++++++++++++
 drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++
 2 files changed, 753 insertions(+)
 create mode 100644 drivers/fpga/xrt/include/metadata.h
 create mode 100644 drivers/fpga/xrt/metadata/metadata.c

Comments

Tom Rix Feb. 20, 2021, 5:07 p.m. UTC | #1
On 2/17/21 10:40 PM, Lizhi Hou wrote:
> XRT drivers use device tree as metadata format to discover HW subsystems

> behind PCIe BAR. Thus libfdt functions are called for driver to parse

for the driver to parse the
> device tree blob.

>

> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>

> Signed-off-by: Max Zhen <max.zhen@xilinx.com>

> Signed-off-by: Lizhi Hou <lizhih@xilinx.com>

> ---

>  drivers/fpga/xrt/include/metadata.h  | 229 ++++++++++++

>  drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++

>  2 files changed, 753 insertions(+)

>  create mode 100644 drivers/fpga/xrt/include/metadata.h

>  create mode 100644 drivers/fpga/xrt/metadata/metadata.c

>

> diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h

> new file mode 100644

> index 000000000000..b929bc469b73

> --- /dev/null

> +++ b/drivers/fpga/xrt/include/metadata.h

> @@ -0,0 +1,229 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Header file for Xilinx Runtime (XRT) driver

> + *

> + * Copyright (C) 2020-2021 Xilinx, Inc.

> + *

> + * Authors:

> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

> + */

> +

> +#ifndef _XRT_METADATA_H

> +#define _XRT_METADATA_H

> +

> +#include <linux/device.h>

> +#include <linux/vmalloc.h>

> +#include <linux/uuid.h>

> +

> +#define XRT_MD_INVALID_LENGTH (~0UL)

> +


These #defines could be in alphabetical order

Some #define with embedded acronyms could be expanded

ex/ XRT_MD_NODE_CMC_REG , what is CMC ?

> +#define XRT_MD_PROP_COMPATIBLE "compatible"

> +#define XRT_MD_PROP_PF_NUM "pcie_physical_function"

> +#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping"

> +#define XRT_MD_PROP_IO_OFFSET "reg"

> +#define XRT_MD_PROP_INTERRUPTS "interrupts"

> +#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid"

> +#define XRT_MD_PROP_LOGIC_UUID "logic_uuid"

> +#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major"

> +

> +#define XRT_MD_PROP_HWICAP "axi_hwicap"

> +#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem"

> +

> +#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints"

> +#define XRT_MD_INTERFACES_PATH "/interfaces"

> +

> +#define XRT_MD_NODE_FIRMWARE "firmware"

> +#define XRT_MD_NODE_INTERFACES "interfaces"

> +#define XRT_MD_NODE_PARTITION_INFO "partition_info"

> +

> +#define XRT_MD_NODE_FLASH "ep_card_flash_program_00"

> +#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00"

> +#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00"

> +#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00"

> +#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00"

> +#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00"

> +#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00"

> +#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00"

> +#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00"

> +#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00"

> +#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00"

> +#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00"

> +#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00"

> +#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00"

> +#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00"

> +#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00"

> +#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00"

> +#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00"

> +#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00"

> +#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00"

> +#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00"

> +#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00"

> +#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00"

> +#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00"

> +#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00"

> +#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00"

> +#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00"

> +#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01"

> +#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00"

> +#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00"

> +#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00"

> +#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00"

> +#define XRT_MD_NODE_XDMA "ep_xdma_00"

> +#define XRT_MD_NODE_MSIX "ep_msix_00"

> +#define XRT_MD_NODE_QDMA "ep_qdma_00"

> +#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00"

> +#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00"

> +#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00"

> +#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00"

> +#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00"

> +#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00"

> +#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00"

> +#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01"

> +#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00"

> +#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00"

> +#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00"

> +#define XRT_MD_NODE_P2P "ep_p2p_00"

> +#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00"

> +#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00"

> +#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00"

> +#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00"

> +#define XRT_MD_NODE_PMC_INTR   "ep_pmc_intr_00"

> +#define XRT_MD_NODE_PMC_MUX    "ep_pmc_mux_00"

> +

> +/* driver defined endpoints */

> +#define XRT_MD_NODE_VSEC "drv_ep_vsec_00"

> +#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00"

> +#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00"

> +#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00"

> +#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00"

> +#define XRT_MD_NODE_TEST "drv_ep_test_00"

> +#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00"

> +#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00"

> +#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00"

> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0"

> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1"

> +

> +#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr"

> +#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr"

> +

> +#define XRT_MD_PROP_OFFSET "drv_offset"

> +#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency"

> +#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter"

> +#define XRT_MD_PROP_VBNV "vbnv"

> +#define XRT_MD_PROP_VROM "vrom"

> +#define XRT_MD_PROP_PARTITION_LEVEL "partition_level"

> +

> +struct xrt_md_endpoint {

> +	const char	*ep_name;

> +	u32		bar;

> +	long		bar_off;

> +	ulong		size;

> +	char		*regmap;

> +	char		*regmap_ver;

> +};

could the existing 'struct regmap' be used here ?
> +

> +/* Note: res_id is defined by leaf driver and must start with 0. */

> +struct xrt_iores_map {

> +	char		*res_name;

> +	int		res_id;

> +};

> +

> +static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map,

> +				     int entry_num, const char *res_name)

> +{

> +	int i;

> +

> +	for (i = 0; i < entry_num; i++) {

> +		if (!strcmp(res_name, res_map->res_name))


Use the 'n' variant strncmp for better safety.

Fix generally.

> +			return res_map->res_id;

> +		res_map++;

> +	}

> +	return -1;

> +}

> +

> +static inline const char *

> +xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id)

> +{

> +	int i;

> +

> +	for (i = 0; i < entry_num; i++) {

> +		if (res_map->res_id == id)

> +			return res_map->res_name;

> +		res_map++;

> +	}

> +	return NULL;

> +}

> +

> +unsigned long xrt_md_size(struct device *dev, const char *blob);

> +int xrt_md_create(struct device *dev, char **blob);

> +int xrt_md_add_endpoint(struct device *dev, char *blob,

> +			struct xrt_md_endpoint *ep);

> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

> +			char *regmap_name);

> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void **val, int *size);

> +int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void *val, int size);

> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char *new_ep_name);

> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

> +			     const char *ep_name,  const char *regmap_name,

> +			     char **next_ep, char **next_regmap);

> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

> +				   const char *regmap_name, const char **ep_name);

> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char **epname);

> +void xrt_md_pack(struct device *dev, char *blob);

> +char *xrt_md_dup(struct device *dev, const char *blob);

> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

> +			  u32 *num_uuids, uuid_t *intf_uuids);

> +static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob)

> +{

> +	return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS,

> +				    NULL, NULL);


A wrapping a single call seems like an unnecessary abstraction layer.

can this be reduced/removed ?

> +}

> +

> +/*

> + * Firmware provides 128 bit hash string as unque id of partition/interface.


The firmware provides a 128 bit hash string as a unique id to the partition/interface.

Existing hw does not yet use the cononical form, so it is necessary to use a translation function.

> + * This string will be canonical textual representation in the future.

> + * Before that, introducing these two functions below to translate

> + * hash string to uuid_t for released hardware.

Is there an existing version string the new hw will use to check which way to go ?
> + */

> +static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr)

> +{

> +	int i, p;

> +	u8 *u = (u8 *)uuid;

> +

> +	for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--)


This loop needs to be improved.

Consider if sizeof(uuid_t) changed, accessing u[] would overflow.

> +		(void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]);

(void) cast isn't needed.
> +}

> +

> +static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid)

> +{

> +	char *p;

> +	const char *str;

> +	char tmp[3] = { 0 };

> +	int i, ret;

> +

> +	memset(p_uuid, 0, sizeof(*p_uuid));

> +	p = (char *)p_uuid;

access with p_uuid->b[] would remove use of 'p'
> +	str = uuidstr + strlen(uuidstr) - 2;

> +

> +	for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) {

is not filling p_uuid completely really ok ?
> +		tmp[0] = *str;

> +		tmp[1] = *(str + 1);

> +		ret = kstrtou8(tmp, 16, p);

> +		if (ret)

> +			return -EINVAL;

> +		p++;

> +		str -= 2;

> +	}

> +

> +	return 0;

> +}

> +

> +#endif

> diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c

> new file mode 100644

> index 000000000000..5d106396f438

> --- /dev/null

> +++ b/drivers/fpga/xrt/metadata/metadata.c

> @@ -0,0 +1,524 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * Xilinx Alveo FPGA Metadata parse APIs

> + *

> + * Copyright (C) 2020-2021 Xilinx, Inc.

> + *

> + * Authors:

> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

> + */

> +

> +#include <linux/libfdt_env.h>

> +#include "libfdt.h"

> +#include "metadata.h"

> +

> +#define MAX_BLOB_SIZE	(4096 * 25)

> +

> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

> +			  const char *prop, const void *val, int size);

> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

> +			  const char *overlay_blob, int overlay_offset);

> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

> +			       const char *ep_name, const char *regmap_name,

> +			       int *ep_offset);

consider reordering functions so these fwd decl's are not needed
> +

> +unsigned long xrt_md_size(struct device *dev, const char *blob)

> +{

> +	unsigned long len = (long)fdt_totalsize(blob);

> +

> +	len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len;

> +	return len;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_size);

> +

> +int xrt_md_create(struct device *dev, char **blob)

> +{

> +	int ret = 0;

> +

> +	WARN_ON(!blob);

> +

> +	*blob = vmalloc(MAX_BLOB_SIZE);

why vmalloc instead of vzalloc ?
> +	if (!*blob)

> +		return -ENOMEM;

> +

> +	ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE);

> +	if (ret) {

> +		dev_err(dev, "format blob failed, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	ret = fdt_next_node(*blob, -1, NULL);


A variable called 'offset' would make more sense here because it is used later in fdt_add_subnode as the parentoffset parameter.

> +	if (ret < 0) {

> +		dev_err(dev, "No Node, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS);


why isn't the parentoffset '0' ?


> +	if (ret < 0) {

> +		dev_err(dev, "add node failed, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	return 0;

> +

> +failed:

> +	vfree(*blob);

> +	*blob = NULL;

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_create);

> +

> +static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset,

> +			   const char *ep_name)

> +{

> +	int ret;

> +

> +	ret = fdt_add_subnode(blob, parent_offset, ep_name);

> +	if (ret < 0 && ret != -FDT_ERR_EXISTS)

> +		dev_err(dev, "failed to add node %s. %d", ep_name, ret);

> +

> +	return ret;

> +}

> +

> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

> +			char *regmap_name)

> +{

> +	int ret;

> +	int ep_offset;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset);

> +	if (ret) {

> +		dev_err(dev, "can not find ep %s", ep_name);

> +		return -EINVAL;

> +	}

> +

> +	ret = fdt_del_node(blob, ep_offset);

> +	if (ret)

> +		dev_err(dev, "delete node %s failed, ret %d", ep_name, ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_del_endpoint);

> +

> +static int __xrt_md_add_endpoint(struct device *dev, char *blob,

> +				 struct xrt_md_endpoint *ep, int *offset, bool root)

> +{

> +	int ret = 0;

> +	int ep_offset = 0;

> +	u32 val, count = 0;

> +	u64 io_range[2];

> +	char comp[128];

> +

> +	if (!ep->ep_name) {

> +		dev_err(dev, "empty name");

> +		return -EINVAL;

> +	}

> +

> +	if (!root) {

> +		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

> +					  &ep_offset);

> +		if (ret) {

> +			dev_err(dev, "invalid blob, ret = %d", ret);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name);

> +	if (ep_offset < 0) {

> +		dev_err(dev, "add endpoint failed, ret = %d", ret);

> +		return -EINVAL;

> +	}

> +	if (offset)

> +		*offset = ep_offset;

> +

> +	if (ep->size != 0) {

> +		val = cpu_to_be32(ep->bar);

> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX,

> +				     &val, sizeof(u32));

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_BAR_IDX, ret);

> +			goto failed;

> +		}

> +		io_range[0] = cpu_to_be64((u64)ep->bar_off);

> +		io_range[1] = cpu_to_be64((u64)ep->size);

is there a type mismatch between bar (u32) and size (u64) ?
> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET,

> +				     io_range, sizeof(io_range));

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_IO_OFFSET, ret);

> +			goto failed;

> +		}

> +	}

> +

> +	if (ep->regmap) {

> +		if (ep->regmap_ver) {

> +			count = snprintf(comp, sizeof(comp),

> +					 "%s-%s", ep->regmap, ep->regmap_ver);

> +			count++;

> +		}

> +

> +		count += snprintf(comp + count, sizeof(comp) - count,

> +				  "%s", ep->regmap);

> +		count++;


unlikely, but overflow is not checked.

are multiple null's in this string ok ?

> +

> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE,

> +				     comp, count);

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_COMPATIBLE, ret);

> +			goto failed;

> +		}

> +	}

> +

> +failed:

> +	if (ret)

> +		xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL);

> +

> +	return ret;

> +}

> +

> +int xrt_md_add_endpoint(struct device *dev, char *blob,

> +			struct xrt_md_endpoint *ep)

> +{

> +	return __xrt_md_add_endpoint(dev, blob, ep, NULL, false);


ok, user doesn't add root's endpoint.

could an assert be added ?

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_add_endpoint);

> +

> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

> +			       const char *ep_name, const char *regmap_name,

> +			       int *ep_offset)

> +{

> +	int offset;

> +	const char *name;

> +

> +	for (offset = fdt_next_node(blob, -1, NULL);

> +	    offset >= 0;

> +	    offset = fdt_next_node(blob, offset, NULL)) {

> +		name = fdt_get_name(blob, offset, NULL);

> +		if (!name || strncmp(name, ep_name, strlen(ep_name) + 1))

strlen() + 1 ?
> +			continue;

> +		if (!regmap_name ||

> +		    !fdt_node_check_compatible(blob, offset, regmap_name))

> +			break;

> +	}

> +	if (offset < 0)

> +		return -ENODEV;

> +

> +	*ep_offset = offset;

> +

> +	return 0;

> +}

> +

> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char **epname)

> +{

> +	int offset;

> +	int ret;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +				  &offset);

> +	if (!ret && epname && offset >= 0)

The offset >= 0 check isn't needed, !ret is enough
> +		*epname = fdt_get_name(blob, offset, NULL);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_find_endpoint);

> +

> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void **val, int *size)

> +{

> +	int offset;

> +	int ret;

> +

> +	if (val)

> +		*val = NULL;


Seems like no point in making this call if !val

Return -EINVAL if !val and remove the if (val) check below.

> +	if (ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +					  &offset);

> +		if (ret) {

> +			dev_err(dev, "cannot get ep %s, regmap %s, ret = %d",

> +				ep_name, regmap_name, ret);

> +			return -EINVAL;

> +		}

> +	} else {

> +		offset = fdt_next_node(blob, -1, NULL);

> +		if (offset < 0) {

> +			dev_err(dev, "internal error, ret = %d", offset);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	if (val) {

> +		*val = fdt_getprop(blob, offset, prop, size);

> +		if (!*val) {

> +			dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_prop);

> +

> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

> +			  const char *prop, const void *val, int size)

> +{

> +	int ret;

> +

> +	ret = fdt_setprop(blob, offset, prop, val, size);

> +	if (ret)

> +		dev_err(dev, "failed to set prop %d", ret);

> +

> +	return ret;

> +}

> +

> +int xrt_md_set_prop(struct device *dev, char *blob,

> +		    const char *ep_name, const char *regmap_name,

> +		    const char *prop, const void *val, int size)

> +{

> +	int offset;

> +	int ret;

> +

> +	if (ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name,

> +					  regmap_name, &offset);

> +		if (ret) {

> +			dev_err(dev, "cannot get node %s, ret = %d",

> +				ep_name, ret);

> +			return -EINVAL;

> +		}

> +	} else {

> +		offset = fdt_next_node(blob, -1, NULL);

> +		if (offset < 0) {

> +			dev_err(dev, "internal error, ret = %d", offset);

> +			return -EINVAL;

> +		}

> +	}


This if-else block is a cut-n-paste from above.

It is good to convert common logic blocks to macros or inline functions.

> +

> +	ret = xrt_md_setprop(dev, blob, offset, prop, val, size);

> +	if (ret)

> +		dev_err(dev, "set prop %s failed, ret = %d", prop, ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_set_prop);

> +

> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char *new_ep_name)

> +{

> +	int offset, target;

> +	int ret;

> +	struct xrt_md_endpoint ep = {0};

> +	const char *newepnm = new_ep_name ? new_ep_name : ep_name;


How is this valid ?

The xrt_md_get_endpoint searches by ep_name and if there names are not unique the second one will never be found.

> +

> +	ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name,

> +				  &offset);

> +	if (ret)

> +		return -EINVAL;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target);

> +	if (ret) {

> +		ep.ep_name = newepnm;

> +		ret = __xrt_md_add_endpoint(dev, blob, &ep, &target,

> +					    fdt_parent_offset(src_blob, offset) == 0);

> +		if (ret)

> +			return -EINVAL;

> +	}

> +

> +	ret = xrt_md_overlay(dev, blob, target, src_blob, offset);

> +	if (ret)

> +		dev_err(dev, "overlay failed, ret = %d", ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint);

> +

> +char *xrt_md_dup(struct device *dev, const char *blob)

> +{

> +	int ret;

> +	char *dup_blob;

> +

> +	ret = xrt_md_create(dev, &dup_blob);

> +	if (ret)

> +		return NULL;

> +	ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1);

would memcpy-ing the blob work ?
> +	if (ret) {

> +		vfree(dup_blob);

> +		return NULL;

> +	}

> +

> +	return dup_blob;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_dup);

> +

> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

> +			  const char *overlay_blob, int overlay_offset)

> +{

> +	int	property, subnode;

> +	int	ret;

> +

> +	WARN_ON(!blob || !overlay_blob);

> +

> +	if (!blob) {

> +		dev_err(dev, "blob is NULL");

> +		return -EINVAL;

> +	}

> +

> +	if (target < 0) {

> +		target = fdt_next_node(blob, -1, NULL);

> +		if (target < 0) {

> +			dev_err(dev, "invalid target");

> +			return -EINVAL;

> +		}

> +	}

> +	if (overlay_offset < 0) {

> +		overlay_offset = fdt_next_node(overlay_blob, -1, NULL);

> +		if (overlay_offset < 0) {

> +			dev_err(dev, "invalid overlay");

> +			return -EINVAL;

> +		}

> +	}

> +

> +	fdt_for_each_property_offset(property, overlay_blob, overlay_offset) {

> +		const char *name;

> +		const void *prop;

> +		int prop_len;

> +

> +		prop = fdt_getprop_by_offset(overlay_blob, property, &name,

> +					     &prop_len);

> +		if (!prop || prop_len >= MAX_BLOB_SIZE) {

could add prop_len < 0 as another sanity check
> +			dev_err(dev, "internal error");

> +			return -EINVAL;

> +		}

> +

> +		ret = xrt_md_setprop(dev, blob, target, name, prop,

> +				     prop_len);

> +		if (ret) {

> +			dev_err(dev, "setprop failed, ret = %d", ret);

> +			return ret;

overlay_blob is half done, as an error handling shouldn't it be undone ?
> +		}

> +	}

> +

> +	fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) {

> +		const char *name = fdt_get_name(overlay_blob, subnode, NULL);

> +		int nnode;

> +

> +		nnode = xrt_md_add_node(dev, blob, target, name);

> +		if (nnode == -FDT_ERR_EXISTS)

> +			nnode = fdt_subnode_offset(blob, target, name);

> +		if (nnode < 0) {

> +			dev_err(dev, "add node failed, ret = %d", nnode);

> +			return nnode;

> +		}

> +

> +		ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode);


eek, recursion.

Any chance this will blow the stack ?

> +		if (ret)

> +			return ret;

> +	}

> +

> +	return 0;

> +}

> +

> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

> +			     const char *ep_name, const char *regmap_name,

> +			     char **next_ep, char **next_regmap)

> +{

> +	int offset, ret;

> +

> +	if (!ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

> +					  &offset);

> +	} else {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +					  &offset);

> +	}

> +

> +	if (ret) {

> +		*next_ep = NULL;

> +		*next_regmap = NULL;

could initialize next_ep and next_regmap to NULL outside the check.
> +		return -EINVAL;

> +	}

> +

> +	offset = ep_name ? fdt_next_subnode(blob, offset) :

> +		fdt_first_subnode(blob, offset);

> +	if (offset < 0) {

> +		*next_ep = NULL;

> +		*next_regmap = NULL;

> +		return -EINVAL;

> +	}

> +

> +	*next_ep = (char *)fdt_get_name(blob, offset, NULL);

> +	*next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE,

> +						  0, NULL);

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint);

> +

> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

> +				   const char *regmap_name, const char **ep_name)

> +{

> +	int ep_offset;

> +

> +	ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name);

> +	if (ep_offset < 0) {

> +		*ep_name = NULL;

> +		return -ENOENT;

> +	}

> +

> +	*ep_name = (char *)fdt_get_name(blob, ep_offset, NULL);

why the cast ?
> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint);

> +

> +void xrt_md_pack(struct device *dev, char *blob)

> +{

> +	int ret;

> +

> +	ret = fdt_pack(blob);

> +	if (ret)

> +		dev_err(dev, "pack failed %d", ret);

maybe return int
> +}

> +EXPORT_SYMBOL_GPL(xrt_md_pack);

> +

The input/output of num_uuids parameter is tricky add a comment
> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

> +			  u32 *num_uuids, uuid_t *intf_uuids)

what is intf ? change to 'interface' ?
> +{

> +	int offset, count = 0;

> +	int ret;

> +	const char *uuid_str;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset);

> +	if (ret)

> +		return -ENOENT;

> +

> +	for (offset = fdt_first_subnode(blob, offset);

> +	    offset >= 0;

> +	    offset = fdt_next_subnode(blob, offset)) {

> +		uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID,

> +				       NULL);

> +		if (!uuid_str) {

> +			dev_err(dev, "empty intf uuid node");

> +			return -EINVAL;

> +		}

> +

> +		if (intf_uuids && count < *num_uuids) {

> +			ret = xrt_md_trans_str2uuid(dev, uuid_str,

> +						    &intf_uuids[count]);

> +			if (ret)

> +				return -EINVAL;

> +		}

> +		count++;


keep going even when count > num_uuids ?

that seems like an error.

Tom

> +	}

> +

> +	*num_uuids = count;

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);
Fernando Pacheco Feb. 23, 2021, 1:23 a.m. UTC | #2
On Wed, Feb 17, 2021 at 10:40:03PM -0800, Lizhi Hou wrote:
> XRT drivers use device tree as metadata format to discover HW subsystems

> behind PCIe BAR. Thus libfdt functions are called for driver to parse

> device tree blob.

> 

> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>

> Signed-off-by: Max Zhen <max.zhen@xilinx.com>

> Signed-off-by: Lizhi Hou <lizhih@xilinx.com>

> ---

>  drivers/fpga/xrt/include/metadata.h  | 229 ++++++++++++

>  drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++

>  2 files changed, 753 insertions(+)

>  create mode 100644 drivers/fpga/xrt/include/metadata.h

>  create mode 100644 drivers/fpga/xrt/metadata/metadata.c

> 

> diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h

> new file mode 100644

> index 000000000000..b929bc469b73

> --- /dev/null

> +++ b/drivers/fpga/xrt/include/metadata.h

> @@ -0,0 +1,229 @@

> +/* SPDX-License-Identifier: GPL-2.0 */

> +/*

> + * Header file for Xilinx Runtime (XRT) driver

> + *

> + * Copyright (C) 2020-2021 Xilinx, Inc.

> + *

> + * Authors:

> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

> + */

> +

> +#ifndef _XRT_METADATA_H

> +#define _XRT_METADATA_H

> +

> +#include <linux/device.h>

> +#include <linux/vmalloc.h>

> +#include <linux/uuid.h>

> +

> +#define XRT_MD_INVALID_LENGTH (~0UL)

> +

> +#define XRT_MD_PROP_COMPATIBLE "compatible"

> +#define XRT_MD_PROP_PF_NUM "pcie_physical_function"

> +#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping"

> +#define XRT_MD_PROP_IO_OFFSET "reg"

> +#define XRT_MD_PROP_INTERRUPTS "interrupts"

> +#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid"

> +#define XRT_MD_PROP_LOGIC_UUID "logic_uuid"

> +#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major"

> +

> +#define XRT_MD_PROP_HWICAP "axi_hwicap"

> +#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem"

> +

> +#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints"

> +#define XRT_MD_INTERFACES_PATH "/interfaces"

> +

> +#define XRT_MD_NODE_FIRMWARE "firmware"

> +#define XRT_MD_NODE_INTERFACES "interfaces"

> +#define XRT_MD_NODE_PARTITION_INFO "partition_info"

> +

> +#define XRT_MD_NODE_FLASH "ep_card_flash_program_00"

> +#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00"

> +#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00"

> +#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00"

> +#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00"

> +#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00"

> +#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00"

> +#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00"

> +#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00"

> +#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00"

> +#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00"

> +#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00"

> +#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00"

> +#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00"

> +#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00"

> +#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00"

> +#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00"

> +#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00"

> +#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00"

> +#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00"

> +#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00"

> +#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00"

> +#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00"

> +#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00"

> +#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00"

> +#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00"

> +#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00"

> +#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01"

> +#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00"

> +#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00"

> +#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00"

> +#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00"

> +#define XRT_MD_NODE_XDMA "ep_xdma_00"

> +#define XRT_MD_NODE_MSIX "ep_msix_00"

> +#define XRT_MD_NODE_QDMA "ep_qdma_00"

> +#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00"

> +#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00"

> +#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00"

> +#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00"

> +#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00"

> +#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00"

> +#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00"

> +#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01"

> +#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00"

> +#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00"

> +#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00"

> +#define XRT_MD_NODE_P2P "ep_p2p_00"

> +#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00"

> +#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00"

> +#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00"

> +#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00"

> +#define XRT_MD_NODE_PMC_INTR   "ep_pmc_intr_00"

> +#define XRT_MD_NODE_PMC_MUX    "ep_pmc_mux_00"

> +

> +/* driver defined endpoints */

> +#define XRT_MD_NODE_VSEC "drv_ep_vsec_00"

> +#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00"

> +#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00"

> +#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00"


Should this be "drv_ep_mailbox_vsec_00"?

> +#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00"

> +#define XRT_MD_NODE_TEST "drv_ep_test_00"

> +#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00"

> +#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00"

> +#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00"

> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0"

> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1"

> +

> +#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr"

> +#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr"

> +

> +#define XRT_MD_PROP_OFFSET "drv_offset"

> +#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency"

> +#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter"

> +#define XRT_MD_PROP_VBNV "vbnv"

> +#define XRT_MD_PROP_VROM "vrom"

> +#define XRT_MD_PROP_PARTITION_LEVEL "partition_level"

> +

> +struct xrt_md_endpoint {

> +	const char	*ep_name;

> +	u32		bar;

> +	long		bar_off;

> +	ulong		size;

> +	char		*regmap;

> +	char		*regmap_ver;

> +};

> +

> +/* Note: res_id is defined by leaf driver and must start with 0. */

> +struct xrt_iores_map {

> +	char		*res_name;

> +	int		res_id;

> +};

> +

> +static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map,

> +				     int entry_num, const char *res_name)

> +{

> +	int i;

> +

> +	for (i = 0; i < entry_num; i++) {

> +		if (!strcmp(res_name, res_map->res_name))

> +			return res_map->res_id;

> +		res_map++;

> +	}

> +	return -1;

> +}

> +

> +static inline const char *

> +xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id)

> +{

> +	int i;

> +

> +	for (i = 0; i < entry_num; i++) {

> +		if (res_map->res_id == id)

> +			return res_map->res_name;

> +		res_map++;

> +	}

> +	return NULL;

> +}

> +

> +unsigned long xrt_md_size(struct device *dev, const char *blob);

> +int xrt_md_create(struct device *dev, char **blob);

> +int xrt_md_add_endpoint(struct device *dev, char *blob,

> +			struct xrt_md_endpoint *ep);

> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

> +			char *regmap_name);

> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void **val, int *size);

> +int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void *val, int size);

> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char *new_ep_name);

> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

> +			     const char *ep_name,  const char *regmap_name,

> +			     char **next_ep, char **next_regmap);

> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

> +				   const char *regmap_name, const char **ep_name);

> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char **epname);

> +void xrt_md_pack(struct device *dev, char *blob);

> +char *xrt_md_dup(struct device *dev, const char *blob);

> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

> +			  u32 *num_uuids, uuid_t *intf_uuids);

> +static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob)

> +{

> +	return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS,

> +				    NULL, NULL);

> +}

> +

> +/*

> + * Firmware provides 128 bit hash string as unque id of partition/interface.

> + * This string will be canonical textual representation in the future.

> + * Before that, introducing these two functions below to translate

> + * hash string to uuid_t for released hardware.

> + */

> +static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr)

> +{

> +	int i, p;

> +	u8 *u = (u8 *)uuid;

> +

> +	for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--)

> +		(void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]);

> +}

> +

> +static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid)

> +{

> +	char *p;

> +	const char *str;

> +	char tmp[3] = { 0 };

> +	int i, ret;

> +

> +	memset(p_uuid, 0, sizeof(*p_uuid));

> +	p = (char *)p_uuid;

> +	str = uuidstr + strlen(uuidstr) - 2;

> +

> +	for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) {

> +		tmp[0] = *str;

> +		tmp[1] = *(str + 1);

> +		ret = kstrtou8(tmp, 16, p);

> +		if (ret)

> +			return -EINVAL;

> +		p++;

> +		str -= 2;

> +	}

> +

> +	return 0;

> +}

> +

> +#endif

> diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c

> new file mode 100644

> index 000000000000..5d106396f438

> --- /dev/null

> +++ b/drivers/fpga/xrt/metadata/metadata.c

> @@ -0,0 +1,524 @@

> +// SPDX-License-Identifier: GPL-2.0

> +/*

> + * Xilinx Alveo FPGA Metadata parse APIs

> + *

> + * Copyright (C) 2020-2021 Xilinx, Inc.

> + *

> + * Authors:

> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

> + */

> +

> +#include <linux/libfdt_env.h>

> +#include "libfdt.h"

> +#include "metadata.h"

> +

> +#define MAX_BLOB_SIZE	(4096 * 25)

> +

> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

> +			  const char *prop, const void *val, int size);

> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

> +			  const char *overlay_blob, int overlay_offset);

> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

> +			       const char *ep_name, const char *regmap_name,

> +			       int *ep_offset);

> +

> +unsigned long xrt_md_size(struct device *dev, const char *blob)

> +{

> +	unsigned long len = (long)fdt_totalsize(blob);

> +

> +	len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len;


how about (avoiding len = len):

	if (len > MAX_BLOB_SIZE)
		return XRT_MD_INVALID_LENGTH;

	return len;

> +	return len;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_size);

> +

> +int xrt_md_create(struct device *dev, char **blob)

> +{

> +	int ret = 0;

> +

> +	WARN_ON(!blob);


Shouldn't this be signaled to caller as a -EINVAL?

> +

> +	*blob = vmalloc(MAX_BLOB_SIZE);

> +	if (!*blob)

> +		return -ENOMEM;

> +

> +	ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE);

> +	if (ret) {

> +		dev_err(dev, "format blob failed, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	ret = fdt_next_node(*blob, -1, NULL);

> +	if (ret < 0) {

> +		dev_err(dev, "No Node, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS);

> +	if (ret < 0) {

> +		dev_err(dev, "add node failed, ret = %d", ret);

> +		goto failed;

> +	}

> +

> +	return 0;

> +

> +failed:

> +	vfree(*blob);

> +	*blob = NULL;

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_create);

> +

> +static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset,

> +			   const char *ep_name)

> +{

> +	int ret;

> +

> +	ret = fdt_add_subnode(blob, parent_offset, ep_name);

> +	if (ret < 0 && ret != -FDT_ERR_EXISTS)

> +		dev_err(dev, "failed to add node %s. %d", ep_name, ret);

> +

> +	return ret;

> +}

> +

> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

> +			char *regmap_name)

> +{

> +	int ret;

> +	int ep_offset;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset);

> +	if (ret) {

> +		dev_err(dev, "can not find ep %s", ep_name);

> +		return -EINVAL;

> +	}

> +

> +	ret = fdt_del_node(blob, ep_offset);

> +	if (ret)

> +		dev_err(dev, "delete node %s failed, ret %d", ep_name, ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_del_endpoint);

> +

> +static int __xrt_md_add_endpoint(struct device *dev, char *blob,

> +				 struct xrt_md_endpoint *ep, int *offset, bool root)

> +{

> +	int ret = 0;

> +	int ep_offset = 0;

> +	u32 val, count = 0;

> +	u64 io_range[2];

> +	char comp[128];

> +

> +	if (!ep->ep_name) {

> +		dev_err(dev, "empty name");

> +		return -EINVAL;

> +	}

> +

> +	if (!root) {

> +		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

> +					  &ep_offset);

> +		if (ret) {

> +			dev_err(dev, "invalid blob, ret = %d", ret);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name);


A little hard to follow when you re-use ep_offset like this. Maybe
parent_offset to match xrt_md_add_node parameter?

> +	if (ep_offset < 0) {

> +		dev_err(dev, "add endpoint failed, ret = %d", ret);

> +		return -EINVAL;

> +	}

> +	if (offset)

> +		*offset = ep_offset;

> +

> +	if (ep->size != 0) {

> +		val = cpu_to_be32(ep->bar);

> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX,

> +				     &val, sizeof(u32));

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_BAR_IDX, ret);

> +			goto failed;

> +		}

> +		io_range[0] = cpu_to_be64((u64)ep->bar_off);

> +		io_range[1] = cpu_to_be64((u64)ep->size);

> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET,

> +				     io_range, sizeof(io_range));

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_IO_OFFSET, ret);

> +			goto failed;

> +		}

> +	}

> +

> +	if (ep->regmap) {

> +		if (ep->regmap_ver) {

> +			count = snprintf(comp, sizeof(comp),

> +					 "%s-%s", ep->regmap, ep->regmap_ver);

> +			count++;

> +		}

> +

> +		count += snprintf(comp + count, sizeof(comp) - count,

> +				  "%s", ep->regmap);

> +		count++;

> +

> +		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE,

> +				     comp, count);

> +		if (ret) {

> +			dev_err(dev, "set %s failed, ret %d",

> +				XRT_MD_PROP_COMPATIBLE, ret);

> +			goto failed;

> +		}

> +	}

> +

> +failed:

> +	if (ret)

> +		xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL);

> +

> +	return ret;

> +}

> +

> +int xrt_md_add_endpoint(struct device *dev, char *blob,

> +			struct xrt_md_endpoint *ep)

> +{

> +	return __xrt_md_add_endpoint(dev, blob, ep, NULL, false);

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_add_endpoint);

> +

> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

> +			       const char *ep_name, const char *regmap_name,

> +			       int *ep_offset)

> +{

> +	int offset;

> +	const char *name;

> +

> +	for (offset = fdt_next_node(blob, -1, NULL);

> +	    offset >= 0;

> +	    offset = fdt_next_node(blob, offset, NULL)) {

> +		name = fdt_get_name(blob, offset, NULL);

> +		if (!name || strncmp(name, ep_name, strlen(ep_name) + 1))

> +			continue;

> +		if (!regmap_name ||

> +		    !fdt_node_check_compatible(blob, offset, regmap_name))

> +			break;

> +	}

> +	if (offset < 0)

> +		return -ENODEV;

> +

> +	*ep_offset = offset;

> +

> +	return 0;

> +}

> +

> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char **epname)

> +{

> +	int offset;

> +	int ret;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +				  &offset);

> +	if (!ret && epname && offset >= 0)

> +		*epname = fdt_get_name(blob, offset, NULL);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_find_endpoint);

> +

> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

> +		    const char *regmap_name, const char *prop,

> +		    const void **val, int *size)

> +{

> +	int offset;

> +	int ret;

> +

> +	if (val)

> +		*val = NULL;

> +	if (ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +					  &offset);

> +		if (ret) {

> +			dev_err(dev, "cannot get ep %s, regmap %s, ret = %d",

> +				ep_name, regmap_name, ret);

> +			return -EINVAL;

> +		}

> +	} else {

> +		offset = fdt_next_node(blob, -1, NULL);

> +		if (offset < 0) {

> +			dev_err(dev, "internal error, ret = %d", offset);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	if (val) {

> +		*val = fdt_getprop(blob, offset, prop, size);

> +		if (!*val) {

> +			dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_prop);

> +

> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

> +			  const char *prop, const void *val, int size)

> +{

> +	int ret;

> +

> +	ret = fdt_setprop(blob, offset, prop, val, size);

> +	if (ret)

> +		dev_err(dev, "failed to set prop %d", ret);

> +

> +	return ret;

> +}

> +

> +int xrt_md_set_prop(struct device *dev, char *blob,

> +		    const char *ep_name, const char *regmap_name,

> +		    const char *prop, const void *val, int size)

> +{

> +	int offset;

> +	int ret;

> +

> +	if (ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name,

> +					  regmap_name, &offset);

> +		if (ret) {

> +			dev_err(dev, "cannot get node %s, ret = %d",

> +				ep_name, ret);

> +			return -EINVAL;

> +		}

> +	} else {

> +		offset = fdt_next_node(blob, -1, NULL);

> +		if (offset < 0) {

> +			dev_err(dev, "internal error, ret = %d", offset);

> +			return -EINVAL;

> +		}

> +	}

> +

> +	ret = xrt_md_setprop(dev, blob, offset, prop, val, size);

> +	if (ret)

> +		dev_err(dev, "set prop %s failed, ret = %d", prop, ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_set_prop);

> +

> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

> +			 const char *ep_name, const char *regmap_name,

> +			 const char *new_ep_name)

> +{

> +	int offset, target;

> +	int ret;

> +	struct xrt_md_endpoint ep = {0};

> +	const char *newepnm = new_ep_name ? new_ep_name : ep_name;

> +

> +	ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name,

> +				  &offset);

> +	if (ret)

> +		return -EINVAL;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target);

> +	if (ret) {

> +		ep.ep_name = newepnm;

> +		ret = __xrt_md_add_endpoint(dev, blob, &ep, &target,

> +					    fdt_parent_offset(src_blob, offset) == 0);

> +		if (ret)

> +			return -EINVAL;

> +	}

> +

> +	ret = xrt_md_overlay(dev, blob, target, src_blob, offset);

> +	if (ret)

> +		dev_err(dev, "overlay failed, ret = %d", ret);

> +

> +	return ret;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint);

> +

> +char *xrt_md_dup(struct device *dev, const char *blob)

> +{

> +	int ret;

> +	char *dup_blob;

> +

> +	ret = xrt_md_create(dev, &dup_blob);

> +	if (ret)

> +		return NULL;

> +	ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1);

> +	if (ret) {

> +		vfree(dup_blob);

> +		return NULL;

> +	}

> +

> +	return dup_blob;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_dup);

> +

> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

> +			  const char *overlay_blob, int overlay_offset)

> +{

> +	int	property, subnode;

> +	int	ret;

> +

> +	WARN_ON(!blob || !overlay_blob);


Why check !blob twice? Also, is a WARN_ON justified here
(and in xrt_md_create)?

Fernando

> +

> +	if (!blob) {

> +		dev_err(dev, "blob is NULL");

> +		return -EINVAL;

> +	}

> +

> +	if (target < 0) {

> +		target = fdt_next_node(blob, -1, NULL);

> +		if (target < 0) {

> +			dev_err(dev, "invalid target");

> +			return -EINVAL;

> +		}

> +	}

> +	if (overlay_offset < 0) {

> +		overlay_offset = fdt_next_node(overlay_blob, -1, NULL);

> +		if (overlay_offset < 0) {

> +			dev_err(dev, "invalid overlay");

> +			return -EINVAL;

> +		}

> +	}

> +

> +	fdt_for_each_property_offset(property, overlay_blob, overlay_offset) {

> +		const char *name;

> +		const void *prop;

> +		int prop_len;

> +

> +		prop = fdt_getprop_by_offset(overlay_blob, property, &name,

> +					     &prop_len);

> +		if (!prop || prop_len >= MAX_BLOB_SIZE) {

> +			dev_err(dev, "internal error");

> +			return -EINVAL;

> +		}

> +

> +		ret = xrt_md_setprop(dev, blob, target, name, prop,

> +				     prop_len);

> +		if (ret) {

> +			dev_err(dev, "setprop failed, ret = %d", ret);

> +			return ret;

> +		}

> +	}

> +

> +	fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) {

> +		const char *name = fdt_get_name(overlay_blob, subnode, NULL);

> +		int nnode;

> +

> +		nnode = xrt_md_add_node(dev, blob, target, name);

> +		if (nnode == -FDT_ERR_EXISTS)

> +			nnode = fdt_subnode_offset(blob, target, name);

> +		if (nnode < 0) {

> +			dev_err(dev, "add node failed, ret = %d", nnode);

> +			return nnode;

> +		}

> +

> +		ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode);

> +		if (ret)

> +			return ret;

> +	}

> +

> +	return 0;

> +}

> +

> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

> +			     const char *ep_name, const char *regmap_name,

> +			     char **next_ep, char **next_regmap)

> +{

> +	int offset, ret;

> +

> +	if (!ep_name) {

> +		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

> +					  &offset);

> +	} else {

> +		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

> +					  &offset);

> +	}

> +

> +	if (ret) {

> +		*next_ep = NULL;

> +		*next_regmap = NULL;

> +		return -EINVAL;

> +	}

> +

> +	offset = ep_name ? fdt_next_subnode(blob, offset) :

> +		fdt_first_subnode(blob, offset);

> +	if (offset < 0) {

> +		*next_ep = NULL;

> +		*next_regmap = NULL;

> +		return -EINVAL;

> +	}

> +

> +	*next_ep = (char *)fdt_get_name(blob, offset, NULL);

> +	*next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE,

> +						  0, NULL);

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint);

> +

> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

> +				   const char *regmap_name, const char **ep_name)

> +{

> +	int ep_offset;

> +

> +	ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name);

> +	if (ep_offset < 0) {

> +		*ep_name = NULL;

> +		return -ENOENT;

> +	}

> +

> +	*ep_name = (char *)fdt_get_name(blob, ep_offset, NULL);

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint);

> +

> +void xrt_md_pack(struct device *dev, char *blob)

> +{

> +	int ret;

> +

> +	ret = fdt_pack(blob);

> +	if (ret)

> +		dev_err(dev, "pack failed %d", ret);

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_pack);

> +

> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

> +			  u32 *num_uuids, uuid_t *intf_uuids)

> +{

> +	int offset, count = 0;

> +	int ret;

> +	const char *uuid_str;

> +

> +	ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset);

> +	if (ret)

> +		return -ENOENT;

> +

> +	for (offset = fdt_first_subnode(blob, offset);

> +	    offset >= 0;

> +	    offset = fdt_next_subnode(blob, offset)) {

> +		uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID,

> +				       NULL);

> +		if (!uuid_str) {

> +			dev_err(dev, "empty intf uuid node");

> +			return -EINVAL;

> +		}

> +

> +		if (intf_uuids && count < *num_uuids) {

> +			ret = xrt_md_trans_str2uuid(dev, uuid_str,

> +						    &intf_uuids[count]);

> +			if (ret)

> +				return -EINVAL;

> +		}

> +		count++;

> +	}

> +

> +	*num_uuids = count;

> +

> +	return 0;

> +}

> +EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);

> -- 

> 2.18.4

>
Lizhi Hou Feb. 23, 2021, 6:05 a.m. UTC | #3
Hi Tom,


On 02/20/2021 09:07 AM, Tom Rix wrote:
> On 2/17/21 10:40 PM, Lizhi Hou wrote:

>> XRT drivers use device tree as metadata format to discover HW subsystems

>> behind PCIe BAR. Thus libfdt functions are called for driver to parse

> for the driver to parse the

will fix.
>> device tree blob.

>>

>> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>

>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>

>> Signed-off-by: Lizhi Hou <lizhih@xilinx.com>

>> ---

>>   drivers/fpga/xrt/include/metadata.h  | 229 ++++++++++++

>>   drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++

>>   2 files changed, 753 insertions(+)

>>   create mode 100644 drivers/fpga/xrt/include/metadata.h

>>   create mode 100644 drivers/fpga/xrt/metadata/metadata.c

>>

>> diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h

>> new file mode 100644

>> index 000000000000..b929bc469b73

>> --- /dev/null

>> +++ b/drivers/fpga/xrt/include/metadata.h

>> @@ -0,0 +1,229 @@

>> +/* SPDX-License-Identifier: GPL-2.0 */

>> +/*

>> + * Header file for Xilinx Runtime (XRT) driver

>> + *

>> + * Copyright (C) 2020-2021 Xilinx, Inc.

>> + *

>> + * Authors:

>> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

>> + */

>> +

>> +#ifndef _XRT_METADATA_H

>> +#define _XRT_METADATA_H

>> +

>> +#include <linux/device.h>

>> +#include <linux/vmalloc.h>

>> +#include <linux/uuid.h>

>> +

>> +#define XRT_MD_INVALID_LENGTH (~0UL)

>> +

> These #defines could be in alphabetical order

>

> Some #define with embedded acronyms could be expanded

>

> ex/ XRT_MD_NODE_CMC_REG , what is CMC ?

Will reorder. Expanding might make macro name too long. I will add 
comment as below:
/*
* IP nodes
* AF: AXI Firewall
* CMC: Card Management Controller
* ERT: Embedded Runtime
* PLP: Provider Reconfigurable Partition
* ULP: User Reconfigurable Partition
*/
>

>> +#define XRT_MD_PROP_COMPATIBLE "compatible"

>> +#define XRT_MD_PROP_PF_NUM "pcie_physical_function"

>> +#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping"

>> +#define XRT_MD_PROP_IO_OFFSET "reg"

>> +#define XRT_MD_PROP_INTERRUPTS "interrupts"

>> +#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid"

>> +#define XRT_MD_PROP_LOGIC_UUID "logic_uuid"

>> +#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major"

>> +

>> +#define XRT_MD_PROP_HWICAP "axi_hwicap"

>> +#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem"

>> +

>> +#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints"

>> +#define XRT_MD_INTERFACES_PATH "/interfaces"

>> +

>> +#define XRT_MD_NODE_FIRMWARE "firmware"

>> +#define XRT_MD_NODE_INTERFACES "interfaces"

>> +#define XRT_MD_NODE_PARTITION_INFO "partition_info"

>> +

>> +#define XRT_MD_NODE_FLASH "ep_card_flash_program_00"

>> +#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00"

>> +#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00"

>> +#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00"

>> +#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00"

>> +#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00"

>> +#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00"

>> +#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00"

>> +#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00"

>> +#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00"

>> +#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00"

>> +#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00"

>> +#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00"

>> +#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00"

>> +#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00"

>> +#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00"

>> +#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00"

>> +#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00"

>> +#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00"

>> +#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00"

>> +#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00"

>> +#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00"

>> +#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00"

>> +#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00"

>> +#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00"

>> +#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00"

>> +#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00"

>> +#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01"

>> +#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00"

>> +#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00"

>> +#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00"

>> +#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00"

>> +#define XRT_MD_NODE_XDMA "ep_xdma_00"

>> +#define XRT_MD_NODE_MSIX "ep_msix_00"

>> +#define XRT_MD_NODE_QDMA "ep_qdma_00"

>> +#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00"

>> +#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00"

>> +#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00"

>> +#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00"

>> +#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00"

>> +#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00"

>> +#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00"

>> +#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01"

>> +#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00"

>> +#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00"

>> +#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00"

>> +#define XRT_MD_NODE_P2P "ep_p2p_00"

>> +#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00"

>> +#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00"

>> +#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00"

>> +#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00"

>> +#define XRT_MD_NODE_PMC_INTR   "ep_pmc_intr_00"

>> +#define XRT_MD_NODE_PMC_MUX    "ep_pmc_mux_00"

>> +

>> +/* driver defined endpoints */

>> +#define XRT_MD_NODE_VSEC "drv_ep_vsec_00"

>> +#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00"

>> +#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00"

>> +#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00"

>> +#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00"

>> +#define XRT_MD_NODE_TEST "drv_ep_test_00"

>> +#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00"

>> +#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00"

>> +#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00"

>> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0"

>> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1"

>> +

>> +#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr"

>> +#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr"

>> +

>> +#define XRT_MD_PROP_OFFSET "drv_offset"

>> +#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency"

>> +#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter"

>> +#define XRT_MD_PROP_VBNV "vbnv"

>> +#define XRT_MD_PROP_VROM "vrom"

>> +#define XRT_MD_PROP_PARTITION_LEVEL "partition_level"

>> +

>> +struct xrt_md_endpoint {

>> +     const char      *ep_name;

>> +     u32             bar;

>> +     long            bar_off;

>> +     ulong           size;

>> +     char            *regmap;

>> +     char            *regmap_ver;

>> +};

> could the existing 'struct regmap' be used here ?

This structure is just a direct translation of firmware node. It is 
simple description of hardware endpoint.
>> +

>> +/* Note: res_id is defined by leaf driver and must start with 0. */

>> +struct xrt_iores_map {

>> +     char            *res_name;

>> +     int             res_id;

>> +};

>> +

>> +static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map,

>> +                                  int entry_num, const char *res_name)

>> +{

>> +     int i;

>> +

>> +     for (i = 0; i < entry_num; i++) {

>> +             if (!strcmp(res_name, res_map->res_name))

> Use the 'n' variant strncmp for better safety.

>

> Fix generally.

Will replace all strcmp()
>

>> +                     return res_map->res_id;

>> +             res_map++;

>> +     }

>> +     return -1;

>> +}

>> +

>> +static inline const char *

>> +xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id)

>> +{

>> +     int i;

>> +

>> +     for (i = 0; i < entry_num; i++) {

>> +             if (res_map->res_id == id)

>> +                     return res_map->res_name;

>> +             res_map++;

>> +     }

>> +     return NULL;

>> +}

>> +

>> +unsigned long xrt_md_size(struct device *dev, const char *blob);

>> +int xrt_md_create(struct device *dev, char **blob);

>> +int xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                     struct xrt_md_endpoint *ep);

>> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

>> +                     char *regmap_name);

>> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void **val, int *size);

>> +int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void *val, int size);

>> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char *new_ep_name);

>> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

>> +                          const char *ep_name,  const char *regmap_name,

>> +                          char **next_ep, char **next_regmap);

>> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

>> +                                const char *regmap_name, const char **ep_name);

>> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char **epname);

>> +void xrt_md_pack(struct device *dev, char *blob);

>> +char *xrt_md_dup(struct device *dev, const char *blob);

>> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

>> +                       u32 *num_uuids, uuid_t *intf_uuids);

>> +static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob)

>> +{

>> +     return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS,

>> +                                 NULL, NULL);

> A wrapping a single call seems like an unnecessary abstraction layer.

>

> can this be reduced/removed ?

Will remove.
>

>> +}

>> +

>> +/*

>> + * Firmware provides 128 bit hash string as unque id of partition/interface.

> The firmware provides a 128 bit hash string as a unique id to the partition/interface.

>

> Existing hw does not yet use the cononical form, so it is necessary to use a translation function.

Will fix
>

>> + * This string will be canonical textual representation in the future.

>> + * Before that, introducing these two functions below to translate

>> + * hash string to uuid_t for released hardware.

> Is there an existing version string the new hw will use to check which way to go ?

We do not have the new hw yet.
>> + */

>> +static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr)

>> +{

>> +     int i, p;

>> +     u8 *u = (u8 *)uuid;

>> +

>> +     for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--)

> This loop needs to be improved.

>

> Consider if sizeof(uuid_t) changed, accessing u[] would overflow.

Will fix all the issues of uuid2str()/str2uuid() you pointed out. The 
rough idea is to use export_uuid() and import_uuid().
>

>> +             (void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]);

> (void) cast isn't needed.

>> +}

>> +

>> +static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid)

>> +{

>> +     char *p;

>> +     const char *str;

>> +     char tmp[3] = { 0 };

>> +     int i, ret;

>> +

>> +     memset(p_uuid, 0, sizeof(*p_uuid));

>> +     p = (char *)p_uuid;

> access with p_uuid->b[] would remove use of 'p'

>> +     str = uuidstr + strlen(uuidstr) - 2;

>> +

>> +     for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) {

> is not filling p_uuid completely really ok ?

>> +             tmp[0] = *str;

>> +             tmp[1] = *(str + 1);

>> +             ret = kstrtou8(tmp, 16, p);

>> +             if (ret)

>> +                     return -EINVAL;

>> +             p++;

>> +             str -= 2;

>> +     }

>> +

>> +     return 0;

>> +}

>> +

>> +#endif

>> diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c

>> new file mode 100644

>> index 000000000000..5d106396f438

>> --- /dev/null

>> +++ b/drivers/fpga/xrt/metadata/metadata.c

>> @@ -0,0 +1,524 @@

>> +// SPDX-License-Identifier: GPL-2.0

>> +/*

>> + * Xilinx Alveo FPGA Metadata parse APIs

>> + *

>> + * Copyright (C) 2020-2021 Xilinx, Inc.

>> + *

>> + * Authors:

>> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

>> + */

>> +

>> +#include <linux/libfdt_env.h>

>> +#include "libfdt.h"

>> +#include "metadata.h"

>> +

>> +#define MAX_BLOB_SIZE        (4096 * 25)

>> +

>> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

>> +                       const char *prop, const void *val, int size);

>> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

>> +                       const char *overlay_blob, int overlay_offset);

>> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

>> +                            const char *ep_name, const char *regmap_name,

>> +                            int *ep_offset);

> consider reordering functions so these fwd decl's are not needed

Will remove decl's.
>> +

>> +unsigned long xrt_md_size(struct device *dev, const char *blob)

>> +{

>> +     unsigned long len = (long)fdt_totalsize(blob);

>> +

>> +     len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len;

>> +     return len;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_size);

>> +

>> +int xrt_md_create(struct device *dev, char **blob)

>> +{

>> +     int ret = 0;

>> +

>> +     WARN_ON(!blob);

>> +

>> +     *blob = vmalloc(MAX_BLOB_SIZE);

> why vmalloc instead of vzalloc ?

Will use vzalloc.
>> +     if (!*blob)

>> +             return -ENOMEM;

>> +

>> +     ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE);

>> +     if (ret) {

>> +             dev_err(dev, "format blob failed, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     ret = fdt_next_node(*blob, -1, NULL);

> A variable called 'offset' would make more sense here because it is used later in fdt_add_subnode as the parentoffset parameter.

the following fdt_add_subnode should use 0 as the parentoffset 
parameter. So I will keep 'ret' here.
>

>> +     if (ret < 0) {

>> +             dev_err(dev, "No Node, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS);

> why isn't the parentoffset '0' ?

Yes, 0 should be used as parentoffset.
>

>

>> +     if (ret < 0) {

>> +             dev_err(dev, "add node failed, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     return 0;

>> +

>> +failed:

>> +     vfree(*blob);

>> +     *blob = NULL;

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_create);

>> +

>> +static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset,

>> +                        const char *ep_name)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_add_subnode(blob, parent_offset, ep_name);

>> +     if (ret < 0 && ret != -FDT_ERR_EXISTS)

>> +             dev_err(dev, "failed to add node %s. %d", ep_name, ret);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

>> +                     char *regmap_name)

>> +{

>> +     int ret;

>> +     int ep_offset;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset);

>> +     if (ret) {

>> +             dev_err(dev, "can not find ep %s", ep_name);

>> +             return -EINVAL;

>> +     }

>> +

>> +     ret = fdt_del_node(blob, ep_offset);

>> +     if (ret)

>> +             dev_err(dev, "delete node %s failed, ret %d", ep_name, ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_del_endpoint);

>> +

>> +static int __xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                              struct xrt_md_endpoint *ep, int *offset, bool root)

>> +{

>> +     int ret = 0;

>> +     int ep_offset = 0;

>> +     u32 val, count = 0;

>> +     u64 io_range[2];

>> +     char comp[128];

>> +

>> +     if (!ep->ep_name) {

>> +             dev_err(dev, "empty name");

>> +             return -EINVAL;

>> +     }

>> +

>> +     if (!root) {

>> +             ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

>> +                                       &ep_offset);

>> +             if (ret) {

>> +                     dev_err(dev, "invalid blob, ret = %d", ret);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name);

>> +     if (ep_offset < 0) {

>> +             dev_err(dev, "add endpoint failed, ret = %d", ret);

>> +             return -EINVAL;

>> +     }

>> +     if (offset)

>> +             *offset = ep_offset;

>> +

>> +     if (ep->size != 0) {

>> +             val = cpu_to_be32(ep->bar);

>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX,

>> +                                  &val, sizeof(u32));

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_BAR_IDX, ret);

>> +                     goto failed;

>> +             }

>> +             io_range[0] = cpu_to_be64((u64)ep->bar_off);

>> +             io_range[1] = cpu_to_be64((u64)ep->size);

> is there a type mismatch between bar (u32) and size (u64) ?

Do you mean ep->bar? 'ep->bar' is bar index. So it does not have type 
mismatch issue.
>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET,

>> +                                  io_range, sizeof(io_range));

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_IO_OFFSET, ret);

>> +                     goto failed;

>> +             }

>> +     }

>> +

>> +     if (ep->regmap) {

>> +             if (ep->regmap_ver) {

>> +                     count = snprintf(comp, sizeof(comp),

>> +                                      "%s-%s", ep->regmap, ep->regmap_ver);

>> +                     count++;

>> +             }

>> +

>> +             count += snprintf(comp + count, sizeof(comp) - count,

>> +                               "%s", ep->regmap);

>> +             count++;

> unlikely, but overflow is not checked.

Will add overflow check.
>

> are multiple null's in this string ok ?

yes.
>

>> +

>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE,

>> +                                  comp, count);

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_COMPATIBLE, ret);

>> +                     goto failed;

>> +             }

>> +     }

>> +

>> +failed:

>> +     if (ret)

>> +             xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                     struct xrt_md_endpoint *ep)

>> +{

>> +     return __xrt_md_add_endpoint(dev, blob, ep, NULL, false);

> ok, user doesn't add root's endpoint.

>

> could an assert be added ?

The word 'root' is misleading here. Will change this argument to 'char 
*parent'
>

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_add_endpoint);

>> +

>> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

>> +                            const char *ep_name, const char *regmap_name,

>> +                            int *ep_offset)

>> +{

>> +     int offset;

>> +     const char *name;

>> +

>> +     for (offset = fdt_next_node(blob, -1, NULL);

>> +         offset >= 0;

>> +         offset = fdt_next_node(blob, offset, NULL)) {

>> +             name = fdt_get_name(blob, offset, NULL);

>> +             if (!name || strncmp(name, ep_name, strlen(ep_name) + 1))

> strlen() + 1 ?

This is to make sure "12345" and "123" are different.
>> +                     continue;

>> +             if (!regmap_name ||

>> +                 !fdt_node_check_compatible(blob, offset, regmap_name))

>> +                     break;

>> +     }

>> +     if (offset < 0)

>> +             return -ENODEV;

>> +

>> +     *ep_offset = offset;

>> +

>> +     return 0;

>> +}

>> +

>> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char **epname)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                               &offset);

>> +     if (!ret && epname && offset >= 0)

> The offset >= 0 check isn't needed, !ret is enough

Will fix.
>> +             *epname = fdt_get_name(blob, offset, NULL);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_find_endpoint);

>> +

>> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void **val, int *size)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     if (val)

>> +             *val = NULL;

> Seems like no point in making this call if !val

>

> Return -EINVAL if !val and remove the if (val) check below.

Will fix
>

>> +     if (ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                                       &offset);

>> +             if (ret) {

>> +                     dev_err(dev, "cannot get ep %s, regmap %s, ret = %d",

>> +                             ep_name, regmap_name, ret);

>> +                     return -EINVAL;

>> +             }

>> +     } else {

>> +             offset = fdt_next_node(blob, -1, NULL);

>> +             if (offset < 0) {

>> +                     dev_err(dev, "internal error, ret = %d", offset);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     if (val) {

>> +             *val = fdt_getprop(blob, offset, prop, size);

>> +             if (!*val) {

>> +                     dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_prop);

>> +

>> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

>> +                       const char *prop, const void *val, int size)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_setprop(blob, offset, prop, val, size);

>> +     if (ret)

>> +             dev_err(dev, "failed to set prop %d", ret);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_set_prop(struct device *dev, char *blob,

>> +                 const char *ep_name, const char *regmap_name,

>> +                 const char *prop, const void *val, int size)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     if (ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name,

>> +                                       regmap_name, &offset);

>> +             if (ret) {

>> +                     dev_err(dev, "cannot get node %s, ret = %d",

>> +                             ep_name, ret);

>> +                     return -EINVAL;

>> +             }

>> +     } else {

>> +             offset = fdt_next_node(blob, -1, NULL);

>> +             if (offset < 0) {

>> +                     dev_err(dev, "internal error, ret = %d", offset);

>> +                     return -EINVAL;

>> +             }

>> +     }

> This if-else block is a cut-n-paste from above.

>

> It is good to convert common logic blocks to macros or inline functions.

Will define a in-line function.
>

>> +

>> +     ret = xrt_md_setprop(dev, blob, offset, prop, val, size);

>> +     if (ret)

>> +             dev_err(dev, "set prop %s failed, ret = %d", prop, ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_set_prop);

>> +

>> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char *new_ep_name)

>> +{

>> +     int offset, target;

>> +     int ret;

>> +     struct xrt_md_endpoint ep = {0};

>> +     const char *newepnm = new_ep_name ? new_ep_name : ep_name;

> How is this valid ?

>

> The xrt_md_get_endpoint searches by ep_name and if there names are not unique the second one will never be found.

ep_name in the firmware will always be unique. It is documented in xrt.rst
>

>> +

>> +     ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name,

>> +                               &offset);

>> +     if (ret)

>> +             return -EINVAL;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target);

>> +     if (ret) {

>> +             ep.ep_name = newepnm;

>> +             ret = __xrt_md_add_endpoint(dev, blob, &ep, &target,

>> +                                         fdt_parent_offset(src_blob, offset) == 0);

>> +             if (ret)

>> +                     return -EINVAL;

>> +     }

>> +

>> +     ret = xrt_md_overlay(dev, blob, target, src_blob, offset);

>> +     if (ret)

>> +             dev_err(dev, "overlay failed, ret = %d", ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint);

>> +

>> +char *xrt_md_dup(struct device *dev, const char *blob)

>> +{

>> +     int ret;

>> +     char *dup_blob;

>> +

>> +     ret = xrt_md_create(dev, &dup_blob);

>> +     if (ret)

>> +             return NULL;

>> +     ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1);

> would memcpy-ing the blob work ?

Sure. Memcpy works. I will remove xrt_md_dup() and use memcpy.
>> +     if (ret) {

>> +             vfree(dup_blob);

>> +             return NULL;

>> +     }

>> +

>> +     return dup_blob;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_dup);

>> +

>> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

>> +                       const char *overlay_blob, int overlay_offset)

>> +{

>> +     int     property, subnode;

>> +     int     ret;

>> +

>> +     WARN_ON(!blob || !overlay_blob);

>> +

>> +     if (!blob) {

>> +             dev_err(dev, "blob is NULL");

>> +             return -EINVAL;

>> +     }

>> +

>> +     if (target < 0) {

>> +             target = fdt_next_node(blob, -1, NULL);

>> +             if (target < 0) {

>> +                     dev_err(dev, "invalid target");

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +     if (overlay_offset < 0) {

>> +             overlay_offset = fdt_next_node(overlay_blob, -1, NULL);

>> +             if (overlay_offset < 0) {

>> +                     dev_err(dev, "invalid overlay");

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     fdt_for_each_property_offset(property, overlay_blob, overlay_offset) {

>> +             const char *name;

>> +             const void *prop;

>> +             int prop_len;

>> +

>> +             prop = fdt_getprop_by_offset(overlay_blob, property, &name,

>> +                                          &prop_len);

>> +             if (!prop || prop_len >= MAX_BLOB_SIZE) {

> could add prop_len < 0 as another sanity check

Will add.
>> +                     dev_err(dev, "internal error");

>> +                     return -EINVAL;

>> +             }

>> +

>> +             ret = xrt_md_setprop(dev, blob, target, name, prop,

>> +                                  prop_len);

>> +             if (ret) {

>> +                     dev_err(dev, "setprop failed, ret = %d", ret);

>> +                     return ret;

> overlay_blob is half done, as an error handling shouldn't it be undone ?

The error will only cause a half done blob. Nothing need to be undone.
>> +             }

>> +     }

>> +

>> +     fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) {

>> +             const char *name = fdt_get_name(overlay_blob, subnode, NULL);

>> +             int nnode;

>> +

>> +             nnode = xrt_md_add_node(dev, blob, target, name);

>> +             if (nnode == -FDT_ERR_EXISTS)

>> +                     nnode = fdt_subnode_offset(blob, target, name);

>> +             if (nnode < 0) {

>> +                     dev_err(dev, "add node failed, ret = %d", nnode);

>> +                     return nnode;

>> +             }

>> +

>> +             ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode);

> eek, recursion.

>

> Any chance this will blow the stack ?

Will add max recursive depth (5).
>

>> +             if (ret)

>> +                     return ret;

>> +     }

>> +

>> +     return 0;

>> +}

>> +

>> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

>> +                          const char *ep_name, const char *regmap_name,

>> +                          char **next_ep, char **next_regmap)

>> +{

>> +     int offset, ret;

>> +

>> +     if (!ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

>> +                                       &offset);

>> +     } else {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                                       &offset);

>> +     }

>> +

>> +     if (ret) {

>> +             *next_ep = NULL;

>> +             *next_regmap = NULL;

> could initialize next_ep and next_regmap to NULL outside the check.

Will move outside.
>> +             return -EINVAL;

>> +     }

>> +

>> +     offset = ep_name ? fdt_next_subnode(blob, offset) :

>> +             fdt_first_subnode(blob, offset);

>> +     if (offset < 0) {

>> +             *next_ep = NULL;

>> +             *next_regmap = NULL;

>> +             return -EINVAL;

>> +     }

>> +

>> +     *next_ep = (char *)fdt_get_name(blob, offset, NULL);

>> +     *next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE,

>> +                                               0, NULL);

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint);

>> +

>> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

>> +                                const char *regmap_name, const char **ep_name)

>> +{

>> +     int ep_offset;

>> +

>> +     ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name);

>> +     if (ep_offset < 0) {

>> +             *ep_name = NULL;

>> +             return -ENOENT;

>> +     }

>> +

>> +     *ep_name = (char *)fdt_get_name(blob, ep_offset, NULL);

> why the cast ?

Will remove.
>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint);

>> +

>> +void xrt_md_pack(struct device *dev, char *blob)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_pack(blob);

>> +     if (ret)

>> +             dev_err(dev, "pack failed %d", ret);

> maybe return int

Will return int.
>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_pack);

>> +

> The input/output of num_uuids parameter is tricky add a comment

Will modify to 'u32 num_uuids'  and return the actual number
>> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

>> +                       u32 *num_uuids, uuid_t *intf_uuids)

> what is intf ? change to 'interface' ?

yes. And will change to 'interface'.
>> +{

>> +     int offset, count = 0;

>> +     int ret;

>> +     const char *uuid_str;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset);

>> +     if (ret)

>> +             return -ENOENT;

>> +

>> +     for (offset = fdt_first_subnode(blob, offset);

>> +         offset >= 0;

>> +         offset = fdt_next_subnode(blob, offset)) {

>> +             uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID,

>> +                                    NULL);

>> +             if (!uuid_str) {

>> +                     dev_err(dev, "empty intf uuid node");

>> +                     return -EINVAL;

>> +             }

>> +

>> +             if (intf_uuids && count < *num_uuids) {

>> +                     ret = xrt_md_trans_str2uuid(dev, uuid_str,

>> +                                                 &intf_uuids[count]);

>> +                     if (ret)

>> +                             return -EINVAL;

>> +             }

>> +             count++;

> keep going even when count > num_uuids ?

>

> that seems like an error.

Will add check.

Thanks,
Lizhi
>

> Tom

>

>> +     }

>> +

>> +     *num_uuids = count;

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);
Lizhi Hou Feb. 25, 2021, 8:27 p.m. UTC | #4
Hi Fernando,


On 02/22/2021 05:23 PM, Fernando Pacheco wrote:
> On Wed, Feb 17, 2021 at 10:40:03PM -0800, Lizhi Hou wrote:

>> XRT drivers use device tree as metadata format to discover HW subsystems

>> behind PCIe BAR. Thus libfdt functions are called for driver to parse

>> device tree blob.

>>

>> Signed-off-by: Sonal Santan <sonal.santan@xilinx.com>

>> Signed-off-by: Max Zhen <max.zhen@xilinx.com>

>> Signed-off-by: Lizhi Hou <lizhih@xilinx.com>

>> ---

>>   drivers/fpga/xrt/include/metadata.h  | 229 ++++++++++++

>>   drivers/fpga/xrt/metadata/metadata.c | 524 +++++++++++++++++++++++++++

>>   2 files changed, 753 insertions(+)

>>   create mode 100644 drivers/fpga/xrt/include/metadata.h

>>   create mode 100644 drivers/fpga/xrt/metadata/metadata.c

>>

>> diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h

>> new file mode 100644

>> index 000000000000..b929bc469b73

>> --- /dev/null

>> +++ b/drivers/fpga/xrt/include/metadata.h

>> @@ -0,0 +1,229 @@

>> +/* SPDX-License-Identifier: GPL-2.0 */

>> +/*

>> + * Header file for Xilinx Runtime (XRT) driver

>> + *

>> + * Copyright (C) 2020-2021 Xilinx, Inc.

>> + *

>> + * Authors:

>> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

>> + */

>> +

>> +#ifndef _XRT_METADATA_H

>> +#define _XRT_METADATA_H

>> +

>> +#include <linux/device.h>

>> +#include <linux/vmalloc.h>

>> +#include <linux/uuid.h>

>> +

>> +#define XRT_MD_INVALID_LENGTH (~0UL)

>> +

>> +#define XRT_MD_PROP_COMPATIBLE "compatible"

>> +#define XRT_MD_PROP_PF_NUM "pcie_physical_function"

>> +#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping"

>> +#define XRT_MD_PROP_IO_OFFSET "reg"

>> +#define XRT_MD_PROP_INTERRUPTS "interrupts"

>> +#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid"

>> +#define XRT_MD_PROP_LOGIC_UUID "logic_uuid"

>> +#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major"

>> +

>> +#define XRT_MD_PROP_HWICAP "axi_hwicap"

>> +#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem"

>> +

>> +#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints"

>> +#define XRT_MD_INTERFACES_PATH "/interfaces"

>> +

>> +#define XRT_MD_NODE_FIRMWARE "firmware"

>> +#define XRT_MD_NODE_INTERFACES "interfaces"

>> +#define XRT_MD_NODE_PARTITION_INFO "partition_info"

>> +

>> +#define XRT_MD_NODE_FLASH "ep_card_flash_program_00"

>> +#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00"

>> +#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00"

>> +#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00"

>> +#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00"

>> +#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00"

>> +#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00"

>> +#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00"

>> +#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00"

>> +#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00"

>> +#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00"

>> +#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00"

>> +#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00"

>> +#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00"

>> +#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00"

>> +#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00"

>> +#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00"

>> +#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00"

>> +#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00"

>> +#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00"

>> +#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00"

>> +#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00"

>> +#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00"

>> +#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00"

>> +#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00"

>> +#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00"

>> +#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00"

>> +#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01"

>> +#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00"

>> +#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00"

>> +#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00"

>> +#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00"

>> +#define XRT_MD_NODE_XDMA "ep_xdma_00"

>> +#define XRT_MD_NODE_MSIX "ep_msix_00"

>> +#define XRT_MD_NODE_QDMA "ep_qdma_00"

>> +#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00"

>> +#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00"

>> +#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00"

>> +#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00"

>> +#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00"

>> +#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00"

>> +#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00"

>> +#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01"

>> +#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00"

>> +#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00"

>> +#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00"

>> +#define XRT_MD_NODE_P2P "ep_p2p_00"

>> +#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00"

>> +#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00"

>> +#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00"

>> +#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00"

>> +#define XRT_MD_NODE_PMC_INTR   "ep_pmc_intr_00"

>> +#define XRT_MD_NODE_PMC_MUX    "ep_pmc_mux_00"

>> +

>> +/* driver defined endpoints */

>> +#define XRT_MD_NODE_VSEC "drv_ep_vsec_00"

>> +#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00"

>> +#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00"

>> +#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00"

> Should this be "drv_ep_mailbox_vsec_00"?

Will fix this.
>

>> +#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00"

>> +#define XRT_MD_NODE_TEST "drv_ep_test_00"

>> +#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00"

>> +#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00"

>> +#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00"

>> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0"

>> +#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1"

>> +

>> +#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr"

>> +#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr"

>> +

>> +#define XRT_MD_PROP_OFFSET "drv_offset"

>> +#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency"

>> +#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter"

>> +#define XRT_MD_PROP_VBNV "vbnv"

>> +#define XRT_MD_PROP_VROM "vrom"

>> +#define XRT_MD_PROP_PARTITION_LEVEL "partition_level"

>> +

>> +struct xrt_md_endpoint {

>> +     const char      *ep_name;

>> +     u32             bar;

>> +     long            bar_off;

>> +     ulong           size;

>> +     char            *regmap;

>> +     char            *regmap_ver;

>> +};

>> +

>> +/* Note: res_id is defined by leaf driver and must start with 0. */

>> +struct xrt_iores_map {

>> +     char            *res_name;

>> +     int             res_id;

>> +};

>> +

>> +static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map,

>> +                                  int entry_num, const char *res_name)

>> +{

>> +     int i;

>> +

>> +     for (i = 0; i < entry_num; i++) {

>> +             if (!strcmp(res_name, res_map->res_name))

>> +                     return res_map->res_id;

>> +             res_map++;

>> +     }

>> +     return -1;

>> +}

>> +

>> +static inline const char *

>> +xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id)

>> +{

>> +     int i;

>> +

>> +     for (i = 0; i < entry_num; i++) {

>> +             if (res_map->res_id == id)

>> +                     return res_map->res_name;

>> +             res_map++;

>> +     }

>> +     return NULL;

>> +}

>> +

>> +unsigned long xrt_md_size(struct device *dev, const char *blob);

>> +int xrt_md_create(struct device *dev, char **blob);

>> +int xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                     struct xrt_md_endpoint *ep);

>> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

>> +                     char *regmap_name);

>> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void **val, int *size);

>> +int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void *val, int size);

>> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char *new_ep_name);

>> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

>> +                          const char *ep_name,  const char *regmap_name,

>> +                          char **next_ep, char **next_regmap);

>> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

>> +                                const char *regmap_name, const char **ep_name);

>> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char **epname);

>> +void xrt_md_pack(struct device *dev, char *blob);

>> +char *xrt_md_dup(struct device *dev, const char *blob);

>> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

>> +                       u32 *num_uuids, uuid_t *intf_uuids);

>> +static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob)

>> +{

>> +     return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS,

>> +                                 NULL, NULL);

>> +}

>> +

>> +/*

>> + * Firmware provides 128 bit hash string as unque id of partition/interface.

>> + * This string will be canonical textual representation in the future.

>> + * Before that, introducing these two functions below to translate

>> + * hash string to uuid_t for released hardware.

>> + */

>> +static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr)

>> +{

>> +     int i, p;

>> +     u8 *u = (u8 *)uuid;

>> +

>> +     for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--)

>> +             (void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]);

>> +}

>> +

>> +static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid)

>> +{

>> +     char *p;

>> +     const char *str;

>> +     char tmp[3] = { 0 };

>> +     int i, ret;

>> +

>> +     memset(p_uuid, 0, sizeof(*p_uuid));

>> +     p = (char *)p_uuid;

>> +     str = uuidstr + strlen(uuidstr) - 2;

>> +

>> +     for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) {

>> +             tmp[0] = *str;

>> +             tmp[1] = *(str + 1);

>> +             ret = kstrtou8(tmp, 16, p);

>> +             if (ret)

>> +                     return -EINVAL;

>> +             p++;

>> +             str -= 2;

>> +     }

>> +

>> +     return 0;

>> +}

>> +

>> +#endif

>> diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c

>> new file mode 100644

>> index 000000000000..5d106396f438

>> --- /dev/null

>> +++ b/drivers/fpga/xrt/metadata/metadata.c

>> @@ -0,0 +1,524 @@

>> +// SPDX-License-Identifier: GPL-2.0

>> +/*

>> + * Xilinx Alveo FPGA Metadata parse APIs

>> + *

>> + * Copyright (C) 2020-2021 Xilinx, Inc.

>> + *

>> + * Authors:

>> + *      Lizhi Hou <Lizhi.Hou@xilinx.com>

>> + */

>> +

>> +#include <linux/libfdt_env.h>

>> +#include "libfdt.h"

>> +#include "metadata.h"

>> +

>> +#define MAX_BLOB_SIZE        (4096 * 25)

>> +

>> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

>> +                       const char *prop, const void *val, int size);

>> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

>> +                       const char *overlay_blob, int overlay_offset);

>> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

>> +                            const char *ep_name, const char *regmap_name,

>> +                            int *ep_offset);

>> +

>> +unsigned long xrt_md_size(struct device *dev, const char *blob)

>> +{

>> +     unsigned long len = (long)fdt_totalsize(blob);

>> +

>> +     len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len;

> how about (avoiding len = len):

>

>          if (len > MAX_BLOB_SIZE)

>                  return XRT_MD_INVALID_LENGTH;

>

>          return len;

Agree. Will fix.
>> +     return len;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_size);

>> +

>> +int xrt_md_create(struct device *dev, char **blob)

>> +{

>> +     int ret = 0;

>> +

>> +     WARN_ON(!blob);

> Shouldn't this be signaled to caller as a -EINVAL?

Agree. Will fix it.
>

>> +

>> +     *blob = vmalloc(MAX_BLOB_SIZE);

>> +     if (!*blob)

>> +             return -ENOMEM;

>> +

>> +     ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE);

>> +     if (ret) {

>> +             dev_err(dev, "format blob failed, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     ret = fdt_next_node(*blob, -1, NULL);

>> +     if (ret < 0) {

>> +             dev_err(dev, "No Node, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS);

>> +     if (ret < 0) {

>> +             dev_err(dev, "add node failed, ret = %d", ret);

>> +             goto failed;

>> +     }

>> +

>> +     return 0;

>> +

>> +failed:

>> +     vfree(*blob);

>> +     *blob = NULL;

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_create);

>> +

>> +static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset,

>> +                        const char *ep_name)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_add_subnode(blob, parent_offset, ep_name);

>> +     if (ret < 0 && ret != -FDT_ERR_EXISTS)

>> +             dev_err(dev, "failed to add node %s. %d", ep_name, ret);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,

>> +                     char *regmap_name)

>> +{

>> +     int ret;

>> +     int ep_offset;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset);

>> +     if (ret) {

>> +             dev_err(dev, "can not find ep %s", ep_name);

>> +             return -EINVAL;

>> +     }

>> +

>> +     ret = fdt_del_node(blob, ep_offset);

>> +     if (ret)

>> +             dev_err(dev, "delete node %s failed, ret %d", ep_name, ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_del_endpoint);

>> +

>> +static int __xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                              struct xrt_md_endpoint *ep, int *offset, bool root)

>> +{

>> +     int ret = 0;

>> +     int ep_offset = 0;

>> +     u32 val, count = 0;

>> +     u64 io_range[2];

>> +     char comp[128];

>> +

>> +     if (!ep->ep_name) {

>> +             dev_err(dev, "empty name");

>> +             return -EINVAL;

>> +     }

>> +

>> +     if (!root) {

>> +             ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

>> +                                       &ep_offset);

>> +             if (ret) {

>> +                     dev_err(dev, "invalid blob, ret = %d", ret);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name);

> A little hard to follow when you re-use ep_offset like this. Maybe

> parent_offset to match xrt_md_add_node parameter?

Will add parent_offset.
>

>> +     if (ep_offset < 0) {

>> +             dev_err(dev, "add endpoint failed, ret = %d", ret);

>> +             return -EINVAL;

>> +     }

>> +     if (offset)

>> +             *offset = ep_offset;

>> +

>> +     if (ep->size != 0) {

>> +             val = cpu_to_be32(ep->bar);

>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX,

>> +                                  &val, sizeof(u32));

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_BAR_IDX, ret);

>> +                     goto failed;

>> +             }

>> +             io_range[0] = cpu_to_be64((u64)ep->bar_off);

>> +             io_range[1] = cpu_to_be64((u64)ep->size);

>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET,

>> +                                  io_range, sizeof(io_range));

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_IO_OFFSET, ret);

>> +                     goto failed;

>> +             }

>> +     }

>> +

>> +     if (ep->regmap) {

>> +             if (ep->regmap_ver) {

>> +                     count = snprintf(comp, sizeof(comp),

>> +                                      "%s-%s", ep->regmap, ep->regmap_ver);

>> +                     count++;

>> +             }

>> +

>> +             count += snprintf(comp + count, sizeof(comp) - count,

>> +                               "%s", ep->regmap);

>> +             count++;

>> +

>> +             ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE,

>> +                                  comp, count);

>> +             if (ret) {

>> +                     dev_err(dev, "set %s failed, ret %d",

>> +                             XRT_MD_PROP_COMPATIBLE, ret);

>> +                     goto failed;

>> +             }

>> +     }

>> +

>> +failed:

>> +     if (ret)

>> +             xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_add_endpoint(struct device *dev, char *blob,

>> +                     struct xrt_md_endpoint *ep)

>> +{

>> +     return __xrt_md_add_endpoint(dev, blob, ep, NULL, false);

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_add_endpoint);

>> +

>> +static int xrt_md_get_endpoint(struct device *dev, const char *blob,

>> +                            const char *ep_name, const char *regmap_name,

>> +                            int *ep_offset)

>> +{

>> +     int offset;

>> +     const char *name;

>> +

>> +     for (offset = fdt_next_node(blob, -1, NULL);

>> +         offset >= 0;

>> +         offset = fdt_next_node(blob, offset, NULL)) {

>> +             name = fdt_get_name(blob, offset, NULL);

>> +             if (!name || strncmp(name, ep_name, strlen(ep_name) + 1))

>> +                     continue;

>> +             if (!regmap_name ||

>> +                 !fdt_node_check_compatible(blob, offset, regmap_name))

>> +                     break;

>> +     }

>> +     if (offset < 0)

>> +             return -ENODEV;

>> +

>> +     *ep_offset = offset;

>> +

>> +     return 0;

>> +}

>> +

>> +int xrt_md_find_endpoint(struct device *dev, const char *blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char **epname)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                               &offset);

>> +     if (!ret && epname && offset >= 0)

>> +             *epname = fdt_get_name(blob, offset, NULL);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_find_endpoint);

>> +

>> +int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,

>> +                 const char *regmap_name, const char *prop,

>> +                 const void **val, int *size)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     if (val)

>> +             *val = NULL;

>> +     if (ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                                       &offset);

>> +             if (ret) {

>> +                     dev_err(dev, "cannot get ep %s, regmap %s, ret = %d",

>> +                             ep_name, regmap_name, ret);

>> +                     return -EINVAL;

>> +             }

>> +     } else {

>> +             offset = fdt_next_node(blob, -1, NULL);

>> +             if (offset < 0) {

>> +                     dev_err(dev, "internal error, ret = %d", offset);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     if (val) {

>> +             *val = fdt_getprop(blob, offset, prop, size);

>> +             if (!*val) {

>> +                     dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_prop);

>> +

>> +static int xrt_md_setprop(struct device *dev, char *blob, int offset,

>> +                       const char *prop, const void *val, int size)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_setprop(blob, offset, prop, val, size);

>> +     if (ret)

>> +             dev_err(dev, "failed to set prop %d", ret);

>> +

>> +     return ret;

>> +}

>> +

>> +int xrt_md_set_prop(struct device *dev, char *blob,

>> +                 const char *ep_name, const char *regmap_name,

>> +                 const char *prop, const void *val, int size)

>> +{

>> +     int offset;

>> +     int ret;

>> +

>> +     if (ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name,

>> +                                       regmap_name, &offset);

>> +             if (ret) {

>> +                     dev_err(dev, "cannot get node %s, ret = %d",

>> +                             ep_name, ret);

>> +                     return -EINVAL;

>> +             }

>> +     } else {

>> +             offset = fdt_next_node(blob, -1, NULL);

>> +             if (offset < 0) {

>> +                     dev_err(dev, "internal error, ret = %d", offset);

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     ret = xrt_md_setprop(dev, blob, offset, prop, val, size);

>> +     if (ret)

>> +             dev_err(dev, "set prop %s failed, ret = %d", prop, ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_set_prop);

>> +

>> +int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,

>> +                      const char *ep_name, const char *regmap_name,

>> +                      const char *new_ep_name)

>> +{

>> +     int offset, target;

>> +     int ret;

>> +     struct xrt_md_endpoint ep = {0};

>> +     const char *newepnm = new_ep_name ? new_ep_name : ep_name;

>> +

>> +     ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name,

>> +                               &offset);

>> +     if (ret)

>> +             return -EINVAL;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target);

>> +     if (ret) {

>> +             ep.ep_name = newepnm;

>> +             ret = __xrt_md_add_endpoint(dev, blob, &ep, &target,

>> +                                         fdt_parent_offset(src_blob, offset) == 0);

>> +             if (ret)

>> +                     return -EINVAL;

>> +     }

>> +

>> +     ret = xrt_md_overlay(dev, blob, target, src_blob, offset);

>> +     if (ret)

>> +             dev_err(dev, "overlay failed, ret = %d", ret);

>> +

>> +     return ret;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint);

>> +

>> +char *xrt_md_dup(struct device *dev, const char *blob)

>> +{

>> +     int ret;

>> +     char *dup_blob;

>> +

>> +     ret = xrt_md_create(dev, &dup_blob);

>> +     if (ret)

>> +             return NULL;

>> +     ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1);

>> +     if (ret) {

>> +             vfree(dup_blob);

>> +             return NULL;

>> +     }

>> +

>> +     return dup_blob;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_dup);

>> +

>> +static int xrt_md_overlay(struct device *dev, char *blob, int target,

>> +                       const char *overlay_blob, int overlay_offset)

>> +{

>> +     int     property, subnode;

>> +     int     ret;

>> +

>> +     WARN_ON(!blob || !overlay_blob);

> Why check !blob twice? Also, is a WARN_ON justified here

> (and in xrt_md_create)?

Using 'if' statement to check is good enough and I will remove WARN_ON 
from both places.

Thanks,
Lizhi
>

> Fernando

>

>> +

>> +     if (!blob) {

>> +             dev_err(dev, "blob is NULL");

>> +             return -EINVAL;

>> +     }

>> +

>> +     if (target < 0) {

>> +             target = fdt_next_node(blob, -1, NULL);

>> +             if (target < 0) {

>> +                     dev_err(dev, "invalid target");

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +     if (overlay_offset < 0) {

>> +             overlay_offset = fdt_next_node(overlay_blob, -1, NULL);

>> +             if (overlay_offset < 0) {

>> +                     dev_err(dev, "invalid overlay");

>> +                     return -EINVAL;

>> +             }

>> +     }

>> +

>> +     fdt_for_each_property_offset(property, overlay_blob, overlay_offset) {

>> +             const char *name;

>> +             const void *prop;

>> +             int prop_len;

>> +

>> +             prop = fdt_getprop_by_offset(overlay_blob, property, &name,

>> +                                          &prop_len);

>> +             if (!prop || prop_len >= MAX_BLOB_SIZE) {

>> +                     dev_err(dev, "internal error");

>> +                     return -EINVAL;

>> +             }

>> +

>> +             ret = xrt_md_setprop(dev, blob, target, name, prop,

>> +                                  prop_len);

>> +             if (ret) {

>> +                     dev_err(dev, "setprop failed, ret = %d", ret);

>> +                     return ret;

>> +             }

>> +     }

>> +

>> +     fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) {

>> +             const char *name = fdt_get_name(overlay_blob, subnode, NULL);

>> +             int nnode;

>> +

>> +             nnode = xrt_md_add_node(dev, blob, target, name);

>> +             if (nnode == -FDT_ERR_EXISTS)

>> +                     nnode = fdt_subnode_offset(blob, target, name);

>> +             if (nnode < 0) {

>> +                     dev_err(dev, "add node failed, ret = %d", nnode);

>> +                     return nnode;

>> +             }

>> +

>> +             ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode);

>> +             if (ret)

>> +                     return ret;

>> +     }

>> +

>> +     return 0;

>> +}

>> +

>> +int xrt_md_get_next_endpoint(struct device *dev, const char *blob,

>> +                          const char *ep_name, const char *regmap_name,

>> +                          char **next_ep, char **next_regmap)

>> +{

>> +     int offset, ret;

>> +

>> +     if (!ep_name) {

>> +             ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,

>> +                                       &offset);

>> +     } else {

>> +             ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,

>> +                                       &offset);

>> +     }

>> +

>> +     if (ret) {

>> +             *next_ep = NULL;

>> +             *next_regmap = NULL;

>> +             return -EINVAL;

>> +     }

>> +

>> +     offset = ep_name ? fdt_next_subnode(blob, offset) :

>> +             fdt_first_subnode(blob, offset);

>> +     if (offset < 0) {

>> +             *next_ep = NULL;

>> +             *next_regmap = NULL;

>> +             return -EINVAL;

>> +     }

>> +

>> +     *next_ep = (char *)fdt_get_name(blob, offset, NULL);

>> +     *next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE,

>> +                                               0, NULL);

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint);

>> +

>> +int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,

>> +                                const char *regmap_name, const char **ep_name)

>> +{

>> +     int ep_offset;

>> +

>> +     ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name);

>> +     if (ep_offset < 0) {

>> +             *ep_name = NULL;

>> +             return -ENOENT;

>> +     }

>> +

>> +     *ep_name = (char *)fdt_get_name(blob, ep_offset, NULL);

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint);

>> +

>> +void xrt_md_pack(struct device *dev, char *blob)

>> +{

>> +     int ret;

>> +

>> +     ret = fdt_pack(blob);

>> +     if (ret)

>> +             dev_err(dev, "pack failed %d", ret);

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_pack);

>> +

>> +int xrt_md_get_intf_uuids(struct device *dev, const char *blob,

>> +                       u32 *num_uuids, uuid_t *intf_uuids)

>> +{

>> +     int offset, count = 0;

>> +     int ret;

>> +     const char *uuid_str;

>> +

>> +     ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset);

>> +     if (ret)

>> +             return -ENOENT;

>> +

>> +     for (offset = fdt_first_subnode(blob, offset);

>> +         offset >= 0;

>> +         offset = fdt_next_subnode(blob, offset)) {

>> +             uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID,

>> +                                    NULL);

>> +             if (!uuid_str) {

>> +                     dev_err(dev, "empty intf uuid node");

>> +                     return -EINVAL;

>> +             }

>> +

>> +             if (intf_uuids && count < *num_uuids) {

>> +                     ret = xrt_md_trans_str2uuid(dev, uuid_str,

>> +                                                 &intf_uuids[count]);

>> +                     if (ret)

>> +                             return -EINVAL;

>> +             }

>> +             count++;

>> +     }

>> +

>> +     *num_uuids = count;

>> +

>> +     return 0;

>> +}

>> +EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);

>> --

>> 2.18.4

>>
diff mbox series

Patch

diff --git a/drivers/fpga/xrt/include/metadata.h b/drivers/fpga/xrt/include/metadata.h
new file mode 100644
index 000000000000..b929bc469b73
--- /dev/null
+++ b/drivers/fpga/xrt/include/metadata.h
@@ -0,0 +1,229 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Header file for Xilinx Runtime (XRT) driver
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *      Lizhi Hou <Lizhi.Hou@xilinx.com>
+ */
+
+#ifndef _XRT_METADATA_H
+#define _XRT_METADATA_H
+
+#include <linux/device.h>
+#include <linux/vmalloc.h>
+#include <linux/uuid.h>
+
+#define XRT_MD_INVALID_LENGTH (~0UL)
+
+#define XRT_MD_PROP_COMPATIBLE "compatible"
+#define XRT_MD_PROP_PF_NUM "pcie_physical_function"
+#define XRT_MD_PROP_BAR_IDX "pcie_bar_mapping"
+#define XRT_MD_PROP_IO_OFFSET "reg"
+#define XRT_MD_PROP_INTERRUPTS "interrupts"
+#define XRT_MD_PROP_INTERFACE_UUID "interface_uuid"
+#define XRT_MD_PROP_LOGIC_UUID "logic_uuid"
+#define XRT_MD_PROP_VERSION_MAJOR "firmware_version_major"
+
+#define XRT_MD_PROP_HWICAP "axi_hwicap"
+#define XRT_MD_PROP_PDI_CONFIG "pdi_config_mem"
+
+#define XRT_MD_NODE_ENDPOINTS "addressable_endpoints"
+#define XRT_MD_INTERFACES_PATH "/interfaces"
+
+#define XRT_MD_NODE_FIRMWARE "firmware"
+#define XRT_MD_NODE_INTERFACES "interfaces"
+#define XRT_MD_NODE_PARTITION_INFO "partition_info"
+
+#define XRT_MD_NODE_FLASH "ep_card_flash_program_00"
+#define XRT_MD_NODE_XVC_PUB "ep_debug_bscan_user_00"
+#define XRT_MD_NODE_XVC_PRI "ep_debug_bscan_mgmt_00"
+#define XRT_MD_NODE_SYSMON "ep_cmp_sysmon_00"
+#define XRT_MD_NODE_AF_BLP_CTRL_MGMT "ep_firewall_blp_ctrl_mgmt_00"
+#define XRT_MD_NODE_AF_BLP_CTRL_USER "ep_firewall_blp_ctrl_user_00"
+#define XRT_MD_NODE_AF_CTRL_MGMT "ep_firewall_ctrl_mgmt_00"
+#define XRT_MD_NODE_AF_CTRL_USER "ep_firewall_ctrl_user_00"
+#define XRT_MD_NODE_AF_CTRL_DEBUG "ep_firewall_ctrl_debug_00"
+#define XRT_MD_NODE_AF_DATA_H2C "ep_firewall_data_h2c_00"
+#define XRT_MD_NODE_AF_DATA_C2H "ep_firewall_data_c2h_00"
+#define XRT_MD_NODE_AF_DATA_P2P "ep_firewall_data_p2p_00"
+#define XRT_MD_NODE_AF_DATA_M2M "ep_firewall_data_m2m_00"
+#define XRT_MD_NODE_CMC_REG "ep_cmc_regmap_00"
+#define XRT_MD_NODE_CMC_RESET "ep_cmc_reset_00"
+#define XRT_MD_NODE_CMC_MUTEX "ep_cmc_mutex_00"
+#define XRT_MD_NODE_CMC_FW_MEM "ep_cmc_firmware_mem_00"
+#define XRT_MD_NODE_ERT_FW_MEM "ep_ert_firmware_mem_00"
+#define XRT_MD_NODE_ERT_CQ_MGMT "ep_ert_command_queue_mgmt_00"
+#define XRT_MD_NODE_ERT_CQ_USER "ep_ert_command_queue_user_00"
+#define XRT_MD_NODE_MAILBOX_MGMT "ep_mailbox_mgmt_00"
+#define XRT_MD_NODE_MAILBOX_USER "ep_mailbox_user_00"
+#define XRT_MD_NODE_GATE_PLP "ep_pr_isolate_plp_00"
+#define XRT_MD_NODE_GATE_ULP "ep_pr_isolate_ulp_00"
+#define XRT_MD_NODE_PCIE_MON "ep_pcie_link_mon_00"
+#define XRT_MD_NODE_DDR_CALIB "ep_ddr_mem_calib_00"
+#define XRT_MD_NODE_CLK_KERNEL1 "ep_aclk_kernel_00"
+#define XRT_MD_NODE_CLK_KERNEL2 "ep_aclk_kernel_01"
+#define XRT_MD_NODE_CLK_KERNEL3 "ep_aclk_hbm_00"
+#define XRT_MD_NODE_KDMA_CTRL "ep_kdma_ctrl_00"
+#define XRT_MD_NODE_FPGA_CONFIG "ep_fpga_configuration_00"
+#define XRT_MD_NODE_ERT_SCHED "ep_ert_sched_00"
+#define XRT_MD_NODE_XDMA "ep_xdma_00"
+#define XRT_MD_NODE_MSIX "ep_msix_00"
+#define XRT_MD_NODE_QDMA "ep_qdma_00"
+#define XRT_MD_XRT_MD_NODE_QDMA4 "ep_qdma4_00"
+#define XRT_MD_NODE_STM "ep_stream_traffic_manager_00"
+#define XRT_MD_NODE_STM4 "ep_stream_traffic_manager4_00"
+#define XRT_MD_NODE_CLK_SHUTDOWN "ep_aclk_shutdown_00"
+#define XRT_MD_NODE_ERT_BASE "ep_ert_base_address_00"
+#define XRT_MD_NODE_ERT_RESET "ep_ert_reset_00"
+#define XRT_MD_NODE_CLKFREQ_K1 "ep_freq_cnt_aclk_kernel_00"
+#define XRT_MD_NODE_CLKFREQ_K2 "ep_freq_cnt_aclk_kernel_01"
+#define XRT_MD_NODE_CLKFREQ_HBM "ep_freq_cnt_aclk_hbm_00"
+#define XRT_MD_NODE_GAPPING "ep_gapping_demand_00"
+#define XRT_MD_NODE_UCS_CONTROL_STATUS "ep_ucs_control_status_00"
+#define XRT_MD_NODE_P2P "ep_p2p_00"
+#define XRT_MD_NODE_REMAP_P2P "ep_remap_p2p_00"
+#define XRT_MD_NODE_DDR4_RESET_GATE "ep_ddr_mem_srsr_gate_00"
+#define XRT_MD_NODE_ADDR_TRANSLATOR "ep_remap_data_c2h_00"
+#define XRT_MD_NODE_MAILBOX_XRT "ep_mailbox_user_to_ert_00"
+#define XRT_MD_NODE_PMC_INTR   "ep_pmc_intr_00"
+#define XRT_MD_NODE_PMC_MUX    "ep_pmc_mux_00"
+
+/* driver defined endpoints */
+#define XRT_MD_NODE_VSEC "drv_ep_vsec_00"
+#define XRT_MD_NODE_VSEC_GOLDEN "drv_ep_vsec_golden_00"
+#define XRT_MD_NODE_BLP_ROM "drv_ep_blp_rom_00"
+#define XRT_MD_NODE_MAILBOX_VSEC "ep_mailbox_vsec_00"
+#define XRT_MD_NODE_PLAT_INFO "drv_ep_platform_info_mgmt_00"
+#define XRT_MD_NODE_TEST "drv_ep_test_00"
+#define XRT_MD_NODE_MGMT_MAIN "drv_ep_mgmt_main_00"
+#define XRT_MD_NODE_FLASH_VSEC "drv_ep_card_flash_program_00"
+#define XRT_MD_NODE_GOLDEN_VER "drv_ep_golden_ver_00"
+#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_BLP "partition_info_0"
+#define XRT_MD_XRT_MD_NODE_PARTITION_INFO_PLP "partition_info_1"
+
+#define XRT_MD_NODE_DDR_SRSR "drv_ep_ddr_srsr"
+#define XRT_MD_REGMAP_DDR_SRSR "drv_ddr_srsr"
+
+#define XRT_MD_PROP_OFFSET "drv_offset"
+#define XRT_MD_PROP_CLK_FREQ "drv_clock_frequency"
+#define XRT_MD_PROP_CLK_CNT "drv_clock_frequency_counter"
+#define XRT_MD_PROP_VBNV "vbnv"
+#define XRT_MD_PROP_VROM "vrom"
+#define XRT_MD_PROP_PARTITION_LEVEL "partition_level"
+
+struct xrt_md_endpoint {
+	const char	*ep_name;
+	u32		bar;
+	long		bar_off;
+	ulong		size;
+	char		*regmap;
+	char		*regmap_ver;
+};
+
+/* Note: res_id is defined by leaf driver and must start with 0. */
+struct xrt_iores_map {
+	char		*res_name;
+	int		res_id;
+};
+
+static inline int xrt_md_res_name2id(const struct xrt_iores_map *res_map,
+				     int entry_num, const char *res_name)
+{
+	int i;
+
+	for (i = 0; i < entry_num; i++) {
+		if (!strcmp(res_name, res_map->res_name))
+			return res_map->res_id;
+		res_map++;
+	}
+	return -1;
+}
+
+static inline const char *
+xrt_md_res_id2name(const struct xrt_iores_map *res_map, int entry_num, int id)
+{
+	int i;
+
+	for (i = 0; i < entry_num; i++) {
+		if (res_map->res_id == id)
+			return res_map->res_name;
+		res_map++;
+	}
+	return NULL;
+}
+
+unsigned long xrt_md_size(struct device *dev, const char *blob);
+int xrt_md_create(struct device *dev, char **blob);
+int xrt_md_add_endpoint(struct device *dev, char *blob,
+			struct xrt_md_endpoint *ep);
+int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,
+			char *regmap_name);
+int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,
+		    const char *regmap_name, const char *prop,
+		    const void **val, int *size);
+int xrt_md_set_prop(struct device *dev, char *blob, const char *ep_name,
+		    const char *regmap_name, const char *prop,
+		    const void *val, int size);
+int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,
+			 const char *ep_name, const char *regmap_name,
+			 const char *new_ep_name);
+int xrt_md_get_next_endpoint(struct device *dev, const char *blob,
+			     const char *ep_name,  const char *regmap_name,
+			     char **next_ep, char **next_regmap);
+int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,
+				   const char *regmap_name, const char **ep_name);
+int xrt_md_find_endpoint(struct device *dev, const char *blob,
+			 const char *ep_name, const char *regmap_name,
+			 const char **epname);
+void xrt_md_pack(struct device *dev, char *blob);
+char *xrt_md_dup(struct device *dev, const char *blob);
+int xrt_md_get_intf_uuids(struct device *dev, const char *blob,
+			  u32 *num_uuids, uuid_t *intf_uuids);
+static inline int xrt_md_copy_all_endpoints(struct device *dev, char *blob, const char *src_blob)
+{
+	return xrt_md_copy_endpoint(dev, blob, src_blob, XRT_MD_NODE_ENDPOINTS,
+				    NULL, NULL);
+}
+
+/*
+ * Firmware provides 128 bit hash string as unque id of partition/interface.
+ * This string will be canonical textual representation in the future.
+ * Before that, introducing these two functions below to translate
+ * hash string to uuid_t for released hardware.
+ */
+static inline void xrt_md_trans_uuid2str(const uuid_t *uuid, char *uuidstr)
+{
+	int i, p;
+	u8 *u = (u8 *)uuid;
+
+	for (p = 0, i = sizeof(uuid_t) - 1; i >= 0; p++, i--)
+		(void)snprintf(&uuidstr[p * 2], 3, "%02x", u[i]);
+}
+
+static inline int xrt_md_trans_str2uuid(struct device *dev, const char *uuidstr, uuid_t *p_uuid)
+{
+	char *p;
+	const char *str;
+	char tmp[3] = { 0 };
+	int i, ret;
+
+	memset(p_uuid, 0, sizeof(*p_uuid));
+	p = (char *)p_uuid;
+	str = uuidstr + strlen(uuidstr) - 2;
+
+	for (i = 0; i < sizeof(*p_uuid) && str >= uuidstr; i++) {
+		tmp[0] = *str;
+		tmp[1] = *(str + 1);
+		ret = kstrtou8(tmp, 16, p);
+		if (ret)
+			return -EINVAL;
+		p++;
+		str -= 2;
+	}
+
+	return 0;
+}
+
+#endif
diff --git a/drivers/fpga/xrt/metadata/metadata.c b/drivers/fpga/xrt/metadata/metadata.c
new file mode 100644
index 000000000000..5d106396f438
--- /dev/null
+++ b/drivers/fpga/xrt/metadata/metadata.c
@@ -0,0 +1,524 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Xilinx Alveo FPGA Metadata parse APIs
+ *
+ * Copyright (C) 2020-2021 Xilinx, Inc.
+ *
+ * Authors:
+ *      Lizhi Hou <Lizhi.Hou@xilinx.com>
+ */
+
+#include <linux/libfdt_env.h>
+#include "libfdt.h"
+#include "metadata.h"
+
+#define MAX_BLOB_SIZE	(4096 * 25)
+
+static int xrt_md_setprop(struct device *dev, char *blob, int offset,
+			  const char *prop, const void *val, int size);
+static int xrt_md_overlay(struct device *dev, char *blob, int target,
+			  const char *overlay_blob, int overlay_offset);
+static int xrt_md_get_endpoint(struct device *dev, const char *blob,
+			       const char *ep_name, const char *regmap_name,
+			       int *ep_offset);
+
+unsigned long xrt_md_size(struct device *dev, const char *blob)
+{
+	unsigned long len = (long)fdt_totalsize(blob);
+
+	len = (len > MAX_BLOB_SIZE) ? XRT_MD_INVALID_LENGTH : len;
+	return len;
+}
+EXPORT_SYMBOL_GPL(xrt_md_size);
+
+int xrt_md_create(struct device *dev, char **blob)
+{
+	int ret = 0;
+
+	WARN_ON(!blob);
+
+	*blob = vmalloc(MAX_BLOB_SIZE);
+	if (!*blob)
+		return -ENOMEM;
+
+	ret = fdt_create_empty_tree(*blob, MAX_BLOB_SIZE);
+	if (ret) {
+		dev_err(dev, "format blob failed, ret = %d", ret);
+		goto failed;
+	}
+
+	ret = fdt_next_node(*blob, -1, NULL);
+	if (ret < 0) {
+		dev_err(dev, "No Node, ret = %d", ret);
+		goto failed;
+	}
+
+	ret = fdt_add_subnode(*blob, ret, XRT_MD_NODE_ENDPOINTS);
+	if (ret < 0) {
+		dev_err(dev, "add node failed, ret = %d", ret);
+		goto failed;
+	}
+
+	return 0;
+
+failed:
+	vfree(*blob);
+	*blob = NULL;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_md_create);
+
+static int xrt_md_add_node(struct device *dev, char *blob, int parent_offset,
+			   const char *ep_name)
+{
+	int ret;
+
+	ret = fdt_add_subnode(blob, parent_offset, ep_name);
+	if (ret < 0 && ret != -FDT_ERR_EXISTS)
+		dev_err(dev, "failed to add node %s. %d", ep_name, ret);
+
+	return ret;
+}
+
+int xrt_md_del_endpoint(struct device *dev, char *blob, const char *ep_name,
+			char *regmap_name)
+{
+	int ret;
+	int ep_offset;
+
+	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name, &ep_offset);
+	if (ret) {
+		dev_err(dev, "can not find ep %s", ep_name);
+		return -EINVAL;
+	}
+
+	ret = fdt_del_node(blob, ep_offset);
+	if (ret)
+		dev_err(dev, "delete node %s failed, ret %d", ep_name, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_md_del_endpoint);
+
+static int __xrt_md_add_endpoint(struct device *dev, char *blob,
+				 struct xrt_md_endpoint *ep, int *offset, bool root)
+{
+	int ret = 0;
+	int ep_offset = 0;
+	u32 val, count = 0;
+	u64 io_range[2];
+	char comp[128];
+
+	if (!ep->ep_name) {
+		dev_err(dev, "empty name");
+		return -EINVAL;
+	}
+
+	if (!root) {
+		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,
+					  &ep_offset);
+		if (ret) {
+			dev_err(dev, "invalid blob, ret = %d", ret);
+			return -EINVAL;
+		}
+	}
+
+	ep_offset = xrt_md_add_node(dev, blob, ep_offset, ep->ep_name);
+	if (ep_offset < 0) {
+		dev_err(dev, "add endpoint failed, ret = %d", ret);
+		return -EINVAL;
+	}
+	if (offset)
+		*offset = ep_offset;
+
+	if (ep->size != 0) {
+		val = cpu_to_be32(ep->bar);
+		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_BAR_IDX,
+				     &val, sizeof(u32));
+		if (ret) {
+			dev_err(dev, "set %s failed, ret %d",
+				XRT_MD_PROP_BAR_IDX, ret);
+			goto failed;
+		}
+		io_range[0] = cpu_to_be64((u64)ep->bar_off);
+		io_range[1] = cpu_to_be64((u64)ep->size);
+		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_IO_OFFSET,
+				     io_range, sizeof(io_range));
+		if (ret) {
+			dev_err(dev, "set %s failed, ret %d",
+				XRT_MD_PROP_IO_OFFSET, ret);
+			goto failed;
+		}
+	}
+
+	if (ep->regmap) {
+		if (ep->regmap_ver) {
+			count = snprintf(comp, sizeof(comp),
+					 "%s-%s", ep->regmap, ep->regmap_ver);
+			count++;
+		}
+
+		count += snprintf(comp + count, sizeof(comp) - count,
+				  "%s", ep->regmap);
+		count++;
+
+		ret = xrt_md_setprop(dev, blob, ep_offset, XRT_MD_PROP_COMPATIBLE,
+				     comp, count);
+		if (ret) {
+			dev_err(dev, "set %s failed, ret %d",
+				XRT_MD_PROP_COMPATIBLE, ret);
+			goto failed;
+		}
+	}
+
+failed:
+	if (ret)
+		xrt_md_del_endpoint(dev, blob, ep->ep_name, NULL);
+
+	return ret;
+}
+
+int xrt_md_add_endpoint(struct device *dev, char *blob,
+			struct xrt_md_endpoint *ep)
+{
+	return __xrt_md_add_endpoint(dev, blob, ep, NULL, false);
+}
+EXPORT_SYMBOL_GPL(xrt_md_add_endpoint);
+
+static int xrt_md_get_endpoint(struct device *dev, const char *blob,
+			       const char *ep_name, const char *regmap_name,
+			       int *ep_offset)
+{
+	int offset;
+	const char *name;
+
+	for (offset = fdt_next_node(blob, -1, NULL);
+	    offset >= 0;
+	    offset = fdt_next_node(blob, offset, NULL)) {
+		name = fdt_get_name(blob, offset, NULL);
+		if (!name || strncmp(name, ep_name, strlen(ep_name) + 1))
+			continue;
+		if (!regmap_name ||
+		    !fdt_node_check_compatible(blob, offset, regmap_name))
+			break;
+	}
+	if (offset < 0)
+		return -ENODEV;
+
+	*ep_offset = offset;
+
+	return 0;
+}
+
+int xrt_md_find_endpoint(struct device *dev, const char *blob,
+			 const char *ep_name, const char *regmap_name,
+			 const char **epname)
+{
+	int offset;
+	int ret;
+
+	ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,
+				  &offset);
+	if (!ret && epname && offset >= 0)
+		*epname = fdt_get_name(blob, offset, NULL);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_md_find_endpoint);
+
+int xrt_md_get_prop(struct device *dev, const char *blob, const char *ep_name,
+		    const char *regmap_name, const char *prop,
+		    const void **val, int *size)
+{
+	int offset;
+	int ret;
+
+	if (val)
+		*val = NULL;
+	if (ep_name) {
+		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,
+					  &offset);
+		if (ret) {
+			dev_err(dev, "cannot get ep %s, regmap %s, ret = %d",
+				ep_name, regmap_name, ret);
+			return -EINVAL;
+		}
+	} else {
+		offset = fdt_next_node(blob, -1, NULL);
+		if (offset < 0) {
+			dev_err(dev, "internal error, ret = %d", offset);
+			return -EINVAL;
+		}
+	}
+
+	if (val) {
+		*val = fdt_getprop(blob, offset, prop, size);
+		if (!*val) {
+			dev_dbg(dev, "get ep %s, prop %s failed", ep_name, prop);
+			return -EINVAL;
+		}
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xrt_md_get_prop);
+
+static int xrt_md_setprop(struct device *dev, char *blob, int offset,
+			  const char *prop, const void *val, int size)
+{
+	int ret;
+
+	ret = fdt_setprop(blob, offset, prop, val, size);
+	if (ret)
+		dev_err(dev, "failed to set prop %d", ret);
+
+	return ret;
+}
+
+int xrt_md_set_prop(struct device *dev, char *blob,
+		    const char *ep_name, const char *regmap_name,
+		    const char *prop, const void *val, int size)
+{
+	int offset;
+	int ret;
+
+	if (ep_name) {
+		ret = xrt_md_get_endpoint(dev, blob, ep_name,
+					  regmap_name, &offset);
+		if (ret) {
+			dev_err(dev, "cannot get node %s, ret = %d",
+				ep_name, ret);
+			return -EINVAL;
+		}
+	} else {
+		offset = fdt_next_node(blob, -1, NULL);
+		if (offset < 0) {
+			dev_err(dev, "internal error, ret = %d", offset);
+			return -EINVAL;
+		}
+	}
+
+	ret = xrt_md_setprop(dev, blob, offset, prop, val, size);
+	if (ret)
+		dev_err(dev, "set prop %s failed, ret = %d", prop, ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_md_set_prop);
+
+int xrt_md_copy_endpoint(struct device *dev, char *blob, const char *src_blob,
+			 const char *ep_name, const char *regmap_name,
+			 const char *new_ep_name)
+{
+	int offset, target;
+	int ret;
+	struct xrt_md_endpoint ep = {0};
+	const char *newepnm = new_ep_name ? new_ep_name : ep_name;
+
+	ret = xrt_md_get_endpoint(dev, src_blob, ep_name, regmap_name,
+				  &offset);
+	if (ret)
+		return -EINVAL;
+
+	ret = xrt_md_get_endpoint(dev, blob, newepnm, regmap_name, &target);
+	if (ret) {
+		ep.ep_name = newepnm;
+		ret = __xrt_md_add_endpoint(dev, blob, &ep, &target,
+					    fdt_parent_offset(src_blob, offset) == 0);
+		if (ret)
+			return -EINVAL;
+	}
+
+	ret = xrt_md_overlay(dev, blob, target, src_blob, offset);
+	if (ret)
+		dev_err(dev, "overlay failed, ret = %d", ret);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(xrt_md_copy_endpoint);
+
+char *xrt_md_dup(struct device *dev, const char *blob)
+{
+	int ret;
+	char *dup_blob;
+
+	ret = xrt_md_create(dev, &dup_blob);
+	if (ret)
+		return NULL;
+	ret = xrt_md_overlay(dev, dup_blob, -1, blob, -1);
+	if (ret) {
+		vfree(dup_blob);
+		return NULL;
+	}
+
+	return dup_blob;
+}
+EXPORT_SYMBOL_GPL(xrt_md_dup);
+
+static int xrt_md_overlay(struct device *dev, char *blob, int target,
+			  const char *overlay_blob, int overlay_offset)
+{
+	int	property, subnode;
+	int	ret;
+
+	WARN_ON(!blob || !overlay_blob);
+
+	if (!blob) {
+		dev_err(dev, "blob is NULL");
+		return -EINVAL;
+	}
+
+	if (target < 0) {
+		target = fdt_next_node(blob, -1, NULL);
+		if (target < 0) {
+			dev_err(dev, "invalid target");
+			return -EINVAL;
+		}
+	}
+	if (overlay_offset < 0) {
+		overlay_offset = fdt_next_node(overlay_blob, -1, NULL);
+		if (overlay_offset < 0) {
+			dev_err(dev, "invalid overlay");
+			return -EINVAL;
+		}
+	}
+
+	fdt_for_each_property_offset(property, overlay_blob, overlay_offset) {
+		const char *name;
+		const void *prop;
+		int prop_len;
+
+		prop = fdt_getprop_by_offset(overlay_blob, property, &name,
+					     &prop_len);
+		if (!prop || prop_len >= MAX_BLOB_SIZE) {
+			dev_err(dev, "internal error");
+			return -EINVAL;
+		}
+
+		ret = xrt_md_setprop(dev, blob, target, name, prop,
+				     prop_len);
+		if (ret) {
+			dev_err(dev, "setprop failed, ret = %d", ret);
+			return ret;
+		}
+	}
+
+	fdt_for_each_subnode(subnode, overlay_blob, overlay_offset) {
+		const char *name = fdt_get_name(overlay_blob, subnode, NULL);
+		int nnode;
+
+		nnode = xrt_md_add_node(dev, blob, target, name);
+		if (nnode == -FDT_ERR_EXISTS)
+			nnode = fdt_subnode_offset(blob, target, name);
+		if (nnode < 0) {
+			dev_err(dev, "add node failed, ret = %d", nnode);
+			return nnode;
+		}
+
+		ret = xrt_md_overlay(dev, blob, nnode, overlay_blob, subnode);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+int xrt_md_get_next_endpoint(struct device *dev, const char *blob,
+			     const char *ep_name, const char *regmap_name,
+			     char **next_ep, char **next_regmap)
+{
+	int offset, ret;
+
+	if (!ep_name) {
+		ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_ENDPOINTS, NULL,
+					  &offset);
+	} else {
+		ret = xrt_md_get_endpoint(dev, blob, ep_name, regmap_name,
+					  &offset);
+	}
+
+	if (ret) {
+		*next_ep = NULL;
+		*next_regmap = NULL;
+		return -EINVAL;
+	}
+
+	offset = ep_name ? fdt_next_subnode(blob, offset) :
+		fdt_first_subnode(blob, offset);
+	if (offset < 0) {
+		*next_ep = NULL;
+		*next_regmap = NULL;
+		return -EINVAL;
+	}
+
+	*next_ep = (char *)fdt_get_name(blob, offset, NULL);
+	*next_regmap = (char *)fdt_stringlist_get(blob, offset, XRT_MD_PROP_COMPATIBLE,
+						  0, NULL);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xrt_md_get_next_endpoint);
+
+int xrt_md_get_compatible_endpoint(struct device *dev, const char *blob,
+				   const char *regmap_name, const char **ep_name)
+{
+	int ep_offset;
+
+	ep_offset = fdt_node_offset_by_compatible(blob, -1, regmap_name);
+	if (ep_offset < 0) {
+		*ep_name = NULL;
+		return -ENOENT;
+	}
+
+	*ep_name = (char *)fdt_get_name(blob, ep_offset, NULL);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xrt_md_get_compatible_endpoint);
+
+void xrt_md_pack(struct device *dev, char *blob)
+{
+	int ret;
+
+	ret = fdt_pack(blob);
+	if (ret)
+		dev_err(dev, "pack failed %d", ret);
+}
+EXPORT_SYMBOL_GPL(xrt_md_pack);
+
+int xrt_md_get_intf_uuids(struct device *dev, const char *blob,
+			  u32 *num_uuids, uuid_t *intf_uuids)
+{
+	int offset, count = 0;
+	int ret;
+	const char *uuid_str;
+
+	ret = xrt_md_get_endpoint(dev, blob, XRT_MD_NODE_INTERFACES, NULL, &offset);
+	if (ret)
+		return -ENOENT;
+
+	for (offset = fdt_first_subnode(blob, offset);
+	    offset >= 0;
+	    offset = fdt_next_subnode(blob, offset)) {
+		uuid_str = fdt_getprop(blob, offset, XRT_MD_PROP_INTERFACE_UUID,
+				       NULL);
+		if (!uuid_str) {
+			dev_err(dev, "empty intf uuid node");
+			return -EINVAL;
+		}
+
+		if (intf_uuids && count < *num_uuids) {
+			ret = xrt_md_trans_str2uuid(dev, uuid_str,
+						    &intf_uuids[count]);
+			if (ret)
+				return -EINVAL;
+		}
+		count++;
+	}
+
+	*num_uuids = count;
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(xrt_md_get_intf_uuids);