Integrate FPGA-accelerated PoW #50

ajblane · 2018-07-27T06:46:58Z

No description provided.

jserv · 2018-08-02T18:12:21Z

src/converter.c

@@ -88,7 +88,7 @@ void getTrits(const char* bytes, int bytelength, char* const trits,
  }
 }

-char* trits_from_trytes(const char* trytes, int length) {
+char* trits_from_trytes_aj(const char* trytes, int length) {


Don't append _aj suffix.

jserv · 2018-08-02T18:13:08Z

mk/defs.mk

@@ -12,3 +12,6 @@ BUILD_JNI ?= 0

 # Build cCurl compatible interface
 BUILD_COMPAT ?= 0
+
+# Build FPGA backend of LampaLab or not
+BUILD_FPGA_LAMPALAB ?= 0


It should be BUILD_FPGA_ACCEL.

jserv · 2018-08-02T18:13:40Z

src/dcurl.c

@@ -15,6 +15,8 @@
 #include "pow_avx.h"
 #elif defined(ENABLE_SSE)
 #include "pow_sse.h"
+#elif defined(ENABLE_FPGA_LAMPALAB)


Accordingly, use consistent naming.

jserv · 2018-08-02T18:14:55Z

src/pow_fpga_accel.h

+#include <unistd.h>
+#include "trinary.h"
+
+FILE *ctrl_fd;


Is it necessary to expose these symbols? Can you make them with static?

jserv · 2018-08-02T18:15:21Z

src/trinary.c

 #include "trinary.h"
+#include <stdint.h>


Don't perform such change.

jserv · 2018-08-02T18:15:39Z

src/pow_sse.h

 #include <stdint.h>
+#include "trinary.h"


Ditto. Minimize functional changes.

jserv · 2018-08-02T18:16:06Z

tests/test-pow_fpga_accel.c

@@ -0,0 +1,69 @@
+/* Test program for pow_sse */


It is not pow_sse.

jserv · 2018-08-02T18:21:12Z

src/pow_fpga_accel.c

+#define MWM_MASK_REG_OFFSET 3
+#define CPOW_BASE 0
+
+#define HINTS                                                           \


The message is obsolete. Drop it.

jserv · 2018-08-04T14:09:50Z

Makefile

@@ -39,6 +39,10 @@ ifeq ("$(BUILD_GPU)","1")
 include mk/opencl.mk
 endif

+ifeq ("$(BUILD_FPGA_ACCEL)","1")
+include mk/fpgaaccel.mk


Rename to mk/fpga-accel.mk.

jserv · 2018-08-04T14:12:02Z

mk/fpgaaccel.mk

@@ -0,0 +1 @@
+CFLAGS += -DENABLE_FPGA_ACCEL


You can set CFLAGS in Makefile. The reason why mk/fpga-accel.mk exists is to provide modularized build procedures.

jserv · 2018-08-04T14:12:43Z

src/pow_fpga_accel.h

+#include <sys/mman.h>
+#include <unistd.h>
+#include "trinary.h"
+


Avoid extra blank lines.

furuame · 2018-08-01T13:23:38Z

src/pow_fpga_accel.c

+
+    if (ctrl_fd == NULL) {
+        perror("cpow-ctrl open fail");
+        exit(EXIT_FAILURE);


Since the dcurl would be used by IRI via JNI, the use of exit(3) would cause the entire process terminated.

furuame · 2018-08-01T13:26:51Z

src/pow_fpga_accel.c

+    fpga_regs_map = 0;
+    cpow_map = 0;
+
+    ctrl_fd = fopen("/dev/cpow-ctrl", "r+");


Should this "/dev/cpow-ctrl" be passed as an argument?

I do not known what you mean.

/dev/cpow-ctrl is a device file created by the kernel module, which communicates to FPGA. It would be better if we define several macros to specify these device files in advance.

furuame · 2018-08-01T13:30:59Z

src/pow_fpga_accel.c

+
+int8_t *PowFPGAAccel(int8_t *itrytes, int mwm, int index)
+{
+    char nonce_trits[NONCE_LEN];


To maintain the consistency and portability, could char be replaced with int8_t?

furuame · 2018-08-01T13:33:37Z

src/pow_fpga_accel.h

+#include <unistd.h>
+#include "trinary.h"
+
+FILE *ctrl_fd;


These global variables, which only be used in this scope, would be dangerous if not declared as static variables.

furuame · 2018-08-04T14:38:13Z

src/pow_fpga_accel.c

+        perror("devmem munmap");
+        exit(EXIT_FAILURE);
+    }
+    exit(EXIT_SUCCESS);


Don't exit at this time, because the destroy function is not the end of the program.

furuame · 2018-08-04T14:43:26Z

src/pow_fpga_accel.c

+    size_t itrytelen = 0;
+    size_t itritlen = 0;
+
+    itrytelen = strnlen((char *) itrytes, TRANSACTION_LEN);


I think the length of transaction trytes should be fixed, which is always 2673 in IOTA.

Yes, you say that I agree, but it can be used to check whether length of transaction trytes is correct, e.g. https://github.com/LampaLab/iota_fpga/blob/master/pow_accel_soc/software/curl_pow_hard/curl_pow_hard.c#L82

If the length of itrytes is not equal to TRANSACTION_LEN, it should return an error instead of continuing to do PoW.
I think every implementation should also add this check like this.

if (strlen((char *) itrytes) != TRANSACTION_LEN) return NULL;

This check is added in dcurl_entry() before doing PoW. Would it be better?

furuame · 2018-08-04T14:48:39Z

src/pow_fpga_accel.c

+    exit(EXIT_SUCCESS);
+}
+
+int8_t *PowFPGAAccel(int8_t *itrytes, int mwm, int index)


I think the parameter, index, can just be removed if the FPGA solution can only be used to calculate one PoW at a time?

jserv · 2018-08-04T18:26:23Z

README.md

@@ -12,6 +12,7 @@ Reference Implementation (IRI).
 * Check JDK installation and set JAVA_HOME if you wish to specify.
 * Only one GPU can be facilitated with dcurl at the moment.
 * If your platform doesn't support Intel SSE, dcurl would be compiled with naive implementation.
+* For the IOTA hardware accelerator, we integrate [Lampa Lab's Cyclone V FPGA PoW](https://github.com/LampaLab/iota_fpga) into dcurl. Lampa Lab supports soc_system.rbf only for DE10-nano board. You need to synthesis to get soc_system.rbf for using Arrow SoCKit board.


s/synthesis/synthesize/

jserv · 2018-08-04T18:39:43Z

README.md

@@ -22,6 +23,7 @@ Reference Implementation (IRI).
                   from downloading from
                   [latest JAVA source](https://github.com/chenwei-tw/iri/tree/feat/new_pow_interface).
    - ``BUILD_COMPAT``: build extra cCurl compatible interface.
+    - ``BUILD_FPGA_ACCEL``: build the dcurl interface in communication with the IOTA hardware accelerator on the Cyclone V FPGA board, e.g. DE10-nano board and Arrow SoCKit board. 


Change the statement to "build the interface interacting with the Cyclone V FPGA based accelerator. Verified on DE10-nano board and Arrow SoCKit board."

jserv · 2018-08-04T18:48:36Z

src/pow_fpga_accel.h

@@ -0,0 +1,18 @@
+#ifndef POW_FPGA_LAMPALAB_H_
+#define POW_FPGA_LAMPALAB_H_


It is inconsistent. Change to POW_FPGA_ACCEL_H_.

jserv · 2018-08-12T14:08:26Z

src/pow_fpga_accel.c

+#define CPOW_BASE 0
+
+/* Set FPGA configuration for device files */
+#define CTRL_FPGA_POW "/dev/cpow-ctrl"


Rename CTRL_FPGA_POW to DEV_CTRL_FPGA to represent the usage of device file(s).

jserv · 2018-08-12T14:09:32Z

src/pow_fpga_accel.c

+    devmem_fd = open("/dev/mem", O_RDWR | O_SYNC);
+
+    if (devmem_fd < 0) {
+        perror("devmem open");


Properly use goto to reduce the lines for exception handling.

jserv · 2018-08-13T14:04:48Z

src/pow_fpga_accel.c

+
+    return 1;
+
+fail_dev_open_mem_map:


Changing to fail_to_open_memmap would be mnemonic.

jserv · 2018-08-13T14:05:16Z

src/pow_fpga_accel.c

+
+fail_dev_open_mem_map:
+   close(devmem_fd);
+fail_dev_open_mem_open:


jserv · 2018-08-17T08:56:27Z

README.md

@@ -12,6 +12,7 @@ Reference Implementation (IRI).
 * Check JDK installation and set JAVA_HOME if you wish to specify.
 * Only one GPU can be facilitated with dcurl at the moment.
 * If your platform doesn't support Intel SSE, dcurl would be compiled with naive implementation.
+* For the IOTA hardware accelerator, we integrate [Lampa Lab's Cyclone V FPGA PoW](https://github.com/LampaLab/iota_fpga) into dcurl. Lampa Lab supports soc_system.rbf only for DE10-nano board. You need to synthesize to get soc_system.rbf for using Arrow SoCKit board.


The reason why you adopted the FPGA implementation of Lampa Lab should be addressed as well.

Do you think it is necessary to maintain our own fork for FPGA-based implementation?

Yes, it is necessary to do it. As we know, RocketBoards.org provides Golden System Reference Design (GSRD) [0] includes Linux drivers, OS, boot loader and GHRD for Cyclone V SoC. In the future, we need to integrate the OPTEE-related solution and the Mender-related solution into own modified GSRD and rebuild the SD card image. For GHRD, we maybe provide new HDL-implemented PoW for new PoW algorithm and rebuild the RBF.

[0] Arria V & Cyclone V Golden System Reference Design(GSRD)

@ajblane, got it. Let's fork the repository from Lampa Lab.

furuame · 2018-08-17T16:11:37Z

@ajblane I am waiting for your commit for the new dcurl interface 👍

furuame · 2018-08-21T03:24:33Z

The commit 119e67d for the new ducrl interface is reviewed.
@jserv @ajblane I think it's time to merge it. Should we do it?

jserv

Once the previous requests are fixed, I would approve and merge this pull request.

jserv · 2018-08-21T03:37:40Z

Makefile


 ifeq ("$(BUILD_GPU)","1")
 include mk/opencl.mk
 endif

+ifeq ("$(BUILD_FPGA_ACCEL)","1")


Can you fix?

jserv · 2018-08-21T03:38:02Z

README.md

@@ -66,6 +68,27 @@ $ make BUILD_AVX=1 check
        [ Verified ]
 ```

+* Test with Arrow SoCKit board with [Download](https://github.com/LampaLab/iota_fpga/releases/tag/v0.1) Linux sd-card image, root password is 123456 and you need to download dcurl into root directory. 


jserv · 2018-08-22T16:21:18Z

docs/FPGA-ACCEL.md

@@ -0,0 +1,22 @@
+
+IOTA FPGA-accelerated solution for Dcurl


Use # IOTA FPGA-accelerated solution for dcurl for consistent Markdown style.

jserv · 2018-08-22T16:21:43Z

docs/FPGA-ACCEL.md

+IOTA FPGA-accelerated solution for Dcurl
+----------------------------------------
+
+Dcurl supports IOTA FPGA-accelerated solutions to improve PoW performance. PoW calculation time for MWM=14 is between 0.001 and 0.8 second and 0.14 second in average and The time for MWM=15 is between 0.01 and 2 second and 0.42 second in average. Currently, it is experimented and verfied on Arrow Sockit board and Intel FPGA DE10-Nano board. We reuse the Lampa Lab-provied FPGA-accelerated solution. 


We usually call it dcurl in lowercase.

Typo: "verfied"

jserv · 2018-08-22T16:22:07Z

docs/FPGA-ACCEL.md

+* Resynthesize the POW hardware accelerator for Arrow Sockit board and flash it into the board
+* Integrate the IOTA PoW hardware accelerator into dcurl's implementation interface
+* Test and verify it
+


Don't append trailing space lines.

jserv · 2018-08-22T16:22:43Z

docs/FPGA-ACCEL.md

+* Use System Verilog and UVM to verify the accelerators
+* Synthesize Curl & POW hardware accelerators for Intel FPGA DE10-Nano board and flash it into the board
+* Write Linux drivers in Gloden System Reference Design for Curl & POW hardware accelerators and verify them
+You want to known it much more and further look at [LampaLab/iota_fpga](https://github.com/LampaLab/iota_fpga)


Replace "You want to known it much more and further look at" with "More information: ".

jserv · 2018-08-22T16:28:21Z

src/pow_fpga_accel.c

+    if (!ctx)
+        goto fail_to_malloc;
+
+    for (i = 0; i < impl_ctx->num_max_thread; i++) {


Move int i from Line 49 to the scope in for iteration.

My fault. Skip the previous comment.

ajblane added 3 commits July 26, 2018 22:48

Begin at code of Lampa Lab

d8f0073

Implement related functions for using the accelerator

2154f76

Add the BUILD_FPGA_LAMPALAB option into Makefile

d2bf35d

jserv requested review from furuame and jserv August 1, 2018 08:12

jserv changed the title ~~Integrate Lampa Lab's FPGA PoW into dcurl~~ Integrate Lampa Lab's FPGA-accelerated PoW Aug 2, 2018

jserv reviewed Aug 2, 2018

View reviewed changes

src/trinary.c Outdated

#include "trinary.h"

#include <stdint.h>

Copy link

Member

jserv Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't perform such change.

jserv reviewed Aug 2, 2018

View reviewed changes

src/pow_sse.h Outdated

#include <stdint.h>

#include "trinary.h"

Copy link

Member

jserv Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Minimize functional changes.

jserv reviewed Aug 2, 2018

View reviewed changes

tests/test-pow_fpga_accel.c Outdated

@@ -0,0 +1,69 @@

/* Test program for pow_sse */

Copy link

Member

jserv Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not pow_sse.

jserv changed the title ~~Integrate Lampa Lab's FPGA-accelerated PoW~~ Integrate FPGA-accelerated PoW Aug 2, 2018

jserv assigned ajblane Aug 2, 2018

jserv reviewed Aug 2, 2018

View reviewed changes

src/pow_fpga_accel.c Outdated

#define MWM_MASK_REG_OFFSET 3

#define CPOW_BASE 0

#define HINTS \

Copy link

Member

jserv Aug 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message is obsolete. Drop it.

ajblane force-pushed the fpga-pow-accel branch 5 times, most recently from ca8f5b0 to 9854378 Compare August 4, 2018 13:48

jserv reviewed Aug 4, 2018

View reviewed changes

src/pow_fpga_accel.h Outdated

#include <sys/mman.h>

#include <unistd.h>

#include "trinary.h"

Copy link

Member

jserv Aug 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid extra blank lines.

furuame reviewed Aug 4, 2018

View reviewed changes

ajblane force-pushed the fpga-pow-accel branch 2 times, most recently from 24f22b0 to e2dbc28 Compare August 4, 2018 15:10

jserv reviewed Aug 4, 2018

View reviewed changes

jserv reviewed Aug 12, 2018

View reviewed changes

ajblane force-pushed the fpga-pow-accel branch from 0a19e83 to c6e757f Compare August 13, 2018 02:50

ajblane added 3 commits August 12, 2018 19:53

Use goto to handle exception

2837ed9

Rename macros

43a865f

Maintain the coding style

97be9ec

ajblane force-pushed the fpga-pow-accel branch from c6e757f to 97be9ec Compare August 13, 2018 02:54

jserv reviewed Aug 13, 2018

View reviewed changes

src/pow_fpga_accel.c Outdated

return 1;

fail_dev_open_mem_map:

Copy link

Member

jserv Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing to fail_to_open_memmap would be mnemonic.

jserv reviewed Aug 13, 2018

View reviewed changes

src/pow_fpga_accel.c Outdated

fail_dev_open_mem_map:

close(devmem_fd);

fail_dev_open_mem_open:

Copy link

Member

jserv Aug 13, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto.

ajblane added 4 commits August 16, 2018 23:38

Use low-level I/O

a0f78de

Use memcpy and remove not useful code

20949ae

Rename labels and use clang-format

2002c1e

Use assert

19ea794

jserv reviewed Aug 17, 2018

View reviewed changes

ajblane added 2 commits August 19, 2018 18:14

Solve conflicts

6c98c16

Modify it to the new interface DLTcollab#59

119e67d

jserv requested changes Aug 21, 2018

View reviewed changes

ajblane added 2 commits August 22, 2018 00:03

Drop fpga-accel.mk

fafa723

Modify README and add FPGA-ACCEL.md

f220b24

jserv reviewed Aug 22, 2018

View reviewed changes

Modify FPGA-ACCEL.md

00711c8

ajblane force-pushed the fpga-pow-accel branch from cb30602 to 00711c8 Compare August 22, 2018 16:41

jserv merged commit c341faa into DLTcollab:dev Aug 22, 2018

ajblane deleted the fpga-pow-accel branch August 27, 2018 15:38

		@@ -0,0 +1,18 @@
		#ifndef POW_FPGA_LAMPALAB_H_
		#define POW_FPGA_LAMPALAB_H_

Integrate FPGA-accelerated PoW #50

Integrate FPGA-accelerated PoW #50

Conversation

ajblane commented Jul 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furuame Aug 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furuame commented Aug 17, 2018

furuame commented Aug 21, 2018

jserv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

furuame Aug 7, 2018 •

edited

Loading