search

riscv-non-isa / riscv-rpmi

RISC-V Platform Management Interface Specification. OS-agnostic messaging interface for system management and control
https://jira.riscv.org/browse/RVG-151
Creative Commons Attribution 4.0 International
8 stars 8 forks source link

Feedback on chapter 2 and chapter 3 #79

Open ved-rivos opened 2 days ago

ved-rivos commented 2 days ago
  1. The distinction between a fast channel vs. a normal channel is unclear. Do fast and normal channels co-exist between same pair of producers and consumers? If so what is the criteria for selecting between the two.
  2. Section 2.2/2.3 - explain more about the caching side effects and what is being avoided.
  3. For P2A channels - what is the distinction between a notification and a request? Do notifications not require an Ack?
  4. Replace "cache line" with "cache block"
  5. Explain rationale for the head and tail to use the -2 notation? As the effective size of the buffer is M-2, there is already a need to special case the wrap decision.
  6. Section 2.3.2 - explain the cache maintenance intended to be done to the head and tail slots.
  7. What are the semantics of the head and tail. Are they expected to have a wrap indicator or is the convention that tail = head-1 indicates a full queue?
  8. Section 2.3.3 - the non-normative comment about having as many slots as number of application processors indicates that this queue is expected to be concurrently operated on by multiple application processors? If so then in the P2A direction: how is the de-multiplixing of requests/notifications/acks performed and which application processor fields the interrupts?
  9. section 2.3.3 - the privileged architecture does not have a concept of PMA entries. Each location in memory has a PMA. Please uplevel this note to explain the intent of the note and call out what type of optimization are intended.
  10. Section 2.3.3 : typo "shared shared" -> "shared"
  11. Chapter 2: Which entity is responsible for initializing the head and tail pointer and the queue memory. What coordination mechanisms are used to re-init the queue (for instance on a kexec).
  12. Section 3.1 - is non-posted request a better term to complement posted request?
  13. section 3.2 - is the message length variable within a slot or can a message occupy multiple slots?
  14. Section 3.2.2 - what is the intent of flag[3]. The earlier sections indicate that the producer must explicitly ring a doorbell. Is "enabled/disabled" meant to indicate that doorbells must not be used for a queue or does it indicate that they are not used for this specific message?
  15. Section 3.2.2 - what is the intent of the Token. Are the sequence numbers required to be monotonic? What are the sequence numbers initialized to? Are there any error checks for missing sequence numbers? How are sequence numbers synchronized (for instance after a kexec).
  16. section 3.2.2 - are there any error checks done on data len? What is handling for a zero length message as the header does not have any operation code?
  17. When multipart acknowledgements are used, how are the multiple parts identified? Can acknowledgements for two different messages be interleaved? Does the STATUS field of each multipart message need to be identical and if not what are the rules for the overall STATUS?
dhaval-rivos commented 1 day ago

Additional comments:

  1. Now that we are extending RPMI to MM as well, in the introduction we should clarify that it is not just for PuC.
  2. Figure 2 should be updated to reflect req-fwd API implementation
  3. Nit: figure2 should have "System Monitoring And Control" instead of just system control.
  4. How do we define Ownership of the channel (I think it is mentioned in one of the diagrams). Is it of any significance?
  5. Messaging Protocol: Typo, response which is "s/send/sent" back as an RPMI acknowledgement
  6. Since we are dealing with PuC, should spec cover a scenario where request times out and any corrective action is required or that is left to the platform FW? There is RPMI_ERR_TIMEOUT but for that Ack has to come which may not happen in some scenarios?
  7. Messaging Protocol: Should this be renamed to /sRPMI_ERR_ALREADY/RPMI_ERR_INPROGRESS?
  8. MPXY: "The SBI MPXY channel must correspond to a single RPMI service group". This means one channel can not be mapped to multiple groups. Can we also clarify other way around that it is possible to have multiple channels that support a service group type. i.e. there can be 2 channels for MM service group.
  9. RPMI service group: Typo "A RPMI service group may be partially implement".."service group may implement..."
  10. We do not need this now I believe: any specific use case for this? MM_ENABLE_NOTIFICATION
  11. MM_SHMEM_ADDR_LOW: In a typical MM implementation NS buffer is allocated by edk2 and then passed into MM as a HOB. If that implementation is done then this attr may not be necessary. But there are some challenges with that implementation that is under discussion. So if eventually implementation does not support this attribute, is there a way to inform caller? Just send 0 in the address?
  12. REQFWD_ENABLE_NOTIFICATION this may not be necessary?