About test aes_controler

55-AA commented 4 years ago

Hi, With commit d49714d, I cannot get the out_fifo_data result. What is wrong.

 `include "aes.vh"

`timescale 1ns / 1ps
`define ECB_ENCRYPT_128     (`ECB_MODE_FLAG | `KEY_128_FLAG | `ENCRYPTION_FLAG) //0x4a

 module test_aes_control();

     reg                     clk;
     reg                     reset;

     reg                     in_bus_data_wren;
     reg                     in_bus_tlast;
     reg [`WORD_S-1:0]       in_bus_data;
     /*out*/ wire            controller_in_busy;

     reg                     out_fifo_almost_full;
     reg                     out_fifo_empty;
     reg                     out_fifo_full;
     reg                     out_fifo_write_tready;
     /*out*/ wire            out_fifo_write_tvalid;
     /*out*/ wire[`BLK_S-1:0]out_fifo_data;
     /*out*/ wire            processing_done;

     `define STREAM_BITS    (128+128+32)

     reg [3:0]               work_state;
     `define STATE_START    0
     `define STATE_CMD      1
     `define STATE_KEY      2
     `define STATE_PAYLOAD  3
     `define STATE_RESULT   4

     reg [`WORD_S-1:0]       cur_cmd;
     reg [`BLK_S-1:0]        cur_key;
     reg [`BLK_S-1:0]        cur_key_iv;
     reg [`BLK_S-1:0]        cur_text;
     reg [`STREAM_BITS-1:0]  stream;
     reg [`STREAM_BITS-6:0]  stream_count;
     reg [`STREAM_BITS-6:0]  stream_idx;

     aes_controller aes(
         .clk(clk),
         .reset(reset),

         .in_bus_data_wren(in_bus_data_wren),
         .in_bus_tlast(in_bus_tlast),
         .in_bus_data(in_bus_data),
         .controller_in_busy(controller_in_busy),

         .out_fifo_almost_full(out_fifo_almost_full),
         .out_fifo_empty(out_fifo_empty),
         .out_fifo_full(out_fifo_full),
         .out_fifo_write_tready(out_fifo_write_tready),
         .out_fifo_write_tvalid(out_fifo_write_tvalid),
         .out_fifo_data(out_fifo_data),
         .processing_done(processing_done)
     );

     function [`BLK_S-1:0] swap_bytes128(input [`BLK_S-1:0] data);
         integer i;
         begin
             for (i = 0; i < `BLK_S / `BYTE_S; i=i+1)
                 swap_bytes128[i*`BYTE_S +: `BYTE_S] = data[(`BLK_S / `BYTE_S - i - 1)*`BYTE_S +: `BYTE_S];
         end
     endfunction

     localparam [127:0] KEY_5            = 128'H0000000011111111_2222222223333333;
     localparam [127:0] PLAINTEXT_5      = 128'Haaaaaaaabbbbbbbb_ccccccccdddddddd;

     initial begin
         clk = 0;
         work_state = `STATE_START;
         cur_cmd = `ECB_ENCRYPT_128;
         cur_key = swap_bytes128(KEY_5);
         cur_key_iv = swap_bytes128(KEY_5);
         cur_text = swap_bytes128(PLAINTEXT_5);
     end

     always @ (posedge clk) begin
         reset <= 0;

         case (work_state)
             `STATE_START: begin
                 work_state <= `STATE_CMD;
                 reset <= 1;

                 in_bus_data_wren <= 1;
                 out_fifo_almost_full <= 0;
                 out_fifo_empty <= 1;
                 out_fifo_full <= 0;
                 out_fifo_write_tready <= 1;

                 stream_count <= 0;
                 stream <= cur_cmd;
                 stream_idx <= 0;
             end

             `STATE_CMD: begin
                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;

                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_KEY;
                         in_bus_tlast <= 1;

                         stream_count <= 4-1;
                         stream <= {cur_key};
                         stream_idx <= 0;
                     end else begin
                         in_bus_tlast <= 0;
                     end
                 end else begin
                     in_bus_data_wren <= 0;
                 end
             end

             `STATE_KEY: begin
                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;

                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_PAYLOAD;
                         in_bus_tlast <= 1;

                         stream_count <= 4-1;
                         stream <= cur_text;
                         stream_idx <= 0;
                     end else begin
                         in_bus_tlast <= 0;
                     end
                 end else begin
                     in_bus_data_wren <= 0;
                 end
             end

             `STATE_PAYLOAD: begin
                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;

                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_RESULT;
                         in_bus_tlast <= 1;
                         in_bus_data_wren <= 0;
                     end else begin
                         in_bus_tlast <= 0;
                     end
                 end else begin
                     in_bus_data_wren <= 0;
                 end
             end

             `STATE_RESULT: begin
                 if (out_fifo_write_tvalid) begin
                     $display("KEY:  %h", swap_bytes128(cur_key));
                     $display("INPUT:%h", swap_bytes128(cur_text));
                     $display("OUT:  %h", swap_bytes128(out_fifo_data));
                     $stop;
                 end
             end
         endcase
     end

     always #1 clk=~clk;

 endmodule

ovpanait commented 4 years ago

Hi,

There are a couple of problems with the code sample:

in_bus_tlast must be asserted only once per packet, for the last 32-bit word: 1 packet = cmd + key + [iv] + payload

In the example, tlast is asserted three times: once after cmd, once after key and once after the AES block. Also, tlast must be deasserted after the last 32-bits are sent.

In STATE_PAYLOAD, for the last 32-bits:
```
if (stream_idx == stream_count) begin
                     work_state <= `STATE_RESULT;
                     in_bus_tlast <= 1;
                     in_bus_data_wren <= 0;
```
in_bus_data_wren is deasserted, so the controller can't know that valid data is pending on the bus. The in_bus_data_wren <= 0; line should be deleted.

Here is an updated code sample, that works on my side:

`include "aes.vh"

`define ECB_ENCRYPT_128     ((1 << `ECB_MODE_BIT) | (1 << `KEY_128_BIT) | (1 << `ENCRYPTION_OP_BIT)) //0x4a

 module tb_main();

     reg                     clk;
     reg                     reset;

     reg                     in_bus_data_wren;
     reg                     in_bus_tlast;
     reg [`WORD_S-1:0]       in_bus_data;
     /*out*/ wire            controller_in_busy;

     reg                     out_fifo_almost_full;
     reg                     out_fifo_empty;
     reg                     out_fifo_full;
     reg                     out_fifo_write_tready;
     /*out*/ wire            out_fifo_write_tvalid;
     /*out*/ wire[`BLK_S-1:0]out_fifo_data;
     /*out*/ wire            processing_done;

     `define STREAM_BITS    (128+128+32)

     reg [3:0]               work_state;
     `define STATE_START    0
     `define STATE_CMD      1
     `define STATE_KEY      2
     `define STATE_PAYLOAD  3
     `define STATE_RESULT   4

     reg [`WORD_S-1:0]       cur_cmd;
     reg [`BLK_S-1:0]        cur_key;
     reg [`BLK_S-1:0]        cur_key_iv;
     reg [`BLK_S-1:0]        cur_text;
     reg [`STREAM_BITS-1:0]  stream;
     reg [`STREAM_BITS-6:0]  stream_count;
     reg [`STREAM_BITS-6:0]  stream_idx;

     aes_controller aes(
         .clk(clk),
         .reset(reset),

         .in_bus_data_wren(in_bus_data_wren),
         .in_bus_tlast(in_bus_tlast),
         .in_bus_data(in_bus_data),
         .controller_in_busy(controller_in_busy),

         .out_fifo_almost_full(out_fifo_almost_full),
         .out_fifo_empty(out_fifo_empty),
         .out_fifo_full(out_fifo_full),
         .out_fifo_write_tready(out_fifo_write_tready),
         .out_fifo_write_tvalid(out_fifo_write_tvalid),
         .out_fifo_data(out_fifo_data),
         .processing_done(processing_done)
     );

     function [`BLK_S-1:0] swap_bytes128(input [`BLK_S-1:0] data);
         integer i;
         begin
             for (i = 0; i < `BLK_S / `BYTE_S; i=i+1)
                 swap_bytes128[i*`BYTE_S +: `BYTE_S] = data[(`BLK_S / `BYTE_S - i - 1)*`BYTE_S +: `BYTE_S];
         end
     endfunction

     localparam [127:0] KEY_5            = 128'H0000000011111111_2222222223333333;
     localparam [127:0] PLAINTEXT_5      = 128'Haaaaaaaabbbbbbbb_ccccccccdddddddd;

     initial begin
         clk = 0;
         work_state = `STATE_START;
         cur_cmd = `ECB_ENCRYPT_128;
         cur_key = swap_bytes128(KEY_5);
         cur_key_iv = swap_bytes128(KEY_5);
         cur_text = swap_bytes128(PLAINTEXT_5);
     end

     always @ (posedge clk) begin
         reset <= 0;

         case (work_state)
             `STATE_START: begin
                 work_state <= `STATE_CMD;
                 reset <= 1;

                 in_bus_data_wren <= 1;
                 out_fifo_almost_full <= 0;
                 out_fifo_empty <= 1;
                 out_fifo_full <= 0;
                 out_fifo_write_tready <= 1;

                 stream_count <= 0;
                 stream <= cur_cmd;
                 stream_idx <= 0;
             end

             `STATE_CMD: begin
                 in_bus_tlast <= 0;

                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;

                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_KEY;

                         stream_count <= 4-1;
                         stream <= {cur_key};
                         stream_idx <= 0;
                     end
                 end
             end

             `STATE_KEY: begin
                 in_bus_data_wren <= 0;

                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;

                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_PAYLOAD;

                         stream_count <= 4-1;
                         stream <= cur_text;
                         stream_idx <= 0;
                     end
                 end
             end

             `STATE_PAYLOAD: begin
                 in_bus_tlast <= 0;
                 in_bus_data_wren <= 0;

                 if (!controller_in_busy) begin
                     in_bus_data <= stream[stream_idx*`WORD_S +: `WORD_S];
                     stream_idx <= stream_idx + 1;
                     in_bus_data_wren <= 1;
                     if (stream_idx == stream_count) begin
                         work_state <= `STATE_RESULT;
                         in_bus_tlast <= 1;
             end
                 end
             end

             `STATE_RESULT: begin
            in_bus_tlast <= 1'b0;
                 if (out_fifo_write_tvalid) begin
                     $display("KEY:  %h", swap_bytes128(cur_key));
                     $display("INPUT:%h", swap_bytes128(cur_text));
                     $display("OUT:  %h", swap_bytes128(out_fifo_data));
                     $stop;
                 end
             end
         endcase
     end

     always #1 clk=~clk;

 endmodule

ovpanait commented 4 years ago

However, I am not sure this is the proper way to test the controller. It expects to receive the write enable signal only on the edges when the controller_in_busy is 0.) (see the hdl/axi_stream_slave.v - an axi stream slave controller that asserts its tready signal based on the controller_in_busy signal).

In your code sample, on each FSM state, it first checks on the clock edge if the controller is not busy, and then asserts the write enable signals and loads the data. The write enable signal is sampled on the next clock cycle by the controller. This might cause problems if in the meantime the controller_in_busy goes HIGH.

Currently, I think the only way to correctly use it is to put in in a TVALID/TREADY handshake configuration and use the controller_in_busy to drive tready.

I am still experimenting with the interfaces, maybe I will change the current implementation. For now it works well with the current AXI stream slave. I plan to formally verify the whole design, so multiple components might be reworked. It might take some time though.

55-AA commented 4 years ago

Thanks for your answer. I will be looking forward to your new version.

ovpanait / zynq-aes

About test aes_controler #5