Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-03-05 Thread Lizhi Hou

Hi Moritz,


On 02/21/2021 10:33 AM, Moritz Fischer wrote:

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 ..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 ..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 >m_sections[i];
+   }
+
+   return NULL;
+}
+
+static int
+xrt_xclbin_check_section_hdr(const struct axlf_section_header *header,
+  

Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-03-04 Thread Lizhi Hou

Hi Moritz,


On 03/02/2021 07:14 AM, Moritz Fischer wrote:


On Mon, Mar 01, 2021 at 04:25:37PM -0800, Lizhi Hou wrote:

Hi Tom,


On 02/28/2021 08:54 AM, Tom Rix wrote:

CAUTION: This message has originated from an External Source. Please use proper 
judgment and caution when opening attachments, clicking links, or responding to 
this email.


On 2/26/21 1:23 PM, Lizhi Hou wrote:

Hi Tom,



snip


I also do not see a pragma pack, usually this is set of 1 so the compiler does 
not shuffle elements, increase size etc.

This data structure is shared with other tools. And the structure is well 
defined with reasonable alignment. It is compatible with all compilers we have 
tested. So pragma pack is not necessary.

You can not have possibly tested all the configurations since the kernel 
supports many arches and compilers.

If the tested existing alignment is ok, pragma pack should be a noop on your 
tested configurations.

And help cover the untested configurations.

Got it. I will add pragma pack(1).

Please do not use pragma pack(), add __packed to the structs in
question.

Ok, I will use __packed.

Thanks,
Lizhi


- Moritz




Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-03-02 Thread Moritz Fischer
On Mon, Mar 01, 2021 at 04:25:37PM -0800, Lizhi Hou wrote:
> Hi Tom,
> 
> 
> On 02/28/2021 08:54 AM, Tom Rix wrote:
> > CAUTION: This message has originated from an External Source. Please use 
> > proper judgment and caution when opening attachments, clicking links, or 
> > responding to this email.
> > 
> > 
> > On 2/26/21 1:23 PM, Lizhi Hou wrote:
> > > Hi Tom,
> > > 
> > > 
> > snip
> > 
> > > > I also do not see a pragma pack, usually this is set of 1 so the 
> > > > compiler does not shuffle elements, increase size etc.
> > > This data structure is shared with other tools. And the structure is well 
> > > defined with reasonable alignment. It is compatible with all compilers we 
> > > have tested. So pragma pack is not necessary.
> > You can not have possibly tested all the configurations since the kernel 
> > supports many arches and compilers.
> > 
> > If the tested existing alignment is ok, pragma pack should be a noop on 
> > your tested configurations.
> > 
> > And help cover the untested configurations.
> Got it. I will add pragma pack(1).

Please do not use pragma pack(), add __packed to the structs in
question.

- Moritz


Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-03-01 Thread Lizhi Hou

Hi Tom,


On 02/28/2021 08:54 AM, Tom Rix wrote:

CAUTION: This message has originated from an External Source. Please use proper 
judgment and caution when opening attachments, clicking links, or responding to 
this email.


On 2/26/21 1:23 PM, Lizhi Hou wrote:

Hi Tom,



snip


I also do not see a pragma pack, usually this is set of 1 so the compiler does 
not shuffle elements, increase size etc.

This data structure is shared with other tools. And the structure is well 
defined with reasonable alignment. It is compatible with all compilers we have 
tested. So pragma pack is not necessary.

You can not have possibly tested all the configurations since the kernel 
supports many arches and compilers.

If the tested existing alignment is ok, pragma pack should be a noop on your 
tested configurations.

And help cover the untested configurations.

Got it. I will add pragma pack(1).

Lizhi


Tom





Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-02-28 Thread Tom Rix


On 2/26/21 1:23 PM, Lizhi Hou wrote:
> Hi Tom,
>
>
snip

>>
>> I also do not see a pragma pack, usually this is set of 1 so the compiler 
>> does not shuffle elements, increase size etc.
> This data structure is shared with other tools. And the structure is well 
> defined with reasonable alignment. It is compatible with all compilers we 
> have tested. So pragma pack is not necessary.

You can not have possibly tested all the configurations since the kernel 
supports many arches and compilers.

If the tested existing alignment is ok, pragma pack should be a noop on your 
tested configurations.

And help cover the untested configurations.

Tom



Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-02-26 Thread Lizhi Hou

Hi Tom,


On 02/21/2021 09:12 AM, 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

Will change this to

Alveo FPGA firmware and partial reconfigure file are in xclbin format. This
code enumerates and extracts sections from xclbin files. xclbin.h is cross
platform and used across all platforms and OS.

  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 ..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.

will fix this.



+
+#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_

Will add prefix 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 ?
This is not real header. This structure saves the information extracted 
from bitstream header.



+ 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 ?

Will remove this.



+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

Will change to 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 ..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

Will use BITSTREAM_ instead.

+/* 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
I will restructure caller function xrt_xclbin_parse_bitstream_header() 
and remove xhi_data_and_inc().

+{_
+ 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 

Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-02-21 Thread Moritz Fischer
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 ..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 ..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;
> > 

Re: [PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-02-21 Thread Tom Rix


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 ..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 ..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 

[PATCH V3 XRT Alveo 03/18] fpga: xrt: xclbin file helper functions

2021-02-17 Thread Lizhi Hou
Alveo FPGA firmware and partial reconfigure file are in xclbin format. 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 ..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
+
+#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 
*/
+
+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 {
+   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);
+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);
+
+#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 ..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"
+
+/* 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)
+{
+   unsigned char data;
+
+   if (*i >= sz)
+   return -1;
+
+   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 >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;
+
+   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)
+