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

fork() types #72

Open
npmccallum opened this issue May 3, 2019 · 20 comments
Open

fork() types #72

npmccallum opened this issue May 3, 2019 · 20 comments

Comments

@npmccallum
Copy link
Member

I'm working on a KVM library and I find myself reimplementing most of the types from this crate. I can't use this crate, however, because of your (legitimate) need for asm!(). Would this project consider merging a patch that splits this crate into two halves: one containing raw types (such as RFlags) and the other containing implementations for things like reading and writing the registers?

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

Is only asm!() the problem or all unstable features? Maybe we can support your use case through (opt-out) cargo features? I'm also open to splitting the crate, depending on the organizational effort (the crate should stay maintainable).

@npmccallum
Copy link
Member Author

It is all unstable features.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

Ok, then a nightly/stable split might make more sense than a raw type/accessing registers split. For example, the InterruptDesriptorTable struct uses the unstable x86-interrupt ABI.

To implement this split, we could add a cargo feature named nightly that is enabled by default. Then we could gate all uses of unstable features on this nightly feature. This way we can compile a part of the library on stable by disabling the feature.

@npmccallum
Copy link
Member Author

I'm still not sure this really accomplishes what I would want. The problem is that this crate is a mix of two things: x86_64 types and x86_64 instructions. In the case of a hypervisor, we only need the types. Likewise, the instructions depend on the types. But I don't really want the hypervisor to depend on the instructions.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

Good point. I guess splitting off a "x86_64_types" crate might be the best solution then.

Which types do you propose to move out?

@npmccallum
Copy link
Member Author

npmccallum commented May 3, 2019

Good question. Definitely *Flags.

I'd also like all of the tables. But that brings up an interesting point: the contents of your table structs are private. That would probably have to change.

I also like the *Addr types. We already have a parallel to this. I wonder if we can merge them. In particular, we have implementations that allow things like Deref and lifetime/mutability safety. So maybe we could collaborate on an improved approach here.

@npmccallum
Copy link
Member Author

*Addr brings up another interesting point because they may not be architecture specific. Or rather, they could be generalized for multiple architectures.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

Sounds good!

I would be more than happy to collaborate with you! How about we start by creating a x86_64_types repository under the rust-osdev organization where we both have push access?

@npmccallum
Copy link
Member Author

Yeah, that sounds great. We can start by just moving existing types and go from there.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

*Addr brings up another interesting point because they may not be architecture specific. Or rather, they could be generalized for multiple architectures.

In principle yes, but I would prefer to introduce a new type for that. The reason is that we use *Addr in some x86_64 specific types that wouldn't make sense with a host specific address type.

Yeah, that sounds great. We can start by just moving existing types and go from there.

Great! I created the repository at https://github.com/rust-osdev/x86_64_types. I invited you to the organization and gave you admin access to the repo. I also published a initial version to crates.io to claim the name. You should have publish access too.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

I put in a MIT/Apache2 license and created some branch protection rules for master (require pull requests and at least 1 approving review), I hope that's fine with you.

@npmccallum
Copy link
Member Author

License is perfect. I'm going to start filing pull requests for moving the types.

@phil-opp
Copy link
Member

phil-opp commented May 3, 2019

Great!

@npmccallum
Copy link
Member Author

How stabilized is this crate currently? Here's a thought...

Once we move out all the register types, it might make sense to implement read/write of those registers as a trait. We could probably even macroize it.

Also, for RFlags, it might be nice to implement some helper methods for getting/settings the iopl.

@phil-opp
Copy link
Member

phil-opp commented May 4, 2019

We're still in the 0.x.y version range, so we can always release a new breaking version. However, I still want to avoid breaking changes when possible.

Adding a trait sounds good in principle. I'm not sure what macros you have in mind, but I'm always a bit reluctant to adding user-facing macros because they are new thing that users of the library have to learn.

Also, for RFlags, it might be nice to implement some helper methods for getting/settings the iopl.

Sure, adding functionality is fine with me. I didn't looked into the details of RFLAGS right now, but if it works it should be fine to add.

@phil-opp
Copy link
Member

phil-opp commented Jun 3, 2019

Cross-linking pull requests #73 and #74, which contain an initial implementation.

@npmccallum What's the state of this? Are you still interested is these changes?

@npmccallum
Copy link
Member Author

npmccallum commented Jun 3, 2019

Yes, I'm still interested. The state is just me being too busy. Let me resubmit against the dev branch today.

@phil-opp
Copy link
Member

phil-opp commented Jun 3, 2019

Ok perfect, thanks! No need to hurry!

@haraldh
Copy link
Contributor

haraldh commented Nov 7, 2019

Any progress here @npmccallum ? 😉

@npmccallum
Copy link
Member Author

@haraldh Typing on it as we speak!

phil-opp added a commit that referenced this issue Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants