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

Add User Mode registers #119

Merged
merged 3 commits into from
Feb 10, 2020
Merged

Add User Mode registers #119

merged 3 commits into from
Feb 10, 2020

Conversation

vinaychandra
Copy link
Contributor

No description provided.

Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the pull request! I left a few comments, but otherwise this looks good to me. Thanks especially for adding extensive documentation for everything!

Comment on lines 240 to 241
SegmentSelector((raw.0 + 16 - 3).try_into().unwrap()),
SegmentSelector((raw.0 + 8 - 3).try_into().unwrap()),
Copy link
Member

Choose a reason for hiding this comment

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

Could you clarify why you're doing - 3 here?

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 values in the registers are or'ed with 0b11 for sysret registers regardless of what the user specified. We are subtracting that back so that the user can directly match with their gdt's user mode entry rather

Copy link
Member

Choose a reason for hiding this comment

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

The problem I see is that the user might also pass a segment selector with the requested privilege bits already set to 0b11. In this case, it would be very strange to get a selector with requested privilege level 0 back, i.e. not the same that was written.

src/registers/model_specific.rs Outdated Show resolved Hide resolved
src/registers/model_specific.rs Outdated Show resolved Hide resolved
src/registers/model_specific.rs Outdated Show resolved Hide resolved
src/registers/model_specific.rs Outdated Show resolved Hide resolved
return Err("Syscall CS and SS is not offset by 8.");
}

Self::write_raw(((ss_sysret.0 - 8) | 0b11).into(), cs_syscall.0.into());
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if automatically setting the RPL to 0b11 is the right solution. The alternative would be to require the caller to properly set these bits. What do you think is the better 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.

The amd64 documentation says it is required but will not be tested. That is why i implicitly left it like this. This also provides direct one to one mapping to the user segment functions from gdt. Also the user can use write_raw to test it otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

This also provides direct one to one mapping to the user segment functions from gdt.

The user segment functions only return a GDT descriptor, not a segment selector. You mean the add_entry function? If yes, there is currently an open issue about whether we should hardcode the requested privilege level in that function: #120

@vinaychandra
Copy link
Contributor Author

@phil-opp, updated as per comments

@phil-opp
Copy link
Member

Thanks for the updates! I'm still not sure whether automatically adjusting the requested privilege level is a good idea (see the inline comments). Maybe @haraldh, who opened the related #120, has an opinion on this matter?

@haraldh
Copy link
Contributor

haraldh commented Jan 27, 2020

Yeah, I think we should fix #120 and then assert for the bits, if set/not set at the corresponding places. Or maybe even better and idiomatic, have different SegmentSelector types according to the privilege bits

@vinaychandra
Copy link
Contributor Author

Another option is to remove the read altogether and just go with read raw for now. What do you think @phil-opp?

@phil-opp
Copy link
Member

phil-opp commented Feb 4, 2020

While we could merge this with only read_raw/write_raw (i.e. without read/write), I think a better solution would be the following:

  • Add a simple set_rpl(&mut self, rpl: PrivilegeLevel) method to the SegmentSelector type
  • Remove the - 3 operation from read.
  • Remove the | 0b11 operation from write. Instead, add an assert_eq!(ss_sysret.rpl(), PrivilegeLevel::Ring3) and mention this requirement in the function documentation.

What do you think about this, @vinaychandra?

@vinaychandra
Copy link
Contributor Author

Makes sense, I updated the PR accordingly

@phil-opp
Copy link
Member

Thanks a lot!

@phil-opp phil-opp merged commit 4ebcf1e into rust-osdev:master Feb 10, 2020
phil-opp added a commit that referenced this pull request Feb 10, 2020
@phil-opp
Copy link
Member

Published together with #118 as version 0.9.0.

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.

None yet

3 participants