Skip to content
This repository has been archived by the owner on Nov 6, 2024. It is now read-only.

Removing x86_64 restriction #72

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JonathanWoollett-Light
Copy link

@JonathanWoollett-Light JonathanWoollett-Light commented Oct 27, 2022

Summary of the PR

It can be useful to manipulate x86_64 structures on other system architectures (e.g. aarch64). These can be easily exposed on non-x86_64 systems to allow this without any drawbacks or breaking changes by making the module public and removing the #[cfg(any(target_arch = "x86", target_arch = "x86_64"))].

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.

Signed-off-by: Jonathan Woollett-Light <[email protected]>
@JonathanWoollett-Light JonathanWoollett-Light changed the title Removing CPUID x86_64 restriction Removing x86_64 restriction Oct 27, 2022
Copy link
Member

@andreeaflorescu andreeaflorescu left a comment

Choose a reason for hiding this comment

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

I am not sure this is the right thing to do in this case. We are not exporting the KVM bindings on platforms where they're not supported on purpose to reduce the risk of misuing them. What feature would make it necessary to manipulate something that is x86_64 specific on aarch64?

@JonathanWoollett-Light
Copy link
Author

JonathanWoollett-Light commented Oct 27, 2022

At the moment for me this is a convenience change.

I'm designing some CPUID functionality in Firecracker in a crate, to restrict the relevant functionality to x86_64 there seems to be 2 approaches:

  1. Add build.rs with
    fn main() {
        #[cfg(not(any(target_arch = "x86", target_arch = "x86_64")))]
        compile_error!("This crate only supports `x86` or `x86_64`);
    }
    
  2. Add #[cfg(any(target_arch = "x86", target_arch = "x86_64"))] to specific unsupported functionality like KVM_SET_CPUID2.

I find 2nd approach here preferable (this usage of build.rs is inelegant/imprecise), however without the changes in this PR doing this becomes significantly messier with a large number of items needing to be under the cfg.

The build.rs approach creates build issues with cargo build --all in a workspace, requiring crates have a lib.rs like:

#[cfg(any(target_arch = "x86", target_arch = "x86_64"))]
mod x86_64 {
   /* lib.rs context */
}
pub use x86_64;

I think only adding cfg where it is absolutely necessary is the best and idiomatic approach.


I would add that it is not unforeseeable an application would want to manipulate CpuId on aarch64, perhaps a client application running on aarch64 configures cpuid locally then across http instantiates a VM on x86_64 with this configuration.

@andreeaflorescu
Copy link
Member

@JonathanWoollett-Light sorry, I forgot to reply to this. We don't do this kind of future looking changes for the crates because by the time this feature would actually be needed (i.e. manipulating CpuId on aarch64 to start an x86 machine) it might come with different requirements and the code changes we did might not actually be appropriate anymore. That is why we're implementing features using working back from customer requests. Having these defines on aarch64 as well can lead to undefined behavior, and to security concerns because of the difference in structures on different platforms. In this case I don't find it appropriate to merge these changes for convenience.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants