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.

Sign up for GitHub

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

Jump to bottom

Add hv headers directly in the repo #165

Closed
wants to merge 5 commits into from
Closed

Add hv headers directly in the repo #165

wants to merge 5 commits into from

Conversation

NunoDasNeves
Copy link
Collaborator

Summary of the PR

The hv headers are leaving the kernel uapi directory.
The goal is to only use these for 'passthrough' parts of the interface - i.e. the generic hypercall IOCTL and run vp IOCTL.

@russell-islam
Copy link
Collaborator

Could you pleas elaborate more details of the purpose?

  1. Why we are keeping this here?
  2. Are we keeping another copy in the Kernel too?
  3. How do we sync in case of modifications?
  4. Are these only needed for rust-crates?

@NunoDasNeves
Copy link
Collaborator Author

  1. Why we are keeping this here?

So it doesn't have to live in kernel uapi, with all the baggage that carries.

  1. Are we keeping another copy in the Kernel too?

Yes, although the copies need not contain all the same definitions; the interfaces we use in the kernel are not exactly the same as those we need in userspace.
I will make a pass to delete a lot of the parts we don't need in userspace.

  1. How do we sync in case of modifications?

Ideally these hypervisor interfaces don't need to change - they are backward compatible. Adding new things is easy; just add the definitions.
For some experimental interfaces that could change, we could keep them separated if we really want to.

  1. Are these only needed for rust-crates?

Any userspace library using our interface will need to know how to use the passthru interfaces.

@NunoDasNeves NunoDasNeves force-pushed the nudasnev/add-hv-headers branch from c7a6534 to 9df6597 Compare September 20, 2024 22:46
@russell-islam
Copy link
Collaborator

Could you please look into the clippy error?

@NunoDasNeves NunoDasNeves force-pushed the nudasnev/add-hv-headers branch from 9df6597 to 5a98d60 Compare October 28, 2024 19:08
@NunoDasNeves NunoDasNeves marked this pull request as ready for review October 28, 2024 19:31
@NunoDasNeves
Copy link
Collaborator Author

NunoDasNeves commented Oct 28, 2024 •
edited
Loading

Could you please look into the clippy error?

I guess it is fixed now. The Cloud Hypervisor compilation error is due to some names that changed in the 6.6 version of mshv.h (VFIO_GROUP -> VFIO_FILE)
After this PR we should have a 0.4 release to fix that, and patch CLH, and vfio crate.
@russell-islam @jinankjain @liuw this is ready for review again

@russell-islam
Copy link
Collaborator

LGTM

@russell-islam russell-islam self-requested a review October 29, 2024 20:56
@russell-islam russell-islam enabled auto-merge (rebase) October 29, 2024 23:51
liuw
liuw approved these changes Oct 30, 2024
View reviewed changes
Copy link
Member

@liuw liuw left a comment

Choose a reason for hiding this comment

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

The CI failure seems to be expected.

@NunoDasNeves NunoDasNeves force-pushed the nudasnev/add-hv-headers branch from 6f1bdea to c2b62b9 Compare October 30, 2024 16:49
NunoDasNeves and others added 5 commits October 30, 2024 09:50
@NunoDasNeves
Add hv-headers directory with hypervisor C headers
bf0992c 
The kernel versions of these files will no longer exist in uapi,
because the hypervisor definitions required by userspace are mostly
disjoint from those required by the kernel.

Add files adapted from the hypervisor headers, which contain only the
definitions required by the bindings.

Any additional hypervisor definitions userspace needs can be added
directly to these new files.

Signed-off-by: Nuno Das Neves <[email protected]>
@NunoDasNeves
scripts: Update generate bindings script to use new hv-headers
ae8623b 
Signed-off-by: Nuno Das Neves <[email protected]>
@NunoDasNeves
ioctls: Update MSHV_DEV_VFIO_ names
70a1b42 
MSHV_DEV_VFIO_GROUP-prefixed names have changed to MSHV_DEV_VFIO_FILE
in the newly generated bindings. The values remain the same.

Signed-off-by: Nuno Das Neves <[email protected]>
@NunoDasNeves
Regen x86 bindings from new hv-headers dir.
97e2c17 
Signed-off-by: Nuno Das Neves <[email protected]>
@NunoDasNeves
Regen arm64 bindings from new hv-headers dir
b515b6c 
Signed-off-by: Nuno Das Neves <[email protected]>
@NunoDasNeves NunoDasNeves force-pushed the nudasnev/add-hv-headers branch from c2b62b9 to b515b6c Compare October 30, 2024 16:50
@russell-islam
Copy link
Collaborator

What's wrong with Quality (clippy, rustfmt) ? @NunoDasNeves

@NunoDasNeves
Copy link
Collaborator Author

NunoDasNeves commented Oct 30, 2024 •
edited
Loading

What's wrong with Quality (clippy, rustfmt) ? @NunoDasNeves

I don't know!
Just now I manually ran the quality checks locally to make sure they pass, not sure what's going on...

@NunoDasNeves
Copy link
Collaborator Author

NunoDasNeves commented Oct 30, 2024 •
edited
Loading

What's wrong with Quality (clippy, rustfmt) ? @NunoDasNeves

@russell-islam I found the quality check in "Actions" here: https://github.com/rust-vmm/mshv/actions/runs/11597890716
It seems to have passed... maybe there is some bug with this PR page and it's just not displaying? Very odd...

auto-merge was automatically disabled October 30, 2024 18:12

Pull request was closed

@NunoDasNeves NunoDasNeves mentioned this pull request Oct 30, 2024
Merged
@russell-islam russell-islam deleted the nudasnev/add-hv-headers branch October 30, 2024 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@russell-islam russell-islam russell-islam approved these changes

@jinankjain jinankjain jinankjain approved these changes

@liuw liuw liuw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@NunoDasNeves @russell-islam @liuw @jinankjain