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

monorepo: Merge kvm-bindings into kvm-ioctls #292

Merged
merged 120 commits into from
Nov 6, 2024

Conversation

TimePrinciple
Copy link
Contributor

Summary of the PR

Fixes #291

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

andreeaflorescu and others added 30 commits January 17, 2019 12:22
The CI script is currently running the release build with the default
features and with kvm-v4_14_0 and kvm-v4_20_0 features. Similarly
cargo test is also run with the available features

Signed-off-by: Andreea Florescu <[email protected]>
This crate should work with older rust as well.

Fixes: rust-vmm/kvm-bindings#1

Signed-off-by: Andreea Florescu <[email protected]>
Signed-off-by: Andreea Florescu <[email protected]>
The PullAssigner bot [1] will pick 2 github handles from the gate
keepers list [2] and assign them for reviewing any new PR.
The bot goes uses a round robin algorithm to select handles from the
list.

What happens when the CODEOWNERS file gets populated with real
owners? Following the PR review and approval process [3], the
PullAssigner bot will either pick only one or zero reviewers from the
gate keepers list, if the CODEOWNERS file points to exactly one or more
than one actual ownwer, respectively.

[1] https://pullpanda.com/assigner
[2] https://github.com/rust-vmm/community/blob/master/GATEKEEPERS.md
[3] https://github.com/rust-vmm/community#pr-review-and-approval

Signed-off-by: Samuel Ortiz <[email protected]>
Since kvm_cpuid2 contains a flexible array member, we can implement
CpuId as a FamStructWrapper<kvm_cpuid2>. This allows users of
kvm-ioctls to work with safe code even when dealing with kvm_cpuid2.

Signed-off-by: Adrian Catangiu <[email protected]>
Since kvm_msrs contains a flexible array member, we can implement
Msrs as a FamStructWrapper<kvm_msrs>. This allows users of kvm-ioctls
to work with safe code even when dealing with kvm_msrs structures.

Signed-off-by: Adrian Catangiu <[email protected]>
Since kvm_msr_list contains a flexible array member, we can implement
MsrList as a FamStructWrapper<kvm_msr_list>. This allows users of
kvm-ioctls to work with safe code even when dealing with kvm_msr_list.

Signed-off-by: Adrian Catangiu <[email protected]>
The FamStructWrappers definitions as well as the vmm-sys-util
dependency are now gated by an opt-in `fam-wrappers` feature.

Signed-off-by: Adrian Catangiu <[email protected]>
Also add Cargo.lock to .gitignore

Signed-off-by: Adrian Catangiu <[email protected]>
Signed-off-by: Adrian Catangiu <[email protected]>
There was newly added functionality which warants a version number
`minor` bump, not just a version number `patch` bump.

Signed-off-by: Adrian Catangiu <[email protected]>
Use Buildkite and rust-vmm-ci for testing.

Signed-off-by: Andreea Florescu <[email protected]>
Import bindings from x86::bindings so we don't need to have the
same logic to exclude features twice (once in the x86/mod.rs and
once in x86/fam_wrappers.rs).

Signed-off-by: Andreea Florescu <[email protected]>
When the crate is built with --all-features we have a bunch of errors
because we are not excluding the 4.14 and 4.20 bindings. Whenever
all features is specified now, the 4.20 bindings are going to be
used. This change is required for switching to rust-vmm-ci where
the tests are run using --all-features flag.

Signed-off-by: Andreea Florescu <[email protected]>
Autogenerated code throws lots of clippy errors. Disable the
warnings as we don't want to manually edit autogenerated code.

Signed-off-by: Andreea Florescu <[email protected]>
The linker was unable to find __addtf3, __multf3 and __subtf3.

Added target-feature=+crt-static and link-arg=-lgcc as a temporary
workaround. This seems to be the accepted fix in the Rust community:
rust-lang/compiler-builtins#201

A permanent fix is yet to be implemented in the Rust compiler.

Signed-off-by: Andreea Florescu <[email protected]>
We have a few combination of features that were not tested with the CI
due to switching to rust-vmm-ci and adding the fam-wrappers feature.

Added a new Buildkite pipeline for the missing tests.

Signed-off-by: Andreea Florescu <[email protected]>
We are now using rust-vmm-ci for testing.

Signed-off-by: Andreea Florescu <[email protected]>
In order to be able to save/restore vcpu registers
on arm, we need to support the KVM_REG_LIST
ioctl. However, the structure holding the registers
list has a flexible array member.
This commit add a `FamStructWrapper` over the `kvm_reg_list`
structure.

Fixes: rust-vmm/kvm-bindings#20.

Signed-off-by: Diana Popa <[email protected]>
Enable build with the v4.14 bindings and `fam-wrappers`
feature on arm.

Signed-off-by: Diana Popa <[email protected]>
The CHANGELOG was also updated with newly
added functionality.

Signed-off-by: Diana Popa <[email protected]>
This update brings the following commits:

e58ea74 Fix kcov_ouput_dir typo in test_coverage.py
d62d781 fix buildkite typos in readme
0fc8ced refactor test_benchmark.py
741b894 checkout to PR branch before finishing test_bench
645a5c3 test_bench: don't crash when no bench on master
bd32544 Fetch origin in benchmark test
35beb91 Fix commit message test
53427aa benchmarks: add test that can run at every PR
abd2c90 Add test for commit message format
fe859f4 Update container image to v6
75d7254 run cargo check on all features
7e3f307 skip coverage-arm test
cd7096e Enable rust-vmm coverage test in CI

Signed-off-by: Alexandru Agache <[email protected]>
The path and threshold have been updated due to changes in the CI.
Added a coverage config file for aarch64 as well (coverage testing
on aarch64 is disabled for now because of tooling issues).

Signed-off-by: Alexandru Agache <[email protected]>
Mentioned in README about potential undefined behavior with
incompatible versions.

Signed-off-by: Alexandru Agache <[email protected]>
This release is to allow kvm-ioctls to be released with an increased
dependency on kvm-bindings rather than pinning to an older version.
Update the vmm-sys-utils version to reflect the latest version and to
match the one from kvm-ioctls.

Signed-off-by: Rob Bradford <[email protected]>
Bumps [rust-vmm-ci](https://github.com/rust-vmm/rust-vmm-ci)
from `e58ea74` to `24d66cd`.

Signed-off-by: dependabot-preview[bot] <[email protected]>
@rbradford
Copy link
Contributor

@rbradford are you okay with this approach as well? e.g. merging kvm-bindings into the kvm-ioctls repository, and renaming this repo to just "kvm", instead of moving both ioctls and bindings to a new repository (the idea being that this way, we preserve stars and other properties of this repository)

Sounds good to me!

roypat
roypat previously approved these changes Nov 5, 2024
Copy link
Collaborator

@roypat roypat left a comment

Choose a reason for hiding this comment

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

I've set the DCO check to "pass" because it's failing on ages old commits from kvm-bindings that look fine to me

@andreeaflorescu
Copy link
Member

Typo on commit 1e72627: "Prepare for kvm-bindings to be merged, prevent conflicks from happening.".

Here is a suggestion for rephrasing your commit message to make it more explicit:

monorepo: Move kvm-ioctls related files into kvm-ioctls folder

This change is needed to be able to merge the kvm-bindings crate into this repository
and prevent conflicts from happening.

.cargo/config Outdated Show resolved Hide resolved
kvm-ioctls/coverage_config_aarch64.json Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
TimePrinciple and others added 2 commits November 5, 2024 19:37
This change is needed to be able to merge the kvm-bindings crate into
this repository and prevent conflicts from happening.

Signed-off-by: Ruoqing He <[email protected]>
As discussed in #291, we are merging `kvm-bindings` into `kvm-ioctls`
and renaming `kvm-ioctls` to `kvm`.

This merges `kvm-bindings` into this workspace with its Git history
intact, through command:

```sh
git remote add bindings https://github.com/rust-vmm/kvm-bindings.git
git fetch bindings
git merge --allow-unrelated-histories bindings/main
```

Co-authored-by: Manos Pitsidianakis <[email protected]>
Signed-off-by: Manos Pitsidianakis <[email protected]>
Signed-off-by: Ruoqing He <[email protected]>
Reorganize files from `kvm-bindings` and `kvm-ioctls`, and layout them
like a monorepo.

Signed-off-by: Ruoqing He <[email protected]>
TimePrinciple and others added 3 commits November 5, 2024 21:00
Create Rust workspace to incorporate `kvm-bindings` and `kvm-ioctls` as
members of this workspace.

Signed-off-by: Ruoqing He <[email protected]>
Bumps [rust-vmm-ci](https://github.com/rust-vmm/rust-vmm-ci) from `cdb4a2d` to `1150c47`.
- [Commits](rust-vmm/rust-vmm-ci@cdb4a2d...1150c47)

---
updated-dependencies:
- dependency-name: rust-vmm-ci
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
After merging `kvm-bindings` and `kvm-ioctls`, the code coverage has
changed, update coverage of `coverage_config_x86_64.json` to 91.46 as
suggested by coverage test.

Signed-off-by: Ruoqing He <[email protected]>
@andreeaflorescu
Copy link
Member

You're missing the top level README.md file. Are you planning to add it in a future PR?

@TimePrinciple
Copy link
Contributor Author

TimePrinciple commented Nov 5, 2024

You're missing the top level README.md file. Are you planning to add it in a future PR?

Yes, @roypat picked that up and I think it could be better to let him continue his work :)

@roypat
Copy link
Collaborator

roypat commented Nov 6, 2024

Similarly to rust-vmm/vhost#114 (comment), this PR cannot be merged with a rebase. I've also temporarily allowed merge commits to merge this

@roypat roypat merged commit 5c0a837 into rust-vmm:main Nov 6, 2024
23 checks passed
@roypat
Copy link
Collaborator

roypat commented Nov 6, 2024

I've

  • updated the buildkite steps to include the custom-tests
  • Updated the repository name
  • Updated the buildkite pipeline name
  • Updated the buildkite pipeline to fetch from the renamed repository

@roypat
Copy link
Collaborator

roypat commented Nov 6, 2024

Also updated branch protection rules to point towards the new pipeline name

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.

Merging kvm-ioctls and kvm-bindings GitHub repositories