AR review feedback

[ved-rivos]

1. Preamble - Should be Stable for ARC review
2. Chapter 1 - first para - "application: Typo, should be plural"
3. Chapter 1 - workloads -> workloads'
4. Chapter 1 - missing commas: ", such as public cloud servers, "
5. Chapter 1 - missing comma: "shared resources leading to" -> "shared
   resources, leading to"
6. Chapter 1 - Should be "another workload's" or "other workloads'"
7. Chapter 1 - What does single-copy atomic mean?
   Ved: This is defined by the Unpriv spec - Chapter 17 - i.e.
   cannot be observed in partially complete state.

Chapter 2:

1. Fourth paragraph - missing comma
2. A non-normative statement about why separate IDs are needed for resource
   control vs monitoring would be helpful. I see that section 6 discusses
   this. Maybe reference it in a non-normative comment?
3. Remove comma after "The Ssqosid extension"
4. The way this sentence is written leads me to believe that Ssqosid is defined
   elsewhere, and this is just a reference to it. I'd either mention in the
   introduction that this spec defines this ISA extension, or make it clear here.
   Ved: Yes, this is a fast track. Restructured to provide a reference and
   remove text from this paragraph.
5. I think menvcfgh is implicit, and adding this is a bit confusing since QOSE
   is not bit 58 in menvcfgh. I'd remove the parenthetical.
   Ved: Yes, this moves to mstateen
6. Section 2.1.2 - QOSE seems more like a state-enable bit than an extension
   enable bit. Would it make sense to move it to mstateen0, where there are more
   bits available?
   Ved: Yes, this moves to mstateen
   Chapter 3:
7. Might be better to refer to cache lines instead of cache blocks, since
   "blocks" is overloaded here with capacity blocks.
   Ved: RISC-V refers to them as cache blocks - consitent with Zic* extensions.
8. Third para following table 2:
   register, not registers. But why not list the reset values for these fields
   in the definition for the register/field below, rather than here? The
   implications of this are not clear here.
   Ved: Since fields with reset are just these two - have them listed here
   with a statement that reset value for other fields is unspecified.
9. If all capacity is allocated to RCID 0 by default, then isn't the allocation
   for other RCIDs implicitly 0?
   Ved: It may or may not be 0. The register files in the controllers may
   have arbitrary values for the non-zero RCID. Software should not use non-zero
   RCID without first initializing thier allcoation.
10. VER: Does this field imply that you expect that future versions may not be
    backwards compatible?
    Ved: Future versions may or may not be backward compatible. Each future
    version should provide that definition.
11. Figure 3 - Why WPRI instead of 0? Do you expect status bits, updated by HW,
    may be added in the future? Or are there cases where some SW agent may write
    bits here that another SW agent will observe?
    Ved: WPRI is following definition of the term in Priv spec. In future there
    may be RW fields defined that default to 1 at reset. SW should preserve them.
    There may be fields that are RW and default to 0 at reset. SW should not rely
    on a write of 1 being ignored.
12. I'd put the table 3 right after this paragraph. Then you probably don't need
    the second sentence.
13. Again, I'd put table 4 right after this. I'd also add a link to the AT
    encoding table in the paragraph.
14. Is this only when OP=CONFIG_EVENT? And I assume this is on the write, not
    any time EVT_ID and MCID hold legal non-zero values. I'm not sure "is
    programmed" makes that clear.
    Ved: Yes. Updated to clarify.
15. Would the counter stabilize sooner if the counter didn't wrap from 0 to -1?
    Ved: Yes.
16. Section 3.3 If you prefer to keep your non-normative blocks together on a page,
    you can add [%unbreakable] on the line after [NOTE]. I don't care if you do so,
    just FYI.
17. Section 3.4 - second para - This is mostly redundant with the second sentence
    in the paragraph above. Consolidate.
18. Table 6 - Flush RCID description - put block in italics
19. I'm confused. "Deallocate" suggests that it undoes the allocation. Is this
    just a flush then, where it evicts each line but keeps them allocated to the
    same RCID? Sounds like there's no way to remove an allocation for an RCID once
    it's added. If so I probably wouldn't use deallocate in this description.
    Ved: It was intended as a flush i.e. deallocate the capacity used by the
    RCID in this controller by evicting the occupation. Updated the description.
20. Section 3.5 comma after allocation
21. Nit, but I might just refer to "the equation below".
22. Nit, but usually this is just stated as "read-only zero"
23. add "by programming" after the comma, otherwise it reads like cc_cunits is
    another thing programmed in cc_block_mask
24. Section 3.6 - Match the text for the reg above and say "then this register
    is read-only 0."
25. Section 3.6 - same comment as the last sentence

Chapter 4:

1. Many comments from ch 3 apply here as well
2. Section 4.2 - Lots of different ways to say "read-only zero". :)
3. Section 4.3 - typos, need a space and make determine plural
4. Why no OVF bit for the cc_mon_ctr_val? Is it just because BW will count
   much faster?
   Ved: Unlike bandwidth, the monitoring counters for capacity are sized for
   the capacity and cannot overflow since the capacity is fixed. Whereas
   bandwidth if counted for sufficiently long time will overflow
5. Should unimplemented bits be ROZ, so that SW can discover the width of the
   counter?
6. Section 4.4 - Remove "also".
7. Section 4.4 - But AT can always be 0 right? So doesn't this really mean
   that, if allocation is specified for one access-type, then allocations must be
   specified for all other access-types?
   Ved: AT of 0 is also a legal AT. If multiple AT are supported they must have
   an allocation specified - either by themself or as being shared with another
   AT.
8. Wouldn't it be better to just set AT=0 here?
   Ved: AT of 0 implies "Data access" . AT of 0 is not a "default". When AT are
   not supported, the controller ignores the incoming AT an treats all request
   as having an AT of 0.
9. Section 4.5 - read-only zero?
10. Page 25 - make this column a bit wider, to avoid wrap

Chapter 5:

1. Section 5.1.1 informative note - Looks like it is required, not suggested
2. Section 5.1.2 - enumerates (add s)
3. Section 5.1.3 - first para after table - fix typos.
4. Section 5.2 - device-initiated
   Ved: No should be IOMMU initiated as these are iommu data structures.
5. Section 5.3 - "e.g. corresponding to each of the page sizes supported"
   And I might move this to right after "multiple IOATC".

Chapter 6:

1. So typical implementaitons (as describedabove, with more MCIDs than RCIDs) do
   not support maximal flexibility? Does this recommendation imply that there
   should be 100s of RCIDs, rather than 10s of MCIDs?
   Ved: The suggestion is that all controllers support identical number of IDs else
   SW will need to use the lowest common denominator. Added a further comment
   to clarify that.
   Chapter 7:
2. Second para - add "a"
3. Section 7.4 - Shouldn't this be ABI?
   Ved: I meant SBI but ABI is more general
4. I'd say "may opt not to". Otherwise it sounds like S/HS is not allowed to modify sqoscfg.
5. Same comment as above.

[ved-rivos]

PR #15 created to address the feedback.

[bcstrongx]

One very small comment:

When multiple IOATC are implemented, (e.g., corresponding to each page sizes supported),

I think you want to remove the first comma, and change the parenthetical to say "an IOATC corresponding to each of the page sizes supported"

Otherwise looks good!

[ved-rivos]

Thanks. Updated in PR #20