Skip to content

Conversation

@RuoqingHe
Copy link
Member

Summary of the PR

Test cases introduced by SBI related changes require SBI_V01 to be enabled during kernel compilation. c41370c is capable of covering this case.

Unblocks #352.

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.

ShadowCurse
ShadowCurse previously approved these changes Oct 13, 2025
@roypat
Copy link
Member

roypat commented Oct 14, 2025

seems like no_run tests are still compiled. I guess this one needs some imports behind # and the size of sizeof -> size_of then

@RuoqingHe
Copy link
Member Author

seems like no_run tests are still compiled. I guess this one needs some imports behind # and the size of sizeof -> size_of then

I'm in the middle of addressing this, it looks like this snippet is outdated, it looks like KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) will not work, I trying to use ioctls_with_val with KVM_CAP_XSAVE2 to get the value here

@roypat
Copy link
Member

roypat commented Oct 14, 2025

seems like no_run tests are still compiled. I guess this one needs some imports behind # and the size of sizeof -> size_of then

I'm in the middle of addressing this, it looks like this snippet is outdated, it looks like KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) will not work, I trying to use ioctls_with_val with KVM_CAP_XSAVE2 to get the value here

admittedly, this snippet was never meant to actually compile or anything, it was just some pseudo code to illuminate the calculation

@RuoqingHe RuoqingHe force-pushed the bump-rust-vmm-ci-to-c41370c branch from 80fb106 to aaa4072 Compare October 15, 2025 09:09
Comment on lines 138 to 158
/// ```no_run
/// # use kvm_bindings::{__u32, kvm_xsave, KVM_CAP_XSAVE2};
/// # use libc::{c_char, open, O_RDWR};
/// # use std::fs::File;
/// # use std::os::unix::io::FromRawFd;
/// # use vmm_sys_util::{ioctl::ioctl_with_val, ioctl_io_nr};
/// # const KVM_PATH: &str = "/dev/kvm\0";
/// # const KVMIO: u32 = 0x03;
/// # let kvm_fd = unsafe { open(KVM_PATH.as_ptr() as *const c_char, O_RDWR) };
/// # assert!(kvm_fd >= 0);
/// # ioctl_io_nr!(KVM_CHECK_EXTENSION, KVMIO, 0x03);
///
/// ((unsafe {
/// ioctl_with_val(
/// &File::from_raw_fd(kvm_fd),
/// KVM_CHECK_EXTENSION(),
/// KVM_CAP_XSAVE2.into(),
/// )
/// }) as usize
/// - size_of::<kvm_xsave>())
/// .div_ceil(size_of::<__u32>());
Copy link
Member

Choose a reason for hiding this comment

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

I really do think this is clearer as just the pseudo code it was before :/

Copy link
Member Author

@RuoqingHe RuoqingHe Oct 19, 2025

Choose a reason for hiding this comment

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

Should I just remove this code snippet (the surrounding ```), and perhaps use (KVM_CHECK_EXTENSION(KVM_CAP_XSAVE2) - sizeof::<kvm_xsave>()).div_ceil(sizeof::<__u32>()) instead

Copy link
Member

Choose a reason for hiding this comment

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

yeah, that'd be my preference

Test cases introduced by SBI related changes require SBI_V01 to be
enabled during kernel compilation. At the point of `042b206` already
incorporates `c41370c`, which is capable of covering this case.

Signed-off-by: Ruoqing He <[email protected]>
@RuoqingHe RuoqingHe force-pushed the bump-rust-vmm-ci-to-c41370c branch from aaa4072 to 7c8cc06 Compare October 27, 2025 10:20
@roypat roypat merged commit 48d93d6 into rust-vmm:main Oct 28, 2025
27 checks passed
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.

4 participants