Received: by 2002:a05:6a10:8c0a:0:0:0:0 with SMTP id go10csp1079015pxb; Sun, 21 Feb 2021 10:35:51 -0800 (PST) X-Google-Smtp-Source: ABdhPJxHxoEbaZsbwyNe6R493PJcTtXmIy339fuRElu6srs6YcPEXhGeKwlhNhLCoEtAkURraqR5 X-Received: by 2002:a17:907:a059:: with SMTP id gz25mr17794092ejc.400.1613932551705; Sun, 21 Feb 2021 10:35:51 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1613932551; cv=none; d=google.com; s=arc-20160816; b=lLRVHkPiVGWaYdW5zUeuzXllxbcq6uOuEIW2N4/HD4u3CVAy7W5nkdxZax292XX6FX RiA/419fBD+4VCPL0z79q3kPqfFy8Gi9q53TV29cWo3DBX3rsW46dCsFc+IXV51HpxWY UcLm80VcNIXv+5zumWvR/O34N0hgDda9cy9feXzo0QR5pQbG7tD7qquHDYbXejBNTNAp rMonwxQKNgSMcir1JSlG0GhUwYu4tDE5GcZbsXZUOndsTPQvjLt/Fk9zLRSnaYJMY7X/ TP5VnZD+JxGIXI7OIzIYadMDwEzbp6KGCsGd1WrfGus4RmGr2RlyYYPuJAm1+yxdzLnJ Nrtw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:message-id:subject:cc :to:from:date; bh=xma8ZrsFdNV15yNCGaC9MV78Wg2ZeVfymiz2oIUtV9w=; b=WDRSUNzo7spBL10eLF0LtNh5/o200WwddmOGGhEY4nq1VTfSPrDUDDy2qnz3IquRw+ K1RhI3FEH0Ab1Y8UimbHLv+nAAbuSKElNdV5oqT6lfhGkZUw5B9WAQoLC01TSYuBTUcr uBptUVBHse/GeFuKTNWcAGWEm5H9ZnJxL1Vl1yjkTG7Sxa7JOgoBrIZiXrWe/AFk8Eg7 C2m4AIpxjvkgJGzEOZ9modIcehWbHl9R/vSCYMZx5MFbacVwVPTpnB7AppzgU7tTXe1x KGQrRex/WMDVZCJcCVOcLZkkxklz/yal64rYSnMF+QSuHbrDTqecjbJ14bZg/VYF9Zs2 XDBw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id i13si10995139ejo.460.2021.02.21.10.35.27; Sun, 21 Feb 2021 10:35:51 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230138AbhBUSek (ORCPT + 99 others); Sun, 21 Feb 2021 13:34:40 -0500 Received: from mail-pf1-f178.google.com ([209.85.210.178]:33610 "EHLO mail-pf1-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229966AbhBUSeg (ORCPT ); Sun, 21 Feb 2021 13:34:36 -0500 Received: by mail-pf1-f178.google.com with SMTP id z6so5199723pfq.0; Sun, 21 Feb 2021 10:34:19 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:content-transfer-encoding :in-reply-to; bh=xma8ZrsFdNV15yNCGaC9MV78Wg2ZeVfymiz2oIUtV9w=; b=Z3kbny3V+KrMndBlCVUTnWW/RBEvFaxEy/NHVvABpHhhelBqE0tRWs7ih2zSSa1HjH ab1fo5HcyXVGCzAj+9vxJyZzc0fqfO6kGq9ps68qASH7G8Z6rWuk7BezruAwkgdiyyn7 shHNMXcoZ4m0d8vm2KrjOUiOT/5lQrJw6Oh6+35SBclgnVVEoWXPZ3Kzj5fQqm14c+t4 NgECwhPHkQr5U2AL01Mh9M7VZrmRh3nV9fN9TWdGSDRqbe2X0hpcJwYfjwiwh9ztbsS0 J3NQhNEgpJ6CbZHxr9s1yu/yukPlc31vX1zJKZC09Kf8o8N7w42aPCz8ftnErz1RVjnL DKbw== X-Gm-Message-State: AOAM530SA0rT/dtdOWIipaUlRLbgmzvirNUtr3Fl1qiY2v87TVriajUq ZcVVNI8+dEPFzkI805cwNSfuGgPNhEH6AA== X-Received: by 2002:a65:49c1:: with SMTP id t1mr16533203pgs.426.1613932432935; Sun, 21 Feb 2021 10:33:52 -0800 (PST) Received: from localhost ([2601:647:5b00:1161:a4cc:eef9:fbc0:2781]) by smtp.gmail.com with ESMTPSA id v7sm16439385pfv.93.2021.02.21.10.33.51 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sun, 21 Feb 2021 10:33:52 -0800 (PST) Date: Sun, 21 Feb 2021 10:33:51 -0800 From: Moritz Fischer To: Tom Rix Cc: Lizhi Hou , linux-kernel@vger.kernel.org, Lizhi Hou , linux-fpga@vger.kernel.org, maxz@xilinx.com, sonal.santan@xilinx.com, michal.simek@xilinx.com, stefanos@xilinx.com, devicetree@vger.kernel.org, mdf@kernel.org, robh@kernel.org, Max Zhen Subject: Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions Message-ID: References: <20210218064019.29189-1-lizhih@xilinx.com> <20210218064019.29189-4-lizhih@xilinx.com> <4628ef05-27d1-b92f-9126-27a1f810c608@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <4628ef05-27d1-b92f-9126-27a1f810c608@redhat.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Feb 21, 2021 at 09:12:37AM -0800, Tom Rix wrote: > > On 2/17/21 10:40 PM, Lizhi Hou wrote: > > Alveo FPGA firmware and partial reconfigure file are in xclbin format. > This code enumerates and extracts > > Add > > code to enumerate and extract sections from xclbin files. xclbin.h is cross > > platform and used across all platforms and OS > > > > Signed-off-by: Sonal Santan > > Signed-off-by: Max Zhen > > Signed-off-by: Lizhi Hou > > --- > > drivers/fpga/xrt/include/xclbin-helper.h | 52 +++ > > drivers/fpga/xrt/lib/xclbin.c | 394 ++++++++++++++++++++++ > > include/uapi/linux/xrt/xclbin.h | 408 +++++++++++++++++++++++ > > 3 files changed, 854 insertions(+) > > create mode 100644 drivers/fpga/xrt/include/xclbin-helper.h > > create mode 100644 drivers/fpga/xrt/lib/xclbin.c > > create mode 100644 include/uapi/linux/xrt/xclbin.h > > > > diff --git a/drivers/fpga/xrt/include/xclbin-helper.h b/drivers/fpga/xrt/include/xclbin-helper.h > > new file mode 100644 > > index 000000000000..68218efc9d0b > > --- /dev/null > > +++ b/drivers/fpga/xrt/include/xclbin-helper.h > > @@ -0,0 +1,52 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Header file for Xilinx Runtime (XRT) driver > > + * > > + * Copyright (C) 2020-2021 Xilinx, Inc. > > + * > > + * Authors: > > + * David Zhang > > + * Sonal Santan > > + */ > > + > > +#ifndef _XRT_XCLBIN_H > > +#define _XRT_XCLBIN_H > > The header guard should match the filename. > > > + > > +#include > > +#include > > +#include > > + > > +#define ICAP_XCLBIN_V2 "xclbin2" > > +#define DMA_HWICAP_BITFILE_BUFFER_SIZE 1024 > > +#define MAX_XCLBIN_SIZE (1024 * 1024 * 1024) /* Assuming xclbin <= 1G, always */ > #defines should have a prefix, maybe XRT_ or XCLBIN_ > > + > > +enum axlf_section_kind; > > +struct axlf; > > + > > +/** > > + * Bitstream header information as defined by Xilinx tools. > > + * Please note that this struct definition is not owned by the driver. > > + */ > > +struct hw_icap_bit_header { > > File headers usually have fixed length fields like uint32_t > > Is this a structure the real header is converted into ? > > > + unsigned int header_length; /* Length of header in 32 bit words */ > > + unsigned int bitstream_length; /* Length of bitstream to read in bytes*/ > > + unsigned char *design_name; /* Design name get from bitstream */ > > + unsigned char *part_name; /* Part name read from bitstream */ > > + unsigned char *date; /* Date read from bitstream header */ > > + unsigned char *time; /* Bitstream creation time */ > > + unsigned int magic_length; /* Length of the magic numbers */ > > + unsigned char *version; /* Version string */ > > +}; > > + > > +const char *xrt_xclbin_kind_to_string(enum axlf_section_kind kind); > > Only add decl's that are using in multiple files. > > This is only defined in xclbin.c, why does it need to be in the header ? > > > +int xrt_xclbin_get_section(const struct axlf *xclbin, > > + enum axlf_section_kind kind, void **data, > > + uint64_t *len); > > +int xrt_xclbin_get_metadata(struct device *dev, const struct axlf *xclbin, char **dtb); > > +int xrt_xclbin_parse_bitstream_header(const unsigned char *data, > > + unsigned int size, > > + struct hw_icap_bit_header *header); > > +void xrt_xclbin_free_header(struct hw_icap_bit_header *header); > > +const char *xrt_clock_type2epname(enum CLOCK_TYPE type); > CLOCK_TYPE needs a prefix, something like XCLBIN_CLOCK_TYPE > > + > > +#endif /* _XRT_XCLBIN_H */ > > diff --git a/drivers/fpga/xrt/lib/xclbin.c b/drivers/fpga/xrt/lib/xclbin.c > > new file mode 100644 > > index 000000000000..47dc6ca25c1b > > --- /dev/null > > +++ b/drivers/fpga/xrt/lib/xclbin.c > > @@ -0,0 +1,394 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Xilinx Alveo FPGA Driver XCLBIN parser > > + * > > + * Copyright (C) 2020-2021 Xilinx, Inc. > > + * > > + * Authors: David Zhang > > + */ > > + > > +#include > > +#include > > +#include > > +#include "xclbin-helper.h" > > +#include "metadata.h" > > + > What is XHI ?? Maybe expand this, at the lease should comment > > +/* Used for parsing bitstream header */ > > +#define XHI_EVEN_MAGIC_BYTE 0x0f > > +#define XHI_ODD_MAGIC_BYTE 0xf0 > > + > > +/* Extra mode for IDLE */ > > +#define XHI_OP_IDLE -1 > > +#define XHI_BIT_HEADER_FAILURE -1 > > + > > +/* The imaginary module length register */ > > +#define XHI_MLR 15 > > + > > +static inline unsigned char xhi_data_and_inc(const unsigned char *d, int *i, int sz) > could move to the *.h > > +{_ > > + unsigned char data; > > + > > + if (*i >= sz) > > + return -1; > The return value of this funtion is not always checked, at the least add a dev_err here > > + > > + data = d[*i]; > > + (*i)++; > > + > > + return data; > > +} > > + > > +static const struct axlf_section_header * > > +xrt_xclbin_get_section_hdr(const struct axlf *xclbin, > > + enum axlf_section_kind kind) > > +{ > > + int i = 0; > > + > > + for (i = 0; i < xclbin->m_header.m_numSections; i++) { > > + if (xclbin->m_sections[i].m_sectionKind == kind) > > + return &xclbin->m_sections[i]; > > + } > > + > > + return NULL; > > +} > > + > > +static int > > +xrt_xclbin_check_section_hdr(const struct axlf_section_header *header, > > + u64 xclbin_len) > > +{ > > + int ret; > > + > > + ret = (header->m_sectionOffset + header->m_sectionSize) > xclbin_len ? -EINVAL : 0; > > Tristate is harder to read, consider replacing with if() > > int ret = 0 > > if () > > ? ret = Why not just: if (header->m_section_offset + header->m_section_size) return -EINVAL; return 0; Also please fix the camelCase throughout the entire patchset. > > > > + > > + return ret; > > +} > > + > > +static int xrt_xclbin_section_info(const struct axlf *xclbin, > > + enum axlf_section_kind kind, > > + u64 *offset, u64 *size) > > +{ > > + const struct axlf_section_header *mem_header = NULL; > > + u64 xclbin_len; > > + int err = 0; > > + > > + mem_header = xrt_xclbin_get_section_hdr(xclbin, kind); > > + if (!mem_header) > > + return -EINVAL; > > + > > + xclbin_len = xclbin->m_header.m_length; > > + if (xclbin_len > MAX_XCLBIN_SIZE) > > + return -EINVAL; > > This check can be added to the function call.. > > or the sanity checking added to the earier call to *get_section_hdr > > There a number of small functions that can be combined. > > > + > > + err = xrt_xclbin_check_section_hdr(mem_header, xclbin_len); > > + if (err) > > + return err; > > + > > + *offset = mem_header->m_sectionOffset; > > + *size = mem_header->m_sectionSize; > > + > > + return 0; > > +} > > + > > +/* caller should free the allocated memory for **data */ > > must free > > This comment also needs to be with the *.h decl > > > +int xrt_xclbin_get_section(const struct axlf *buf, > > + enum axlf_section_kind kind, > > + void **data, u64 *len) > > +{ > > + const struct axlf *xclbin = (const struct axlf *)buf; > > + void *section = NULL; > > + int err = 0; > > + u64 offset = 0; > > + u64 size = 0; > > + > > + err = xrt_xclbin_section_info(xclbin, kind, &offset, &size); > > + if (err) > > + return err; > > + > > + section = vmalloc(size); > > + if (!section) > > + return -ENOMEM; > > + > > + memcpy(section, ((const char *)xclbin) + offset, size); > > + > > + *data = section; > > a general comment > > for exported function checking the validity of the inputs in more important. > > here you assume **data is valid, really you should check. > > > + if (len) > > + *len = size; > len setting being optional, needs to be in the *.h comment > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(xrt_xclbin_get_section); > > + > Instead of allocating new memory and making copies of bits of *data > > why not have the points reference data ? > > The size operations look like translating big endian data to little endian. > > This will break on a big endian host. > > > +/* parse bitstream header */ > > +int xrt_xclbin_parse_bitstream_header(const unsigned char *data, > > + unsigned int size, > > + struct hw_icap_bit_header *header) > > +{ > > + unsigned int index; > > + unsigned int len; > > + unsigned int tmp; > > + unsigned int i; > > + > > + memset(header, 0, sizeof(*header)); > > + /* Start Index at start of bitstream */ > > + index = 0; > > + > > + /* Initialize HeaderLength. If header returned early inidicates > > + * failure. > > This side effect should be documented in the *.h comment. > > Also the multi line comment is a bit weird, not sure if it is ok > > > + */ > > + header->header_length = XHI_BIT_HEADER_FAILURE; > > + > > + /* Get "Magic" length */ > > + header->magic_length = xhi_data_and_inc(data, &index, size); > > + header->magic_length = (header->magic_length << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* Read in "magic" */ > > + for (i = 0; i < header->magic_length - 1; i++) { > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (i % 2 == 0 && tmp != XHI_EVEN_MAGIC_BYTE) if !(i % 2) ... > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + if (i % 2 == 1 && tmp != XHI_ODD_MAGIC_BYTE) > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + } > > + > > + /* Read null end of magic data. */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + > > + /* Read 0x01 (short) */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + tmp = (tmp << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* Check the "0x01" half word */ > > + if (tmp != 0x01) > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Read 'a' */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (tmp != 'a') > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Get Design Name length */ > > + len = xhi_data_and_inc(data, &index, size); > > + len = (len << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* allocate space for design name and final null character. */ > > + header->design_name = vmalloc(len); > > + if (!header->design_name) > > + return -ENOMEM; > > + > > + /* Read in Design Name */ > > + for (i = 0; i < len; i++) > > + header->design_name[i] = xhi_data_and_inc(data, &index, size); > > + > > + if (header->design_name[len - 1] != '\0') > > + return -1; > > + > > + header->version = strstr(header->design_name, "Version=") + strlen("Version="); > > + > > + /* Read 'b' */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (tmp != 'b') > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Get Part Name length */ > > + len = xhi_data_and_inc(data, &index, size); > > + len = (len << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* allocate space for part name and final null character. */ > > + header->part_name = vmalloc(len); > > + if (!header->part_name) > > + return -ENOMEM; > > + > > + /* Read in part name */ > > + for (i = 0; i < len; i++) > > + header->part_name[i] = xhi_data_and_inc(data, &index, size); > > + > > + if (header->part_name[len - 1] != '\0') > > + return -1; > > + > > + /* Read 'c' */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (tmp != 'c') > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Get date length */ > > + len = xhi_data_and_inc(data, &index, size); > > + len = (len << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* allocate space for date and final null character. */ > > + header->date = vmalloc(len); > > + if (!header->date) > > + return -ENOMEM; > > + > > + /* Read in date name */ > > + for (i = 0; i < len; i++) > > + header->date[i] = xhi_data_and_inc(data, &index, size); > > + > > + if (header->date[len - 1] != '\0') > > + return -1; > generally -EINVAL is more meaningful than -1 > > + > > + /* Read 'd' */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (tmp != 'd') > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Get time length */ > > + len = xhi_data_and_inc(data, &index, size); > > + len = (len << 8) | xhi_data_and_inc(data, &index, size); > > + > > + /* allocate space for time and final null character. */ > > + header->time = vmalloc(len); > > + if (!header->time) > > + return -ENOMEM; > > + > > + /* Read in time name */ > > + for (i = 0; i < len; i++) > > + header->time[i] = xhi_data_and_inc(data, &index, size); > > + > > + if (header->time[len - 1] != '\0') > > + return -1; > > + > > + /* Read 'e' */ > > + tmp = xhi_data_and_inc(data, &index, size); > > + if (tmp != 'e') > > + return -1; /* INVALID_FILE_HEADER_ERROR */ > > + > > + /* Get byte length of bitstream */ > > + header->bitstream_length = xhi_data_and_inc(data, &index, size); > > + header->bitstream_length = (header->bitstream_length << 8) | > > + xhi_data_and_inc(data, &index, size); > > + header->bitstream_length = (header->bitstream_length << 8) | > > + xhi_data_and_inc(data, &index, size); > > + header->bitstream_length = (header->bitstream_length << 8) | > > + xhi_data_and_inc(data, &index, size); > > generally a problem > > This is confusing, collect the bytes in a temp[] and construct the header->bitstream_length in on statement. > > This is a case where xhi_data_and_inc return is not checked and if it failed could blow up later. > > > + > > + header->header_length = index; > > index is not a good variable name if it going to be stored as a length. > > consider changing it to something like current_length. > > > + > > + return 0; > > +} > > +EXPORT_SYMBOL_GPL(xrt_xclbin_parse_bitstream_header); > > + > > +void xrt_xclbin_free_header(struct hw_icap_bit_header *header) > > +{ > > + vfree(header->design_name); > > + vfree(header->part_name); > > + vfree(header->date); > > + vfree(header->time); > missing header->version > > +} > > +EXPORT_SYMBOL_GPL(xrt_xclbin_free_header); > > + > > +struct xrt_clock_desc { > > + char *clock_ep_name; > > + u32 clock_xclbin_type; > > + char *clkfreq_ep_name; > > +} clock_desc[] = { > > + { > > + .clock_ep_name = XRT_MD_NODE_CLK_KERNEL1, > > + .clock_xclbin_type = CT_DATA, > > + .clkfreq_ep_name = XRT_MD_NODE_CLKFREQ_K1, > > + }, > > + { > > + .clock_ep_name = XRT_MD_NODE_CLK_KERNEL2, > > + .clock_xclbin_type = CT_KERNEL, > > + .clkfreq_ep_name = XRT_MD_NODE_CLKFREQ_K2, > > + }, > > + { > > + .clock_ep_name = XRT_MD_NODE_CLK_KERNEL3, > > + .clock_xclbin_type = CT_SYSTEM, > > + .clkfreq_ep_name = XRT_MD_NODE_CLKFREQ_HBM, > > + }, > > +}; > > + > > +const char *xrt_clock_type2epname(enum CLOCK_TYPE type) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(clock_desc); i++) { > > + if (clock_desc[i].clock_xclbin_type == type) > > + return clock_desc[i].clock_ep_name; > > + } > > + return NULL; > > +} > > +EXPORT_SYMBOL_GPL(xrt_clock_type2epname); > > What is clock stuff doing in xclbin ? > > I think clock needs its own file > > > + > > +static const char *clock_type2clkfreq_name(u32 type) > > +{ > > + int i; > > + > > + for (i = 0; i < ARRAY_SIZE(clock_desc); i++) { > > + if (clock_desc[i].clock_xclbin_type == type) > > + return clock_desc[i].clkfreq_ep_name; > > + } > > + return NULL; > > +} > > + > > +static int xrt_xclbin_add_clock_metadata(struct device *dev, > > + const struct axlf *xclbin, > > + char *dtb) > > +{ > > + int i; > > + u16 freq; > > + struct clock_freq_topology *clock_topo; > > + int rc = xrt_xclbin_get_section(xclbin, CLOCK_FREQ_TOPOLOGY, > > + (void **)&clock_topo, NULL); > > + > > + if (rc) > > + return 0; > failing is ok ? > > + > > + for (i = 0; i < clock_topo->m_count; i++) { > > + u8 type = clock_topo->m_clock_freq[i].m_type; > > + const char *ep_name = xrt_clock_type2epname(type); > > + const char *counter_name = clock_type2clkfreq_name(type); > > + > > + if (!ep_name || !counter_name) > > + continue; > > + > > + freq = cpu_to_be16(clock_topo->m_clock_freq[i].m_freq_Mhz); > > + rc = xrt_md_set_prop(dev, dtb, ep_name, NULL, XRT_MD_PROP_CLK_FREQ, > > + &freq, sizeof(freq)); > > + if (rc) > > + break; > > + > > + rc = xrt_md_set_prop(dev, dtb, ep_name, NULL, XRT_MD_PROP_CLK_CNT, > > + counter_name, strlen(counter_name) + 1); > > + if (rc) > > + break; > Failing in a loop, why isn't there some cleanup of the *set_prop() calls ? > > + } > > + > > + vfree(clock_topo); > > + > > + return rc; > > +} > > + > > +int xrt_xclbin_get_metadata(struct device *dev, const struct axlf *xclbin, char **dtb) > > +{ > > + char *md = NULL, *newmd = NULL; > > + u64 len; > *dtb = NULL; > > + int rc = xrt_xclbin_get_section(xclbin, PARTITION_METADATA, > > + (void **)&md, &len); > > + > > + if (rc) > > + goto done; > > + > > + /* Sanity check the dtb section. */ > > + if (xrt_md_size(dev, md) > len) { > > + rc = -EINVAL; > > + goto done; > > + } > > + > > + newmd = xrt_md_dup(dev, md); > > + if (!newmd) { > > + rc = -EFAULT; > > + goto done; > > + } > > + /* Convert various needed xclbin sections into dtb. */ > > + rc = xrt_xclbin_add_clock_metadata(dev, xclbin, newmd); > > newmd is only valid here, but the above error handling jump here. change this to > > if (!rc) > > ? *dtb = newmd > > else > > ?? vfree(newmd) > > done: > > ? vfree(md) > > ? return rc; > > > + > > +done: > > + if (rc == 0) > > + *dtb = newmd; > > + else > > + vfree(newmd); > > + vfree(md); > > + return rc; > > +} > > +EXPORT_SYMBOL_GPL(xrt_xclbin_get_metadata); > > diff --git a/include/uapi/linux/xrt/xclbin.h b/include/uapi/linux/xrt/xclbin.h > > new file mode 100644 > > index 000000000000..53f140123ef1 > > --- /dev/null > > +++ b/include/uapi/linux/xrt/xclbin.h > > @@ -0,0 +1,408 @@ > > +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */ > > +/* > > + * Xilinx FPGA compiled binary container format > > + * > > + * Copyright (C) 2015-2021, Xilinx Inc > > + */ > > + > > +#ifndef _XCLBIN_H_ > > +#define _XCLBIN_H_ > > + > > +#ifdef _WIN32 > > WIN32 ? > > Only 1 other header has this ifdef > > > + #include > > + #include > c++ is being assumed for windows > > + #include "windows/uuid.h" > thank you for not including windows.h ;) > > +#else > > + #if defined(__KERNEL__) > > + #include > > + #include > > + #include > > + #elif defined(__cplusplus) > > + #include > > + #include > > + #include > > + #include > > + #else > > + #include > > + #include > > + #include > > + #endif > > +#endif Tbh, not a big fan of this ... > > + > > Review these includes, some could be convenience includes. > > ex/ linux/version.h with no obvious use of version macros. > > struct axlf_header { > + uint64_t m_length; /* Total size of the xclbin file */ > .. snip .. > + union { > + char m_next_axlf[16]; /* Name of next xclbin file */ > + /* in the daisy chain */ > + uuid_t uuid; /* uuid of this xclbin*/ > + }; > > As mentioned in an earlier patch, if uuid_t is larger than 16 bytes, axlf_header breaks. > while it is convenient to have this type here, it would be better this access was handled in another way. > Maybe a host specific function. > > I also do not see a pragma pack, usually this is set of 1 so the compiler does not shuffle elements, increase size etc. > > > +#ifdef __cplusplus > > +extern "C" { > > +#endif > > + > > +/** > > + * DOC: Container format for Xilinx FPGA images > > + * The container stores bitstreams, metadata and firmware images. > > + * xclbin/xsabin is ELF-like binary container format. It is structured > is an ELF-like file format.? It is a structured > > + * series of sections. There is a file header followed by several section > > + * headers which is followed by sections. A section header points to an > > + * actual section. There is an optional signature at the end. The > > + * following figure illustrates a typical xclbin: > > + * > > + * +---------------------+ > > + * | | > > + * | HEADER | > > + * +---------------------+ > > + * | SECTION HEADER | > > + * | | > > + * +---------------------+ > > + * | ... | > > + * | | > > + * +---------------------+ > > + * | SECTION HEADER | > > + * | | > > + * +---------------------+ > > + * | SECTION | > > + * | | > > + * +---------------------+ > > + * | ... | > > + * | | > > + * +---------------------+ > > + * | SECTION | > > + * | | > > + * +---------------------+ > > + * | SIGNATURE | > > + * | (OPTIONAL) | > > + * +---------------------+ > > This ascii art is a mixture of tabs and spaces, for someone with tab = 2 spaces, this will look messed up. > > convert the tabs to spaces > > > + */ > > + > > +enum XCLBIN_MODE { > > + XCLBIN_FLAT, > > generally > > all enums used in a file format should be initialized. > > This likely should be > > XCLBIN_FLAT = 0, > > > + XCLBIN_PR, > > + XCLBIN_TANDEM_STAGE2, > > + XCLBIN_TANDEM_STAGE2_WITH_PR, > > + XCLBIN_HW_EMU, > > + XCLBIN_SW_EMU, > > + XCLBIN_MODE_MAX > > +}; > > + > > +enum axlf_section_kind { > > + BITSTREAM = 0, > > + CLEARING_BITSTREAM, > > + EMBEDDED_METADATA, > > + FIRMWARE, > > + DEBUG_DATA, > > + SCHED_FIRMWARE, > > + MEM_TOPOLOGY, > > + CONNECTIVITY, > > + IP_LAYOUT, > > + DEBUG_IP_LAYOUT, > > + DESIGN_CHECK_POINT, > > + CLOCK_FREQ_TOPOLOGY, > > + MCS, > > + BMC, > > + BUILD_METADATA, > > + KEYVALUE_METADATA, > > + USER_METADATA, > > + DNA_CERTIFICATE, > > + PDI, > > + BITSTREAM_PARTIAL_PDI, > > + PARTITION_METADATA, > > + EMULATION_DATA, > > + SYSTEM_METADATA, > > + SOFT_KERNEL, > > + ASK_FLASH, > > + AIE_METADATA, > > + ASK_GROUP_TOPOLOGY, > > + ASK_GROUP_CONNECTIVITY > > +}; > > + > > +enum MEM_TYPE { > > + MEM_DDR3, > > + MEM_DDR4, > > + MEM_DRAM, > > + MEM_STREAMING, > > + MEM_PREALLOCATED_GLOB, > > + MEM_ARE, > > + MEM_HBM, > > + MEM_BRAM, > > + MEM_URAM, > > + MEM_STREAMING_CONNECTION > > +}; > > + > > +enum IP_TYPE { > > + IP_MB = 0, > > + IP_KERNEL, > > + IP_DNASC, > > + IP_DDR4_CONTROLLER, > > + IP_MEM_DDR4, > > + IP_MEM_HBM > > +}; > > + > > +struct axlf_section_header { > > + uint32_t m_sectionKind; /* Section type */ > > + char m_sectionName[16]; /* Examples: "stage2", "clear1", */ > > + /* "clear2", "ocl1", "ocl2, */ > > + /* "ublaze", "sched" */ > > + uint64_t m_sectionOffset; /* File offset of section data */ > > + uint64_t m_sectionSize; /* Size of section data */ > > +}; > > + > > +struct axlf_header { > > + uint64_t m_length; /* Total size of the xclbin file */ > > + uint64_t m_timeStamp; /* Number of seconds since epoch */ > > + /* when xclbin was created */ > > + uint64_t m_featureRomTimeStamp; /* TimeSinceEpoch of the featureRom */ > > + uint16_t m_versionPatch; /* Patch Version */ > > + uint8_t m_versionMajor; /* Major Version - Version: 2.1.0*/ > i did not see the version checked earlier, which one is expected ? > > + uint8_t m_versionMinor; /* Minor Version */ > > + uint32_t m_mode; /* XCLBIN_MODE */ > > + union { > > + struct { > > + uint64_t m_platformId; /* 64 bit platform ID: */ > > + /* vendor-device-subvendor-subdev */ > > + uint64_t m_featureId; /* 64 bit feature id */ > > + } rom; > > + unsigned char rom_uuid[16]; /* feature ROM UUID for which */ > > + /* this xclbin was generated */ > > + }; > > + unsigned char m_platformVBNV[64]; /* e.g. */ > what is VBNV? > > + /* xilinx:xil-accel-rd-ku115:4ddr-xpr:3.4: null terminated */ > > + union { > > + char m_next_axlf[16]; /* Name of next xclbin file */ > > + /* in the daisy chain */ > > + uuid_t uuid; /* uuid of this xclbin*/ > > + }; > > + char m_debug_bin[16]; /* Name of binary with debug */ > > + /* information */ > > + uint32_t m_numSections; /* Number of section headers */ > > +}; > > + > > +struct axlf { > > + char m_magic[8]; /* Should be "xclbin2\0" */ > > + int32_t m_signature_length; /* Length of the signature. */ > > + /* -1 indicates no signature */ > > + unsigned char reserved[28]; /* Note: Initialized to 0xFFs */ > > + > > + unsigned char m_keyBlock[256]; /* Signature for validation */ > > + /* of binary */ > > + uint64_t m_uniqueId; /* axlf's uniqueId, use it to */ > > + /* skip redownload etc */ > > + struct axlf_header m_header; /* Inline header */ > > + struct axlf_section_header m_sections[1]; /* One or more section */ > > + /* headers follow */ > > +}; > > + > > +/* bitstream information */ > > +struct xlnx_bitstream { > > + uint8_t m_freq[8]; > > + char bits[1]; > > +}; > > + > > +/**** MEMORY TOPOLOGY SECTION ****/ > > +struct mem_data { > > + uint8_t m_type; /* enum corresponding to mem_type. */ > > + uint8_t m_used; /* if 0 this bank is not present */ > > + union { > > + uint64_t m_size; /* if mem_type DDR, then size in KB; */ > > + uint64_t route_id; /* if streaming then "route_id" */ > > + }; > > + union { > > + uint64_t m_base_address;/* if DDR then the base address; */ > > + uint64_t flow_id; /* if streaming then "flow id" */ > > + }; > > + unsigned char m_tag[16]; /* DDR: BANK0,1,2,3, has to be null */ > > + /* terminated; if streaming then stream0, 1 etc */ > > +}; > > + > > +struct mem_topology { > > + int32_t m_count; /* Number of mem_data */ > > + struct mem_data m_mem_data[1]; /* Should be sorted on mem_type */ > > +}; > > + > > +/**** CONNECTIVITY SECTION ****/ > > +/* Connectivity of each argument of Kernel. It will be in terms of argument > > This section does not make sense. > > Likely you mean some algorithm kernel, rather than the linux kernel. > > > + * index associated. For associating kernel instances with arguments and > > + * banks, start at the connectivity section. Using the m_ip_layout_index > > + * access the ip_data.m_name. Now we can associate this kernel instance > > + * with its original kernel name and get the connectivity as well. This > > + * enables us to form related groups of kernel instances. > > + */ > > + > > +struct connection { > > + int32_t arg_index; /* From 0 to n, may not be contiguous as scalars */ > > + /* skipped */ > > + int32_t m_ip_layout_index; /* index into the ip_layout section. */ > > + /* ip_layout.m_ip_data[index].m_type == IP_KERNEL */ > > + int32_t mem_data_index; /* index of the m_mem_data . Flag error is */ > > + /* m_used false. */ > > +}; > > + > > +struct connectivity { > > + int32_t m_count; > > + struct connection m_connection[1]; > > +}; > > + > > +/**** IP_LAYOUT SECTION ****/ > > + > > +/* IP Kernel */ > > +#define IP_INT_ENABLE_MASK 0x0001 > > +#define IP_INTERRUPT_ID_MASK 0x00FE > > +#define IP_INTERRUPT_ID_SHIFT 0x1 > > + > > +enum IP_CONTROL { > > + AP_CTRL_HS = 0, > > + AP_CTRL_CHAIN = 1, > > + AP_CTRL_NONE = 2, > > + AP_CTRL_ME = 3, > > + ACCEL_ADAPTER = 4 > assigning beyond the first is not necessary unless there are dups or gaps > > +}; > > + > > +#define IP_CONTROL_MASK 0xFF00 > > +#define IP_CONTROL_SHIFT 0x8 > > + > > +/* IPs on AXI lite - their types, names, and base addresses.*/ > > +struct ip_data { > > + uint32_t m_type; /* map to IP_TYPE enum */ > > + union { > > + uint32_t properties; /* Default: 32-bits to indicate ip */ > > + /* specific property. */ > > + /* m_type: IP_KERNEL > > + * m_int_enable : Bit - 0x0000_0001; > > + * m_interrupt_id : Bits - 0x0000_00FE; > > + * m_ip_control : Bits = 0x0000_FF00; > > + */ > > + struct { /* m_type: IP_MEM_* */ > > + uint16_t m_index; > > + uint8_t m_pc_index; > > + uint8_t unused; > > + } indices; > > + }; > > + uint64_t m_base_address; > > + uint8_t m_name[64]; /* eg Kernel name corresponding to KERNEL */ > > + /* instance, can embed CU name in future. */ > > +}; > > + > > +struct ip_layout { > > + int32_t m_count; > > + struct ip_data m_ip_data[1]; /* All the ip_data needs to be sorted */ > > + /* by m_base_address. */ > > general > > doing the bla[1] for c++ ? > > Tom > > > +}; > > + > > +/*** Debug IP section layout ****/ > > +enum DEBUG_IP_TYPE { > > + UNDEFINED = 0, > > + LAPC, > > + ILA, > > + AXI_MM_MONITOR, > > + AXI_TRACE_FUNNEL, > > + AXI_MONITOR_FIFO_LITE, > > + AXI_MONITOR_FIFO_FULL, > > + ACCEL_MONITOR, > > + AXI_STREAM_MONITOR, > > + AXI_STREAM_PROTOCOL_CHECKER, > > + TRACE_S2MM, > > + AXI_DMA, > > + TRACE_S2MM_FULL > > +}; > > + > > +struct debug_ip_data { > > + uint8_t m_type; /* type of enum DEBUG_IP_TYPE */ > > + uint8_t m_index_lowbyte; > > + uint8_t m_properties; > > + uint8_t m_major; > > + uint8_t m_minor; > > + uint8_t m_index_highbyte; > > + uint8_t m_reserved[2]; > > + uint64_t m_base_address; > > + char m_name[128]; > > +}; > > + > > +struct debug_ip_layout { > > + uint16_t m_count; > > + struct debug_ip_data m_debug_ip_data[1]; > > +}; > > + > > +/* Supported clock frequency types */ > > +enum CLOCK_TYPE { > > + CT_UNUSED = 0, /* Initialized value */ > > + CT_DATA = 1, /* Data clock */ > > + CT_KERNEL = 2, /* Kernel clock */ > > + CT_SYSTEM = 3 /* System Clock */ > > +}; > > + > > +/* Clock Frequency Entry */ > > +struct clock_freq { > > + uint16_t m_freq_Mhz; /* Frequency in MHz */ > > + uint8_t m_type; /* Clock type (enum CLOCK_TYPE) */ > > + uint8_t m_unused[5]; /* Not used - padding */ > > + char m_name[128]; /* Clock Name */ > > +}; > > + > > +/* Clock frequency section */ > > +struct clock_freq_topology { > > + int16_t m_count; /* Number of entries */ > > + struct clock_freq m_clock_freq[1]; /* Clock array */ > > +}; > > + > > +/* Supported MCS file types */ > > +enum MCS_TYPE { > > + MCS_UNKNOWN = 0, /* Initialized value */ > > + MCS_PRIMARY = 1, /* The primary mcs file data */ > > + MCS_SECONDARY = 2, /* The secondary mcs file data */ > > +}; > > + > > +/* One chunk of MCS data */ > > +struct mcs_chunk { > > + uint8_t m_type; /* MCS data type */ just call them type, unused, offset. Drop the m_* > > + uint8_t m_unused[7]; /* padding */ > > + uint64_t m_offset; /* data offset from the start of */ > > + /* the section */ > > + uint64_t m_size; /* data size */ > > +}; > > + > > +/* MCS data section */ > > +struct mcs { > > + int8_t m_count; /* Number of chunks */ > > + int8_t m_unused[7]; /* padding */ > > + struct mcs_chunk m_chunk[1]; /* MCS chunks followed by data */ > > +}; > > + > > +/* bmc data section */ > > +struct bmc { > > + uint64_t m_offset; /* data offset from the start of */ > > + /* the section */ > > + uint64_t m_size; /* data size (bytes) */ > > + char m_image_name[64]; /* Name of the image */ > > + /* (e.g., MSP432P401R) */ > > + char m_device_name[64]; /* Device ID (e.g., VCU1525) */ > > + char m_version[64]; > > + char m_md5value[33]; /* MD5 Expected Value */ > > + /* (e.g., 56027182079c0bd621761b7dab5a27ca)*/ > > + char m_padding[7]; /* Padding */ > > +}; > > + > > +/* soft kernel data section, used by classic driver */ > > +struct soft_kernel { > > + /** Prefix Syntax: > > + * mpo - member, pointer, offset > > + * This variable represents a zero terminated string > > + * that is offseted from the beginning of the section. > > + * The pointer to access the string is initialized as follows: > > + * char * pCharString = (address_of_section) + (mpo value) > > + */ > > + uint32_t mpo_name; /* Name of the soft kernel */ > > + uint32_t m_image_offset; /* Image offset */ > > + uint32_t m_image_size; /* Image size */ > > + uint32_t mpo_version; /* Version */ > > + uint32_t mpo_md5_value; /* MD5 checksum */ > > + uint32_t mpo_symbol_name; /* Symbol name */ > > + uint32_t m_num_instances; /* Number of instances */ > > + uint8_t padding[36]; /* Reserved for future use */ > > + uint8_t reservedExt[16]; /* Reserved for future extended data */ > > +}; > > + > > +enum CHECKSUM_TYPE { > > + CST_UNKNOWN = 0, > > + CST_SDBM = 1, > > + CST_LAST > > +}; > > + > > +#ifdef __cplusplus > > +} > > +#endif > > + > > +#endif > I'll take a closer look, these were just random things I bumped into. - Moritz