Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

reorder capability chapter to show cap layout first #23

Merged
merged 1 commit into from
Apr 8, 2024

Conversation

axel-h
Copy link
Contributor

@axel-h axel-h commented Jan 23, 2024

Show the encoding first, then describe the field. This is what many other specs also do. When reading the specification, it gives a better overview what the rest of the chapter is about as one can picture something.

@axel-h axel-h changed the title reorder capability chapter to show cap layout fist reorder capability chapter to show cap layout first Jan 23, 2024
@axel-h axel-h force-pushed the patch-axel-1 branch 2 times, most recently from cbf0bbc to d1f6575 Compare January 23, 2024 14:36
@axel-h
Copy link
Contributor Author

axel-h commented Jan 23, 2024

Aligned with the changes proposed in #19

@Timmmm
Copy link
Contributor

Timmmm commented Jan 24, 2024

I agree this is much better.

@axel-h
Copy link
Contributor Author

axel-h commented Jan 25, 2024

rebased, separated changes to different commits to allow better tracking.

@sorear
Copy link
Contributor

sorear commented Jan 25, 2024

It would be useful to start the chapter with a picture of the programmer's view of a capability. IMO it is extremely unuseful to start with a picture of the compressed capability bit representation, since the compression format is difficult to understand without first understanding the valid values of the abstract fields.

@Timmmm
Copy link
Contributor

Timmmm commented Jan 25, 2024

I agree it might seem weird but I definitely remember reading it and thinking "it's talking about all these things (bounds, permissions, etc.) but what are they?". Showing the compressed capability first gives you something concrete to explain.

Currently it's like trying to explain a car to someone that has never seen one before by saying "the components of a car are: wheels, engine, windows, ..." rather than than "here is a picture of a car, these bits are called the wheels, etc."

So yeah, it's backwards if you're writing a functional program, but I still think it's much much clearer. And I distinctly recall starting to read the "components of a capability, giving up, searching for the diagram and thinking 'this should be first'". If there are two of us with the exact same thought...

@Timmmm
Copy link
Contributor

Timmmm commented Jan 25, 2024

I think the wording could be refined after this is merged but it probably makes sense just to move things first.

@sorear
Copy link
Contributor

sorear commented Jan 25, 2024

What would people think about replacing everything between EF and BE in the introductory picture with a single BOUNDS field, and deferring the details of how the bounds are encoded until the bounds encoding chapter, in much the same way that we don't call out individual bits in SDP and AP and defer those to their respective chapters? (Don't block the merge on account of this question, this is a proposal for post-merge refinement.)

@arichardson
Copy link
Collaborator

What would people think about replacing everything between EF and BE in the introductory picture with a single BOUNDS field, and deferring the details of how the bounds are encoded until the bounds encoding chapter, in much the same way that we don't call out individual bits in SDP and AP and defer those to their respective chapters? (Don't block the merge on account of this question, this is a proposal for post-merge refinement.)

I think grouping the fields together would be very helpful. Not sure if we need two separate diagrams for that, maybe the asciidoc diagramming tool could allow us to add some shading and a legend or a grouping annotation? I would assume this is possible but I've not used it before.

I believe reording makes a lot of sense and is clearer.

@Timmmm
Copy link
Contributor

Timmmm commented Jan 26, 2024

Yeah I agree, a BOUNDS field makes loads of sense. Something like this?

image
image

The other thing that I think would make things clearer is a separate diagram for E=0, but that can probably wait, or maybe go in a separate explanatory document.

image

[bytefield]
----
(def svg-attrs {:style "background-color:white"})
(defattrs :plain [:plain {:font-family "M+ 1p Fallback" :font-size 30}])
(defattrs :bg-yellow {:fill "#ffffc0"})
(def row-height 80)
(def row-header-fn nil)
(def left-margin 30)
(def right-margin 30)
(def bottom-margin 30)
(def boxes-per-row 32)
(draw-column-headers {:height 50 :font-size 26 :labels [
    "63" "" "" "57"
    "56" "53"
    "52" "48"
    "47" "" "" "" "" "" "" "" "28"
    "27"
    "26" "" "" "" "" "" "" "" "" "" "" "" "" "0"
]})

(draw-box "Reserved"    {:span 4})
(draw-box "SDP"         {:span 2})
(draw-box "AP"          {:span 2})
(draw-box "Reserved"    {:span 9})
(draw-box "S"           {:span 1})
(draw-box "BOUNDS"      [{:span 14} :bg-yellow])
(draw-box "Address"     {:span 32})
----

[bytefield]
----
(def svg-attrs {:style "background-color:white"})
(defattrs :plain [:plain {:font-family "M+ 1p Fallback" :font-size 30}])
(defattrs :bg-yellow {:fill "#ffffc0"})
(def row-height 80)
(def left-margin 30)
(def right-margin 30)
(def bottom-margin 30)
(def boxes-per-row 14)
(draw-column-headers {:height 50 :font-size 26 :labels [
    "26"
    "25" "" "" "17"
    "16" "14"
    "13" "" "" "" "3"
    "2" "0"
]})

(draw-box "EF"          [{:span 1} :bg-yellow])
(draw-box "T[11:3]"     [{:span 4} :bg-yellow])
(draw-box "TE"          [{:span 2} :bg-yellow])
(draw-box "B[13:3]"     [{:span 5} :bg-yellow])
(draw-box "BE"          [{:span 2} :bg-yellow])
----

@axel-h
Copy link
Contributor Author

axel-h commented Jan 26, 2024

I think the wording could be refined after this is merged but it probably makes sense just to move things first.

have have moved this out to other PRs, so here is just the reordering.

a BOUNDS field makes loads of sense. Something like this?

Yes, this look easier for the initial picture, then the dedicated BOUNDS section can break this down. Having separate Diagrams for EF 0/1 also make it easier to understand this.

@arichardson
Copy link
Collaborator

I think the wording could be refined after this is merged but it probably makes sense just to move things first.

have have moved this out to other PRs, so here is just the reordering.

a BOUNDS field makes loads of sense. Something like this?

Yes, this look easier for the initial picture, then the dedicated BOUNDS section can break this down. Having separate Diagrams for EF 0/1 also make it easier to understand this.

Agreed, I think both of these changes will make it easier to understand.

Copy link
Collaborator

@arichardson arichardson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just one non-blocking comment.

src/cap-description.adoc Show resolved Hide resolved
arichardson pushed a commit to arichardson/riscv-cheri that referenced this pull request Jan 30, 2024
@axel-h axel-h force-pushed the patch-axel-1 branch 2 times, most recently from accfc3b to 276f2ea Compare January 31, 2024 12:30
@axel-h
Copy link
Contributor Author

axel-h commented Feb 9, 2024

Rebased again. Since this is approved, I am not doing any significant changes here. The Main purpose of this PR was changing the order in the chapter to make this easier for the reader. Content change like suggested in #23 (comment) could be a follow-up then. If there is no plan to merge this reordering, please say so and close this PR.

@axel-h axel-h force-pushed the patch-axel-1 branch 3 times, most recently from 669345e to 1dc567d Compare April 8, 2024 20:53
Show the encoding first so it is easier to find.

Signed-off-by: Axel Heider <[email protected]>
@arichardson arichardson merged commit f58313d into riscv:main Apr 8, 2024
3 checks passed
@axel-h axel-h deleted the patch-axel-1 branch April 8, 2024 22:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants