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

Add KVM_CAP_GUEST_DEBUG_HW_BPS/WPS #202

Merged
merged 2 commits into from
Aug 22, 2022

Conversation

michael2012z
Copy link
Contributor

Summary of the PR

The API KVM_CAP_GUEST_DEBUG_HW_BPS and KVM_CAP_GUEST_DEBUG_HW_WPS
are required to determine the number of supported guest debug registers.

Requirements

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

  • All commits in this PR are signed (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.
  • Any newly added unsafe code is properly documented.

Comment on lines +607 to +626
assert!(kvm.get_guest_debug_hw_bps() >= 0);
assert!(kvm.get_guest_debug_hw_wps() >= 0);
Copy link
Contributor Author

@michael2012z michael2012z Aug 18, 2022

Choose a reason for hiding this comment

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

To discuss:

I am not sure about the rule to check the result. The returned value vary on different machines. ">=0" seems not very meaningful.

And good idea?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's okay to check it as it is now in the test because we don't know what hosts we're running on. Can you add a comment about why we're doing the check this way? (i.e. different values on different platforms).

///
/// Returns 0 if the capability is not available and a positive integer otherwise.
#[cfg(target_arch = "aarch64")]
pub fn get_guest_debug_hw_wps(&self) -> i32 {
Copy link
Contributor Author

@michael2012z michael2012z Aug 18, 2022

Choose a reason for hiding this comment

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

If there is increasing requirements to call the APIs that return a integer, is it good to expose check_extension_int itself?

Copy link
Member

Choose a reason for hiding this comment

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

We can mark it as public as well, I don't have any concern with that. That's a good point!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a commit for that.

pub fn get_host_ipa_limit(&self) -> i32 {
self.check_extension_int(Cap::ArmVmIPASize)
}

/// AArch64 specific call to get the number of supported hardware breakpoints.
///
/// Returns 0 if the capability is not available and a positive integer otherwise.
Copy link
Member

Choose a reason for hiding this comment

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

Does the integer bear any meaning? Can we return a bool in case it doesn't have any useful information?

Copy link
Contributor

Choose a reason for hiding this comment

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

The integer is the number of breakpoints.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The integer is useful in supporting the debug of guest kernel.
On x86, the max number of breakpoints is 4, while on arm the number is not fixed but is indicated by this API.

The API `KVM_CAP_GUEST_DEBUG_HW_BPS` and `KVM_CAP_GUEST_DEBUG_HW_WPS`
are required to determine the number of supported guest debug registers.

Signed-off-by: Michael Zhao <michael.zhao@arm.com>
The function is useful for checking the capabilities that return numbers
rather than booleans.

Signed-off-by: Michael Zhao <michael.zhao@arm.com>
Copy link
Collaborator

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@lauralt lauralt merged commit 6705a61 into rust-vmm:main Aug 22, 2022
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.

5 participants