aws / aws-fpga

Official repository of the AWS EC2 FPGA Hardware and Software Development Kit
Other
1.5k stars 513 forks source link

Bug in PCIM interface model? #589

Open nmoroze opened 1 year ago

nmoroze commented 1 year ago

Hi, I think I may have found a bug in the PCIM interface model in sh_bfm.sv. The problematic logic (with lines omitted for clarity) is the following:

      if (sh_cl_rd_data.size() != 0) begin
         // ...
         sh_cl_pcim_rvalid <= !sh_cl_pcim_rvalid ? 1'b1 :
                                 !cl_sh_pcim_rready ? 1'b1 :
                                 !sh_cl_pcim_rlast  ? 1'b1 : 1'b0;
         // ...
         beat = {512{1'b1}};

       if (cl_sh_pcim_rready) begin
         // set beat ... 
         sh_cl_pcim_rdata <= beat;
       end //if(cl_sh_pcim_rready)
      end

When a new read request is received from the CL, the SH model sets sh_cl_pcim_rvalid to 1 on the next cycle. However, it doesn't actually put data into sh_cl_pcim_rdata unless cl_sh_pcim_rready is asserted. If the CL has already set rready high before the read request goes through, then this logic works fine. However, if rready is asserted after rvalid goes high, then the data becomes valid a cycle late. I believe this violates the AXI handshake protocol, since the data needs to be valid on the positive edge that rvalid and rready are high.

Does that sound right to you?

Thanks!

AWSjoeluc commented 1 year ago

Hi Noah,

Thanks for reaching out to AWS on this issue. Can you please provide a response to the following questions?

  1. Did you observe any unexpected behavior in your simulations with this sh_bfm.sv?
  2. If so, were you using a CL provided by AWS or your own CL design?

Thanks! -Joe

AWSjoeluc commented 1 year ago

Hi Noah,

We were able to reproduce the issue you described above. We're currently working on a fix and will let you know when it's released!

Thank you so much! -Joe

nmoroze commented 1 year ago

Thank you very much! In case the replies to your previous queries are still useful: yes, I did observe unexpected behavior and I was using my own CL design.

Let me know if there's any other information I can provide that would be useful!