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

HypMap clean-up #284

Merged
merged 3 commits into from
Mar 10, 2023
Merged

HypMap clean-up #284

merged 3 commits into from
Mar 10, 2023

Conversation

glg-rv
Copy link
Contributor

@glg-rv glg-rv commented Mar 9, 2023

This PR serves both to clean up my conscience of U-mode PRs leftovers and to prepare HypMap to support virtual mapped stack more easily.

Sharing before the stack patch to gather opinions.

patch1: U-mode Shared Region is renamed to U-mode Input Region, as it is not writable from U-mode.
patch2: All layout const definitions are moved to a new module: hyp_layout.
patch3: Private/Shared regions (per-cpu, global mappings) is untenable as it would complicate the types too much. Rename them for what they are: hwmap regions and umode_elf regions.

Copy link
Collaborator

@abrestic-rivos abrestic-rivos left a comment

Choose a reason for hiding this comment

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

just a couple nits; otherwise lgtm.

src/hyp_map.rs Outdated
Comment on lines 258 to 265
/// Generic Id names for each of the U-mode mapping slots.
/// There is no mandated use for each of the slots, and caller can decide to map each of them
/// readable or writable based on the requirement.
#[derive(Copy, Clone)]
pub enum UmodeSlotId {
A,
B,
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this go in hyp_layout.rs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could go. Thought about it for about 30 seconds. SlotIDs and Slot Addrs are a specific of the implementation. But yeah, I'll move it, would make things cleaner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved.

Comment on lines +30 to +31
// Maximum number of hardware map regions.
const MAX_HW_MAP_REGIONS: usize = 32;
Copy link
Collaborator

Choose a reason for hiding this comment

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

these regions are that are identity mapped, right? might be more obvious if this were called the "linear map" or "identity map"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was future-proofing for #180, and in general other possible identity mapped might require a different structure, and this change was exactly to create a region type for each source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haven't changed this. Let me know if you have strong opinions about this.

glg-rv added 3 commits March 10, 2023 16:38
Create a new module, hyp_layout and move all the VM-layout definitions
there.

Signed-off-by: Gianluca Guida <[email protected]>
HypMap region types were originally built to separate the shared (same
across all CPUs) and private (per-CPU allocated) regions of the VA
space.

This definition became obsolete very quickly, and already since the
addition of UmodeInput Region it became clear that it's better to have
regions specific to the kind of memory, rather than their
shared/private nature.

Signed-off-by: Gianluca Guida <[email protected]>
@glg-rv glg-rv force-pushed the topic/hypmap_clean branch from f10ac71 to b88632f Compare March 10, 2023 16:39
@abrestic-rivos abrestic-rivos merged commit 7596c4f into rivosinc:main Mar 10, 2023
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.

2 participants