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

spec: Add XTheadMaee #42

Merged
merged 1 commit into from
Feb 4, 2024
Merged

spec: Add XTheadMaee #42

merged 1 commit into from
Feb 4, 2024

Conversation

romanheros
Copy link
Contributor

We name the mxstatus maee to a specific extension XTheadMaee, which describles whether enable the the extend page attributes in PTE or not.

@CLAassistant
Copy link

CLAassistant commented Jan 31, 2024

CLA assistant check
All committers have signed the CLA.

xtheadmaee.adoc Outdated
[NOTE,caption=Frozen]
The `XTheadMaee` extension is `stable`.

The `XTheadMaee` ISA extension provides a way to configure page attributes of addresses when virtual addresses are translated into physical addresses by extended page attributesin page table entries (PTEs).
Copy link
Contributor

Choose a reason for hiding this comment

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

attributesin -> attributes in

== Extend MMU address attribute

[NOTE,caption=Frozen]
The `XTheadMaee` extension is `stable`.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably clarify that XTheadMaee is a non-standard non-conforming extension.
The PTE bits that are used by MAEE are reserved for standard usage.
As far as I understood, this was caused by the fact that RVI did not have a standard yet and T-Head needed a solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should probably clarify that XTheadMaee is a non-standard non-conforming extension. The PTE bits that are used by MAEE are reserved for standard usage. As far as I understood, this was caused by the fact that RVI did not have a standard yet and T-Head needed a solution.
Yes, when c906 came to the world, RVI didn't have the standard with similar function. But now RVI has ratify the SVPBMT, which has the similar function with XTheadMaee. c908 supports both XTheadMaee and SVPBMT. And c907 and other newer CPUs only support SVPBMT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The XTheadMaee extension is not a custom-extension, but a non-standard non-conforming
extension. TheadMaee uses preserved [60:63] bits of PTE, which have been allocted to SVPBMT and SVNAPOT.
This conflict is not on purpose, but the result of issues in the past that have been resolved since. Therefore, the XTheadMaee extension and the SVPBMT or SVNAPOT extension are in conflict. In other words, tools should not allow to enable the TheadMaee extension and the SVPBMT or SVNAPOT at the same time and should report an error if both are enabled by the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think XTheadMaee is an custom extension, as it is controlled by an custom CSR. And actually, it can live with SVPBMT in the same hardware(such in C908). It just can't be enabled simultaneously with SVPBMT.

Copy link
Contributor

Choose a reason for hiding this comment

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

By definition, custom extensions can always be enabled with RVI extensions simultaneously. Also, the bits used by MAEE were marked as reserved in older RVI priv specifications that don't include Svpbmt. So, there is not much doubt that the XTheadMaee extension is non-conforming.

The issue was that PTEs don't have custom bits, and therefore, page attributes could not be set until Svpbmt was ratified (long after the C906 was in production).

But there is no point in reasoning why this happened and what could have been done differently since we are not here to blame someone for something, or to justify decisions that were made long ago. We just document technical details, so that others can understand the technical properties.

xtheadmaee.adoc Outdated
| T | 60 | Trustable
|===

XTheadMaee depends on the value of the MAEE field in extended register MXSTATUS. If the MAEE field is 1, page attributes of addresses are determined by extended page attributes in corresponding PTEs. If the MAEE field is 0, page attributes of addresses are determined by the sysmap.h file.
Copy link
Contributor

Choose a reason for hiding this comment

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

It is not defined (or referenced) what MXSTATUS is (it is a custom CSR with a certain number).
Also, we should probably give that CSR a prefix (e.g. TH_) like we do with instruction mnemonics to prevent name collisions with standard CSR names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK. Yes, we can name it to TH_MXSTATUS. It's number is 0x7C0.

|===

XTheadMaee depends on the value of the MAEE field in extended register MXSTATUS. If the MAEE field is 1, page attributes of addresses are determined by extended page attributes in corresponding PTEs. If the MAEE field is 0, page attributes of addresses are determined by the sysmap.h file.

Copy link
Contributor

Choose a reason for hiding this comment

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

How can we find out if XTheadMaee is available on a system (how can the kernel find out if XTheadMaee is present or not).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Usually it is the uboot spl or the zero stage boot, which run before the OpenSBI and the kernel, do the enable work by write 1 to TH_MXSTATUS MAEE field. Util now, the Linux kernel directly use the XTheadMaee. It doesn't detect. I think we should do it in the OpenSBI and export an interface to Linux kernel if we don't have a supervisor register.

Copy link
Contributor

Choose a reason for hiding this comment

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

Linux does not unconditionally write or read a bit in a custom CSR.
Instead, it needs to detect first that the kernel is running on a core that implements MAEE.
And that detection mechanism should be documented here (so the code can be reviewed against the specification).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can export the XTheadMaee through DTB. But util now, Linux can't read or write TH_MXSTATUS. It is not exposed in menvcfg like SVPBMT.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If MVENDORID is 0x5B7, OS kernel should read TH_MXSTATUS MAEE as the enable status of XTheadMaee. Is this OK?

Copy link
Contributor

Choose a reason for hiding this comment

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

The current kernel implementation can be found here:

The following requirements are checked:

  • static test: CONFIG_ERRATA_THEAD (riscv_fill_cpu_mfr_info)
  • vendor_id == THEAD_VENDOR_ID (riscv_fill_cpu_mfr_info)
  • static test: CONFIG_ERRATA_THEAD_PBMT (errata_probe_pbmt)
  • arch_id == 0 (errata_probe_pbmt)
  • impid == 0 (errata_probe_pbmt)

If I understand you correct, then you state that this is not sufficient and future proof.
You would also like to see a test for a set MAEE bit in TH_MXSTATUS (in errata_probe_pbmt).
This looks fine to me.

But now we have the question of the requirements for accessing TH_MXSTATUS.MAEE.
Is this CSR available for all cores with (vendor_id == THEAD_VENDOR_ID) && (arch_id == 0) && (impid == 0) (follow-up question: what about future cores?). If this test is sufficient, then we are done. If not, we need to clarify what is needed.

Copy link
Contributor Author

@romanheros romanheros Feb 2, 2024

Choose a reason for hiding this comment

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

This CSR available for all cores in Xuantie CPU. One shortcoming using TH_MXSTATUS.MAEE is that it can't be read from supervisor mode.If you don't think it is a problem, I am happy to add TH_MXSTATUS.MAEE as XTheadMaee proof. @cmuellner

@Cooper-Qu Cooper-Qu merged commit da13ff3 into XUANTIE-RV:master Feb 4, 2024
3 checks passed
@cmuellner
Copy link
Contributor

I don't like so much the fact, that we introduce th.mxstatus here (I would prefer if we just introduce the MAEE bit). But I don't have a good proposal at the moment on how to structure this better. We could add another specification for th.mxstatus (and probably others) full of reserved bits and define only the MAEE bit here. This would clearly scale better, but since we only have one consumer specified yet, it is pointless at this time.

The important details (how to probe a CPU for the feature) are here, so let's keep it as it is for now.

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