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

ioctls: Don't panic on unsupported exit reason #195

Merged
merged 1 commit into from
Feb 15, 2022
Merged

Conversation

sboeuf
Copy link

@sboeuf sboeuf commented Feb 14, 2022

In case the underlying KVM version is more recent than what is known
and supported by the kvm-ioctls crate, and in case KVM reports an
unknown reason for a VM exit, we don't want the vCPU run() to panic.

A more elegant way of handling such situation is by propagating the exit
reason up to the consumer's crate as it might know what to do with it.

Signed-off-by: Sebastien Boeuf sebastien.boeuf@intel.com

@sboeuf
Copy link
Author

sboeuf commented Feb 14, 2022

In our case, this is particularly useful to support KVM_EXIT_TDX which is not yet part of the upstream Linux kernel.

/// Corresponds to an exit reason that is unknown from the current version
/// of the kvm-ioctls crate. Let the consumer decide about what to do with
/// it.
Future(u32),
Copy link
Member

Choose a reason for hiding this comment

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

Can we have it as Unknown instead?

Copy link
Author

Choose a reason for hiding this comment

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

I wanted to use Unknown, but there's an actual KVM exit reason called KVM_EXIT_UNKNOWN :(

Do you have any other idea?

Copy link
Member

Choose a reason for hiding this comment

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

Right, I forgot about that one. We can name it Unsupported? Returning this as Unsupported is not going to bring much information though, it is just a stepping stone I assume until we add the actual variants defined here?

Copy link
Member

Choose a reason for hiding this comment

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

Really funny, a known unknown and an unknown unknown:)

Copy link
Member

Choose a reason for hiding this comment

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

Really funny, a known unknown and an unknown unknown:)

Exactly 😆

Copy link
Author

Choose a reason for hiding this comment

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

Yes I think Unsupported would be appropriate here.

Copy link
Author

Choose a reason for hiding this comment

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

it is just a stepping stone I assume until we add the actual variants defined here?

Yes exactly, but we have to wait for these things to land in upstream Linux kernel before we can update the kvm-bindings and kvm-ioctls.

In case the underlying KVM version is more recent than what is known
and supported by the kvm-ioctls crate, and in case KVM reports an
unknown reason for a VM exit, we don't want the vCPU run() to panic.

A more elegant way of handling such situation is by propagating the exit
reason up to the consumer's crate as it might know what to do with it.

Signed-off-by: Sebastien Boeuf <sebastien.boeuf@intel.com>
@jiangliu jiangliu merged commit f499ce9 into rust-vmm:main Feb 15, 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.

3 participants