Discrepancy in mhpmevent bits for Sscofpmf

[atishp04]

The ratified sscofpmf extension spec[1][2] had bits 56:57 marked as reserved but the integrated priv spec mentioned only 6 bits.
However, the Smcntrpmf extension CSRs (mcyclecfg, minstretcfg) have 8 bits with bits 56:57 marked as reserved citing sscofpmf.

Is this an integration issue or the definition bits actually changed after ratification ?

[1] https://drive.google.com/file/d/1-3rp7Fw69aH26wbRkvwd4lBHangiVRMN/view
[2] https://github.com/riscvarchive/riscv-count-overflow/blob/main/content.adoc

@gfavor @bcstrongx

[bcstrongx]

I'll leave this one for @gfavor, Smcntrpmf was simply emulating the Sscofpmf spec. Either those bits should be reserved in both mhpmeventX and mcyclecfg/minstretcfg, or in neither.

[gfavor]

I'm not completely certain, but I believe the integrated spec got it right. Bits 57:56 are Reserved in any case. The only difference is between being generally reserved versus reserved for a restricted future usage (and off limits to implementation-specific use). Practically speaking this is a small difference since introduction of any new modes into the architecture is going to affect a lot of things and a lot of CSRs. And what's to say that just two bits in these CSRs will be enough. Plus there has been no thought in anyone's mind (that I'm aware of) about adding new privilege modes to the architecture. In short, there's just too much unknown about, as well as unlikelihood of, addition of new privilege modes into the architecture to be reserving at this time exactly two bits for only this one potential purpose. So, with that, Smcntrpmf should follow suit and not specify anything about these two bits. Greg

…

On Thu, Aug 1, 2024 at 10:55 AM Beeman Strong ***@***.***> wrote: I'll leave this one for @gfavor <https://github.com/gfavor>, Smcntrpmf was simply emulating the Sscofpmf spec. Either those bits should be reserved in both mhpmeventX and mcyclecfg/minstretcfg, or in neither. — Reply to this email directly, view it on GitHub <#1578 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ALLX6GSFARQPC4YIP6Y5E3TZPJZAFAVCNFSM6AAAAABL3BBQO2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENRTGYZTSOJWGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[bcstrongx]

See the linked PR that removes that sentence.

[atishp04]

@gfavor : I could not find anything about "Bits 57:56 are Reserved in any case" in the latest integrated priv spec. Please let me know if I missed something ?

Cc: @avpatel

[bcstrongx]

To take this a step further, I'm not sure how we managed to define 63:58 in Sscofpmf, or how we can ever define any bits in future standard extensions, given the current definition:

The event selector CSRs, mhpmevent3-mhpmevent31, are 64-bit WARL registers that control which event causes the corresponding counter to increment. The meaning of these events is defined by the platform, but event 0 is defined to mean "no event."

I read that as the entire register is reserved for custom use.

[ved-rivos]

I read that as the entire register is reserved for custom use.

My interpretation is that when Sscofpmf is implemented, that is true. When Sscofpmf is implemented the 63:58 are provided by Sscofpmf. So user of mphmevent* has to discover availability of Sscofpmf and interpret 63:58 per Sscofpmf only if the extension is present.

[atishp04]

Yes. That's what software is implemented with that assumption. But the ratified Sscofpmf marked 63:56 while the latest integrated spec 63:58. If the bits 57:56 are marked reserved somewhere else we should point that out in sscofpmf section. Otherwise, an implementation would use those bits for custom event encoding as well.

[gfavor]

At the time of developing Sscofpmf, the view by the ICs was the following:

The hpmevent bits are officially (in the ISA spec) simply WARL without any specific further definition (other than to say that "the meaning of these events is defined by the platform") - which leaves the door open to future extensions further defining and/or constraining values for fields of these registers. Maybe that's arguable, but the more practical observation was that it would be best for Sscofpmf to stay as far away as possible from the lower bits of these registers, i.e. from the bits that have more likely been used by existing implementations.

For RV32 this was guaranteed, but was only statistically very likely true for RV64. Which arguably establishes a little bit of a precedent for introducing definitions of some hpmevent bits.

Practically speaking it is comparably safe to use some number more of the highest bits in RV64, with the odds of a conflict slower growing the lower one goes in these registers.

From this perspective just reserving two high bits for the future would have been very short-sighted if it was felt that that needed to be explicitly done (which wasn't really the case).

So, going forward, using the high byte or two of the hpmevent registers is reasonably fair game for new extensions. And past that slowly gets more dangerous.

There's also the possibility of a future arch extension standardizing a format and structure to the low many bits of these registers. Existing and future implementations would/could not implement this, while other future implementations could choose to implement this if they so chose. For example, standardizing a format that directly mirrors the format that OpenSBI uses in its API. Although OpenSBI already largely accomplishes this in guiding or suggesting (without forcing) implementations to implement a format that matches the OpenSBI API format.

[atishp04]

Thanks for the background @gfavor.
As per the current spec, Shall we consider "Bits 57:56 are Reserved" or allow implementation to reuse those ?

There are the suggestions in the mailing list to allow those bits for implementation as the current integrated spec doesn't say anything about it. So a clear cut definition would be helpful.

[gfavor]

I'll run this by the ARC and get a firm resolution.

[atishp04]

Thanks @gfavor.

[gfavor]

After discussion by the ARC, the resolution is that the integrated spec is correct in not explicitly calling out bits [57:56] as specially reserved for future new priv modes (and correct in not overriding the default for these bits). In general RISC-V arch only has the defined general and unqualified concept of Reserved. Any future definition of any of the upper mhpmeventX bits by a new arch extension is subject to the same considerations, as mentioned in #1578 (comment), that applied to Sscofpmf.

So no change to the current integrated spec and @bcstrongx's corresponding correction to the Smcntrpmf spec stands.

[atishp04]

@gfavor : The ARC notes email says the opposite to what you described here. Am I misunderstanding something ?

"The ratified version of the Sscofpmf specification documents bits 57:56 of mhpmevent as Reserved. The current documentation somehow lost this Reserved status. After a brief discussion, it was concluded that the current documentation should be corrected (i.e. to once again document bits 57:56 as Reserved)."

[bcstrongx]

I read it as 57:56 are 0, which means they are reserved for any future standard use, not just for future priv modes. As opposed to 55:0, which are custom.

[gfavor]

The ARC minutes didn't capture the stuation accurately. The issue was that the original spec reserved bit 57:56 for a specific potential (but unlikely) purpose, versus just generally reserving them. The ARC judged that to be improper and inconsistent with the RV arch at large - and hence that the integrated spec was correct and in line with standard practice within the overall architecture. The writer of the minutes also didn't quite capture properly (and this mistake was missed) that the spec should simply not do what the original spec did, i.e. the curent spec should remain silent regarding these bits. Which means any future standard (overriding) definition of these bits (or any lower bits) faces the same considerations that applied for Sscofpmf providing an overriding definition of bits 63:58.

[gfavor]

Also note that the mhpmeventN bits are NOT "custom" - otherwise it would not have been allowable for RVI to redefine the upper bits (or any other bits in the future) of these registers. They are WARL.

[atishp04]

Thanks again @gfavor for the clarification.

[bcstrongx]

Wait, now I'm confused. So are bits 57:56 0 or WARL?

[gfavor]

Setting aside what the original spec "inappropriately" said, the default definition of all 64 bits of the mhpmevent registers is WARL and not Reserved. So even Sscofpmf was either defining or overriding/redefining (depending on one's perspective) the pre-existing definition of the high register bits. Whether one "officially" reserves certain bits or not doesn't really make a difference. Whenever a new extension comes along that defines/redefines any bits of these registers, the same considerations will apply that applied to Sscofpmf when it was being developed. (Put differently, the original "reserved for future priv modes" was both anachronistic wrt the general architecture and reserving of bits, and was reserving bits for a highly unlikely and hence flawed purpose. So then the question was whether the current integrated spec represented the more appropriate handling of bits 57:56 or not.)

[bcstrongx]

Sorry, still not sure of the answer. I understand the history, and that Sscofpmf redefined 63:58. I'm less sure where we landed on 57:56. Are they now 0 (reserved) or WARL?

[gfavor]

By the integrated spec continuing to say nothing about these bits, the pre-existing definition of WARL applies for these bits.

[bcstrongx]

Got it, thanks!

[gfavor]

Sorry for the churn, but the ARC discussed this issue some more and came to a final slightly different conclusion (wrt the last ARC minutes about this issue and more so different wrt what I described above). As a result, some small changes will need to be made by @wmat to the integrated Sscofpmf spec and by @bcstrongx to the Smcntrpmf spec.

First, for context for Bill, the original ratified unintegrated Sscofpmf spec defined bits [57:56] of the mhpmeventN CSRs as "Reserved for future modes". And the integrated Sscofpmf spec completely lost any definition of these two bits.

The final resolution is that the explicit definition of bits [57:56] as Reserved should be added back in, BUT without the addiitonal words about "for future modes", i.e. these bits should simply be defined as "Reserved". Further, in the register diagram these two bits should be shown as being WPRI. This correction is for Bill to make.

The correction for Beeman to make to the Smcntrpmf spec is to mirror the preceding resolution.

[bcstrongx]

The Smcntrpmf github has been archived, since the extension has been integrated into the priv spec. So it probaby makes sense for @wmat to duplicate his Sscofpmf changes in the Smcntrpmf chapter. But let me know if for some reason you need me to do it.

[bcstrongx]

Just curious, why WPRI rather than 0?

[gfavor]

WPRI foremost is a directive to software (although also a directive to hardware). Here's the spec definition of WPRI:

Some whole read/write fields are reserved for future use. Software should ignore the values read from these
fields, and should preserve the values held in these fields when writing values to other fields of the same
register. For forward compatibility, implementations that do not furnish these fields must make them
read-only zero.

[wmat]

So the old version of the spec is significantly different. There's a whole section called Machine Level Additions that doesn't exist in the integrated chapter. Can someone please let me know what exactly from the old version should be added to the integrated chapter and where? Here's the old content.adoc:

https://github.com/riscvarchive/riscv-count-overflow/blob/main/content.adoc

[gfavor]

@wmat The original spec for Sscofpmf defined bits [63:56] of the mhpmevent registers (CSRs). The integrated version of the spec "lost" the definitions of bits [57:56], i.e. the integrated spec says nothing about these two mhpmevent bits.

The original spec defined bits [57:56] as "0 - Reserved for possible future modes". In the integrated spec a definition of these two bits should be added back in. In the register figure and associated table, these two bits should each be marked as "WPRI" and as Reserved, i.e. Field is "WPRI" and Description is "Reserved".

Per Beeman's post, the same should be done in the Smcntrpmf chapter for the mhpmevent register descriptions there.

[gfavor]

Closing this issue now that @wmat has created a PR for updating both extensions to reflect the final ARC resolution as described in the last post.