Skip to content

Conversation

@ludfjig
Copy link

@ludfjig ludfjig commented Oct 29, 2025

Summary of the PR

Adds KVM_X86_SET_MSR_FILTER vm ioctl. This is my first contribution so I might be missing something, feedback greatly appreciated. I'm not sure whether there needs to be an actual test running vcpu that tries to read/write some MSR

Closes #358

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.

@ludfjig
Copy link
Author

ludfjig commented Oct 29, 2025

CI is red, but looks unrelated to this change I believe

@RuoqingHe
Copy link
Member

CI is red, but looks unrelated to this change I believe

Yes, leave it to me

Copy link
Member

@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 wonder if we could have a slightly higher level, but safe API here. E.g. kvm_msr_filer::bitmap is a pointer to array of u8s that has nmsrs many bits. We could just have an function that takes a Vec, range checks against nmsrs, and then convert its arguments to the kvm_msr_filter structure and does the ioctl maybe? The other args could even be enums then I think

@ludfjig
Copy link
Author

ludfjig commented Nov 3, 2025

I wonder if we could have a slightly higher level, but safe API here. E.g. kvm_msr_filer::bitmap is a pointer to array of u8s that has nmsrs many bits. We could just have an function that takes a Vec, range checks against nmsrs, and then convert its arguments to the kvm_msr_filter structure and does the ioctl maybe? The other args could even be enums then I think

Thanks for reviewing. That sounds reasonable to me. Do you prefer to have 2 versions of it around (keep the old function around as an unsafe _unchecked()), or do you only want the safe one?

@roypat
Copy link
Member

roypat commented Nov 18, 2025

Sorry for taking a while to get back to this, life's been busy 😭

I think having both higher and lower level functions is good, so that if the ioctl ever gets updated, people can use the low level function without needing to wait for the crate to update the high level apis.

as for the cast, yea, this should be fine as the ioctl really does not actually write to bitmap

roypat
roypat previously approved these changes Nov 18, 2025
@ludfjig
Copy link
Author

ludfjig commented Nov 18, 2025

Sorry for taking a while to get back to this, life's been busy 😭

I think having both higher and lower level functions is good, so that if the ioctl ever gets updated, people can use the low level function without needing to wait for the crate to update the high level apis.

No worries. Thanks for the feedback.

as for the cast, yea, this should be fine as the ioctl really does not actually write to bitmap

Thanks, removed the TODO comment.

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.

x86: KVM_X86_SET_MSR_FILTER vm ioctl

3 participants