Fix TCP stack – vFPGA routing and add TCP performance examples#149
Fix TCP stack – vFPGA routing and add TCP performance examples#149adpp00 wants to merge 2 commits intofpgasystems:masterfrom
Conversation
bo3z
left a comment
bo3z
left a comment
There was a problem hiding this comment.
Thank you for bringing back the TCP/IP stack. The code looks very good. I've finished the review and there are some minor comments that cover corner cases and make sure the TCP/IP stack follows the same design principles as other components in Coyote. I can look into addressing these.
| `ifdef EN_TCP | ||
| // TCP | ||
| metaIntf.m m_open_port_cmd, | ||
| metaIntf.s m_open_port_sts, | ||
| metaIntf.m m_open_conn_cmd, | ||
| metaIntf.s m_open_conn_sts, | ||
| `endif | ||
|
|
There was a problem hiding this comment.
TCP management functions (open connection, close connect etc.) should be part of the MMU and cnfg_slave as memory mapped registers. This is to make it consistent with RDMA. More details in the overview comment of this PR.
| // TCP | ||
| `ifdef EN_TCP | ||
| metaIntf.m m_open_port_cmd, | ||
| metaIntf.s s_open_port_sts, | ||
| metaIntf.m m_open_conn_cmd, | ||
| metaIntf.s s_open_conn_sts, | ||
| `endif | ||
|
|
There was a problem hiding this comment.
Same comment as in the MMU; TCP management interfaces (tcp_listen_req, tcp_listen_rsp, tcp_open_req, tcp_open_rsp, tcp_close_req) should be in memory-mapped registers here (in cnfg_slave, cnfg_slave_avx) and not in the vFPGA user logic. Can we move these 5 here?
| metaIntf.s s_open_conn_sts, | ||
| `endif | ||
|
|
||
| // Writeback |
There was a problem hiding this comment.
Same comment as in cnfg_slave.
| if (sid_hit) begin | ||
| dst_vfid_N = vfid_sid; | ||
| end else begin | ||
| dst_vfid_N = vfid_port; | ||
| end | ||
| end |
There was a problem hiding this comment.
For me to better understand - is it common to have sid_hit = 0? Assuming the connections were set-up properly, such behaviour shouldn't really happen, right?
| for(genvar i = 0; i < N_REGIONS; i++) begin | ||
| assign m_rx_meta_ready[i] = m_rx_meta[i].ready; | ||
| assign m_rx_meta[i].valid = m_rx_meta_valid[i]; | ||
| assign m_rx_meta[i].data = m_rx_meta_data[i]; |
There was a problem hiding this comment.
Will this synthesise correctly? If I recall correctly, arrays inside generate blocks are finnicky.
|
|
||
| /** | ||
| * @brief TCP RX multiplexer | ||
| * @brief RX arbitration (tlast-based) |
There was a problem hiding this comment.
Does the TCP stack guarantee there will always be a tlast?
| logic [N_REGIONS-1 : 0] s_axis_tx_last; | ||
| logic [AXI_DATA_BITS-1 : 0] s_axis_tx_data [N_REGIONS]; | ||
|
|
||
| for(genvar i = 0; i < N_REGIONS; i++) begin |
There was a problem hiding this comment.
As above, we should check if these arrays will synthesize correctly in a genvar block.
2 participants
Description
This pull request includes:
tcp_perf)Type of change
Tests & Results
tcp_perfChecklist