[XIF] eXtension Interface Review Feedback

Created by: vogelpi

Hi @Silabs-ArjanB ,

I did review your updated eXtension interface. In general, this looks good and I think the main limitation of the original CVXIF spec on back to back memory ops etc. are mitigated with your proposal.

Below, I have a list of remarks/questions regarding your proposal. I've posted them here as your proposal currently lives in this repo. Ultimately, it would be great if your proposal got merged into the CVXIF spec.

Compressed Instructions:

1. In case of compressed instructions, why does the core need the uncompressed instruction in the first place? Isn't it possible to to identify the rs/rd indices already from the compressed instruction?
2. Your proposal says "The instruction is originally compressed and the coprocessor accepted the compressed instruction and provided a 32-bit uncompressed instruction. In this case the 32-bit uncompressed instruction will be attempted for offload even if it matches in the core’s non-compressed instruction decoder." Does that mean it would be legal for a coprocessor to have instructions with uncompressed encodings overlapping with other instructions, e.g., of the base instruction set? Isn't that dangerous? Is this really necessary or did I misunderstand something?
3. Signal dependencies: In your proposal you note that CV32E40X splits the compressed/uncompressed decoders with a pipeline stage to avoid outputs depending directly on inputs. For simpler and configurable cores like Ibex and if one doesn't target max frequency, this might not be worth the complexity. Meaning such cores wouldn't comply with the spec. Should we make an exception here for simple cores? Or weaken the requirement into a recommendation?

Memory Transactions:

1. Should we specify load/store support on the core side as optional feature for the extension interface? Obviously for CV32E40X you will have it but I can imagine that the most simple cores/coprocessors don't need this.
2. For unaligned loads/stores (split into two aligned transactions) a PMP look-up is required for both aligned transactions. This means a synchronous PMP fault can happen in at least two cycles. I think the proposal and also the original spec currently can't handle this. Your proposal assumes that the fault is signaled in x_mem_resp_t during the x_mem_valid/ready handshake. We should probably discuss this.

Handshakes:

1. Some typos "The signals in x_issue_resp_i are valid when x_issue_req_o and x_issue_resp_i are both 1. There are no stability requirements." I think this should read "The signals in x_issue_resp_i are valid when x_issue_req_o and x_issue_ready_i are both 1. There are no stability requirements."
2. Two interfaces don't have a proper handshake: x_commit_valid_o/x_mem_result_valid_o. I assume this is to not make the core overly complex and I agree with this choice. But maybe we should discuss this in the working group and think if this can cause difficulties in the coprocessor that we didn't think of yet.
3. In the Handshake rules section (a space is needed between "with" and "``")
4. Your proposal specifies that valid can be dropped without ready in case an instruction gets killed (I agree with this). The next sentence then says: "A new transaction can be started by changing the id signal and keeping the valid signal asserted (thereby possibly terminating a previous transaction before it completed)." I would strictly specify that this is only possible due to the killl signal either. Or did you have some other scenario in mind here?

General remarks:

1. It's not 100% clear to me whether all interfaces are in-order or can be out-of-order. Your spec says "A coprocessor is allowed to provide results to the core in an out of order fashion." and at the same time "The transaction ordering on the result interface must correspond to the transaction ordering on the issue interface.".