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

build: Bump vmm-sys-util crate and its consumers #6134

Merged

Conversation

likebreath
Copy link
Member

@likebreath likebreath commented Jan 23, 2024

This patch bumps the following crates, including [email protected]*,
[email protected]**, [email protected], [email protected],
[email protected]***, [email protected],
[email protected], [email protected], [email protected],
[email protected], and the latest of vfio-bindings, vfio-ioctls,
mshv-bindings,mshv-ioctls, and vfio-user.

* A fork of the kvm-bindings crate is being used to support
serialization of various structs for migration [1]. Also, code changes
are made to accommodate the updated struct xsave from the Linux
kernel. Note: these changes related to struct xsave break
live-upgrade.

** The new kvm-ioctls crate introduced breaking changes for
the get/set_one_reg API on aarch64 [2], so code changes are made to
the new APIs.

*** A fork of the versionize_derive crate is being used to support
versionize on packed structs [3].

[1] https://github.com/cloud-hypervisor/kvm-bindings/tree/ch-v0.7.0
[2] rust-vmm/kvm#223
[3] https://github.com/cloud-hypervisor/versionize_derive/tree/ch-0.1.6

Fixes: #6072

Signed-off-by: Bo Chen [email protected]

@rbradford
Copy link
Member

Thank you for tackling this!

@rbradford
Copy link
Member

rbradford commented Jan 23, 2024

[1] https://github.com/cloud-hypervisor/kvm-bindings/tree/ch-v0.7.0-tdx-kernel-v5.19

Why did you go back from v6.3 to v5.19? I think we should try and at least match upstream kvm-bindings which is v6.2.

@likebreath
Copy link
Member Author

likebreath commented Jan 23, 2024

Thank you for tackling this!

Of course. The fuzzer issue is fixed on my end and I will need to look into issues from aarch64.

[1] https://github.com/cloud-hypervisor/kvm-bindings/tree/ch-v0.7.0-tdx-kernel-v5.19

Why did you go back from v6.3 to v5.19?

This is to keep the TDX support in the upstream Cloud Hypervisor that only works with this version of Linux kernel. This is the same as what we are doing right now with https://github.com/cloud-hypervisor/kvm-bindings/commits/ch-v0.6.0-tdx/ whose bindings are based on v5.12 while the [email protected] are based on v5.13.

I am working on enabling TDX for Cloud Hypervisor with a newer version of Linux kernel based on Canonical's Technical Preview stack [1][2]. Once this is done (estimated end of this quarter), we can move the Linux kernel to v6.3+. Also, we are trying to make TDX stack available as a part of Canonical's upcoming LTS release, e.g. Ubuntu 24.04, which we are looking at H2 this year.

In the meantime, this is the best solution that I came up with.

[1] https://ubuntu.com/blog/intel-tdx-1-0-preview-on-ubuntu-23-10
[2] https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/mantic/log/?h=intel-opt-next

@likebreath
Copy link
Member Author

Also, changes are being made to accommodate the changes
on the struct xsave from the Linux kernel.

@rbradford One more thing to mention. This causes test failures on live-upgrade. This is expected. Please let me know your thoughts.

@rbradford
Copy link
Member

I've looked through where we import symbols from kvm_bindings and I can't find any that are TDX specific. Are you sure we need to have modified bindings for TDX? Can you try without and if a constant or struct is missing just copy it in.

@likebreath
Copy link
Member Author

I've looked through where we import symbols from kvm_bindings and I can't find any that are TDX specific. Are you sure we need to have modified bindings for TDX? Can you try without and if a constant or struct is missing just copy it in.

@rbradford Here is one example [1]. The return of the KVM_RUN ioctl (e.g. struct kvm_run) was extended to have specific information about VM exit caused by TDX hyper-calls. We can't "copy it in" to Cloud Hypervisor code base, as API functions from kvm-ioctl are also using struct kvm_run extensively. I may manually instrument kvm-bindings crate for this bits, but I trust bindgen more than manual instrument from myself.

What's your concern of using a kvm-bindings based on v5.19 for now? From the x86 CI runner, it looks like it does not bring any functional differences.

[1]

let tdx_vmcall = unsafe { &mut kvm_run.__bindgen_anon_1.tdx.u.vmcall };

@likebreath likebreath force-pushed the 0117/bump_rust_vmm_crates branch 2 times, most recently from 114732d to 056030f Compare January 24, 2024 00:59
@rbradford
Copy link
Member

I've looked through where we import symbols from kvm_bindings and I can't find any that are TDX specific. Are you sure we need to have modified bindings for TDX? Can you try without and if a constant or struct is missing just copy it in.

@rbradford Here is one example [1]. The return of the KVM_RUN ioctl (e.g. struct kvm_run) was extended to have specific information about VM exit caused by TDX hyper-calls. We can't "copy it in" to Cloud Hypervisor code base, as API functions from kvm-ioctl are also using struct kvm_run extensively. I may manually instrument kvm-bindings crate for this bits, but I trust bindgen more than manual instrument from myself.

What's your concern of using a kvm-bindings based on v5.19 for now? From the x86 CI runner, it looks like it does not bring any functional differences.

[1]

let tdx_vmcall = unsafe { &mut kvm_run.__bindgen_anon_1.tdx.u.vmcall };

This is the only code that requires the bindings - for consistency with the rest of the TDX code I think it would be better to carry our own versions. (See #6137)

My motivation for not using the modified bindings are:

  • Ease with upgrading to new kernel versions (e.g. to support new architectures, new functionality)
  • Easier crate upgrades
  • Consistency with the rest of the TDX support - with the exception of the vmcall struct everything else is done using CH "owned" structures and constants.

And I think most importantly:

  • Detaches the TDX kernel ABI from the version of kvm-bindings API - e.g. if we wanted to support newer or kernel ABI than the kernel API (this has been necessary in the past.)

@likebreath
Copy link
Member Author

This is the only code that requires the bindings - for consistency with the rest of the TDX code I think it would be better to carry our own versions. (See #6137)

My motivation for not using the modified bindings are:

  • Ease with upgrading to new kernel versions (e.g. to support new architectures, new functionality)
  • Easier crate upgrades
  • Consistency with the rest of the TDX support - with the exception of the vmcall struct everything else is done using CH "owned" structures and constants.

And I think most importantly:

  • Detaches the TDX kernel ABI from the version of kvm-bindings API - e.g. if we wanted to support newer or kernel ABI than the kernel API (this has been necessary in the past.)

Okay. I thought the practice for our TDX code is directly using data structures and constants from our kvm-bindings that specifically comes with TDX patches. Looks like it is a misunderstanding.

Thanks for sending the PR. The code looks good to me. Just merged it. I believe this "copy-in" approach (e.g. carry our own versions of new data structures and constants) would work as long as we are only using constants, new data structure for "setters", and new data structure that is extended as a union member of the existing data structure for "getters" (e.g. the case for vmcall struct).

I agree detaching TDX kernel ABIs is a better way to go and good to see most of our code base is already doing that. I went through my patches for supporting newer version of TDX APIs along with new KVM APIs for guest memory support (expected to land at v6.8) [1]. I believe the "copy-in" approach would work, though some re-work is needed.

[1] torvalds/linux@6c370dc

@likebreath likebreath force-pushed the 0117/bump_rust_vmm_crates branch 2 times, most recently from 55cf734 to 2b0892c Compare January 24, 2024 19:26
This patch bumps the following crates, including `[email protected]`*,
`[email protected]`**, `[email protected]`, `[email protected]`,
`[email protected]`***, `[email protected]`,
`[email protected]`, `[email protected]`, `[email protected]`,
`[email protected]`, and the latest of `vfio-bindings`, `vfio-ioctls`,
`mshv-bindings`,`mshv-ioctls`, and `vfio-user`.

* A fork of the `kvm-bindings` crate is being used to support
serialization of various structs for migration [1]. Also, code changes
are made to accommodate the updated `struct xsave` from the Linux
kernel. Note: these changes related to `struct xsave` break
live-upgrade.

** The new `kvm-ioctls` crate introduced breaking changes for
the `get/set_one_reg` API on `aarch64` [2], so code changes are made to
the new APIs.

*** A fork of the `versionize_derive` crate is being used to support
versionize on packed structs [3].

[1] https://github.com/cloud-hypervisor/kvm-bindings/tree/ch-v0.7.0
[2] rust-vmm/kvm#223
[3] https://github.com/cloud-hypervisor/versionize_derive/tree/ch-0.1.6

Fixes: cloud-hypervisor#6072

Signed-off-by: Bo Chen <[email protected]>
@likebreath likebreath marked this pull request as ready for review January 24, 2024 19:31
@likebreath likebreath requested a review from a team as a code owner January 24, 2024 19:31
@rbradford
Copy link
Member

@likebreath In the long term when the ABI/API is stabilised all the locally defined contstants and structures can be removed and replaced with versions from the bindings.

Copy link
Member

@rbradford rbradford left a comment

Choose a reason for hiding this comment

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

🚀

@rbradford rbradford merged commit 3ce0fef into cloud-hypervisor:main Jan 25, 2024
25 checks passed
@likebreath likebreath added the bug-fix Bug fix to include in release notes label Jan 26, 2024
blitz added a commit to cyberus-technology/cloud-hypervisor that referenced this pull request Mar 20, 2024
This is a bug fix release. The following issues have been addressed:

* Fix several security advisories from dependencies (cloud-hypervisor#6134, cloud-hypervisor#6141)
* Enable HTT flag to avoid crashing cpu topology enumeration software
such as hwloc in the guest (cloud-hypervisor#6146)
* Enable nested virtualization on AMD if supported (cloud-hypervisor#6106)
* Handle non-power-of-two CPU topology properly (cloud-hypervisor#6062)
* Various bug fixes around virtio-vsock(cloud-hypervisor#6080, cloud-hypervisor#6091, cloud-hypervisor#6095)
* Align VFIO devices PCI BARs naturally (cloud-hypervisor#6196)

Conflicts:
	Cargo.toml
	arch/src/x86_64/mod.rs
blitz added a commit to cyberus-technology/cloud-hypervisor that referenced this pull request Mar 20, 2024
This is a bug fix release. The following issues have been addressed:

* Fix several security advisories from dependencies (cloud-hypervisor#6134, cloud-hypervisor#6141)
* Enable HTT flag to avoid crashing cpu topology enumeration software
such as hwloc in the guest (cloud-hypervisor#6146)
* Enable nested virtualization on AMD if supported (cloud-hypervisor#6106)
* Handle non-power-of-two CPU topology properly (cloud-hypervisor#6062)
* Various bug fixes around virtio-vsock(cloud-hypervisor#6080, cloud-hypervisor#6091, cloud-hypervisor#6095)
* Align VFIO devices PCI BARs naturally (cloud-hypervisor#6196)

Conflicts:
	Cargo.toml
	arch/src/x86_64/mod.rs
rbradford added a commit to rbradford/cloud-hypervisor that referenced this pull request May 3, 2024
And bump release verion used to v39.0

Fixes: cloud-hypervisor#6134

Signed-off-by: Rob Bradford <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request May 4, 2024
And bump release verion used to v39.0

Fixes: #6134

Signed-off-by: Rob Bradford <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug-fix Bug fix to include in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Bump vmm-sys-util to v0.12.0 or newer
2 participants