-
Notifications
You must be signed in to change notification settings - Fork 111
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
move encrypt_op{_sev} into system ioctls #259
base: main
Are you sure you want to change the base?
Conversation
51f7146
to
4e450a0
Compare
Based on the KVM API, KVM_MEMORY_ENCRYPT_OP is a system ioctl. However, this was not reflected in the crate. Signed-off-by: Jake Correnti <[email protected]>
4e450a0
to
83734be
Compare
Just for reference, this is the documentation that states the KVM API now uses |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late review, my GitHub search was not showing this one for some reason :(
@@ -7,6 +7,8 @@ | |||
- [[#255](https://github.com/rust-vmm/kvm-ioctls/issues/255)]: Fixed a | |||
soundness issue when accessing the `kvm_run` struct. `VcpuFd::run()` and | |||
`VcpuFd::set_kvm_immediate_exit()` now take `&mut self` as a consequence. | |||
- Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add the PR number here?
- Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line | |
- [[#259](https://github.com/rust-vmm/kvm-ioctls/pull/259)] Changed `encrypt_op` and `encrypt_op_sev` to system ioctls to be in line |
pub unsafe fn encrypt_op<T>(&self, fd: &impl AsRawFd, op: *mut T) -> Result<()> { | ||
let ret = ioctl_with_mut_ptr(fd, KVM_MEMORY_ENCRYPT_OP(), op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with AMD-SEV, but if this is a system ioctl, shouldn't it be issued on the KVM fd? e.g.
pub unsafe fn encrypt_op<T>(&self, fd: &impl AsRawFd, op: *mut T) -> Result<()> { | |
let ret = ioctl_with_mut_ptr(fd, KVM_MEMORY_ENCRYPT_OP(), op); | |
pub unsafe fn encrypt_op<T>(&self, op: *mut T) -> Result<()> { | |
let ret = ioctl_with_mut_ptr(&self.kvm, KVM_MEMORY_ENCRYPT_OP(), op); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I believe you're right. It was brought to my attention that I was referencing stale documentation. However, the changes to the ioctl should be so that it is both a vm ioctl and a vcpu ioctl, so that requires an update on my end.
Not sure how old that documentation is - but v6.9 - the last release version shows this is a vm type ioctl - https://elixir.bootlin.com/linux/v6.9/source/Documentation/virt/kvm/api.rst#L4732 The wrong label was fixed in torvalds/linux@46ca9ee |
I identified the issue as a case of stale content on kernel.org when the directory was renamed by commit 2f5947dfcaecb99f2dd559156eecbeb7b95e4c02 Author: Christoph Hellwig <[email protected]> Date: Wed Jul 24 09:24:49 2019 +0200 Documentation: move Documentation/virtual to Documentation/virt Renaming docs seems to be en vogue at the moment, so fix on of the grossly misnamed directories. We usually never use "virtual" as a shortcut for virtualization in the kernel, but always virt, as seen in the virt/ top-level directory. Fix up the documentation to match that. Fixes: ed16648eb5b8 ("Move kvm, uml, and lguest subdirectories under a common "virtual" directory, I.E:") Signed-off-by: Christoph Hellwig <[email protected]> Signed-off-by: Paolo Bonzini <[email protected]> Compare - https://www.kernel.org/doc/Documentation/virt/kvm/api.txt (current) with https://www.kernel.org/doc/Documentation/virtual/kvm/api.txt (stale) I have contacted the kernel.org team to highlight the issue. |
Thank you for doing that. However, I still see this change being needed (with some adjustments). Intel TDX (and I think Arm CCA as well) is repurposing this ioctl and it is used on both Vm fd's and Vcpu fd's. Intel's documentation about it can be located here. |
Summary of the PR
According to the KVM API,
KVM_MEMORY_ENCRYPT_OP
is labeled as a system ioctl. However, it is currently treated as a VM ioctl. There are no functional changes to the code, only the location and function signature.Requirements
Before submitting your PR, please make sure you addressed the following
requirements:
git commit -s
), and the commit message has max 60 characters for thesummary and max 75 characters for each description line.
test.
Release" section of CHANGELOG.md (if no such section exists, please create one).
unsafe
code is properly documented.