Updating IOCTL interface

[AlexAltea]

Disclaimer: This changes the API. Consider it just a proposal, rather than a ready-to-merge patch.

Following the IOCTL-refactoring discussion at #108 (comment), I prepared this "proposal" PR, just for the sake of open discussion: this way seems more appropriate than the issue tracker.

Although centralizing IOCTL's seemed trivial at first, it seems the Darwin and Windows backends evolved separately (not sure though, due to #23). Consequently, they have different numbering, and some comments hint that at some point even the API differed more than it does today. Thus, unifying definitions require backwards-incompatible changes.

Assuming HAXM is using semver this should bump it to v8.x.x.

Changes

The code is self-explanatory. This patch (specifically, 761d8f2) centralizes all IOCTL definitions in the hax_interface.h file by renumbering the function codes as follows:

• Function code space: The lowest common denominator of all platforms: Windows offers 11 bits, Linux/Mac offer 8 bits. Thus we will consistently use 8 bits, and codes are in range [0x00, 0xFF].
  Windows will mask this code with 0x800, to enable the "Custom bit" (see reference).
• Values: Every device will number their functions codes starting from 0x00. This means for instance that HAX_VM_IOCTL_VCPU_CREATE and HAX_VCPU_IOCTL_RUN share the same function code, but that's alright since they are different devices to begin with.
• Flags: Since it is unlikely that all 256 function codes will be needed, we can restrict ourselves to 64 function codes per device, and assigning a special meaning to the top 2 bits. I suggest:
  • Platform ioctls: Flagged with the bit 7th bit (i.e. mask 0x40): This could include ioctls like HAX_VCPU_IOCTL_KICKOFF (which is Windows-specific).
  • Extension ioctls: Flagged with the bit 8th bit (i.e. mask 0x80): This could provide a space for researchers/developers to extend the API to alter the behavior of HAXM in specific ways not suitable to be upstreamed.

Additionally, since the API is changing, I took the opportunity to corrected naming inconsistencies:

• The *DEBUG ioctl of VCPU devices was accidentally declared with a HAX_IOCTL_VCPU_ prefix, not with the de-facto standard HAX_VCPU_IOCTL_ prefix. This has been corrected.
• The *GET_REGS, *SET_REGS ioctls of VCPU devices were declared with a HAX_VCPU_ prefix, not HAX_VCPU_IOCTL_. This has been corrected.

Pending

There's other proposals that I haven't implemented yet, but might be worth discussing (if we are going to change the interface that radically, it's better to get it done right in one attempt):

• Removing HAX_VM_IOCTL_VCPU_CREATE_ORIG.
  All platforms treat it exactly the same as HAX_VM_IOCTL_VCPU_CREATE.
• Removing HAX_VCPU_IOCTL_KICKOFF.
  Seems to be Windows XP specific, which is no longer supported (?).
• Removing HAX_VM_IOCTL_NOTIFY_QEMU_VERSION.
  An hypervisor should have no dependency on a specific host userland application.

Alternatively, and this is a radically different approach, there's the option of adopting the KVM API, as suggested in #106 (comment), but I'm not sure how large the internal differences between both hypervisors are. Regarding license implications, as long as we provide custom headers with identical constants, we should be GPL-free, so it's doable.

The question is whether we want to do it now (v8.x.x), in the future (v9.x.x?), or never.

[AlexAltea]

Alternatively, as yet another option: We would be could allow both numberings to coexist: (1) Mac/Linux, and (2) Windows. This would be backwards-compatible, and just require a slight modification of the macro:

-#define HAX_IOCTL(access, code, type) // ...
+#define HAX_IOCTL(access, code_posix, code_windows, type) // ...

This would be strict refactoring, i.e. changing code, without effect on functionality or compatibility.

[raphaelning]

Ah, yes, thanks for considering the issue of backward compatibility! That's my biggest concern, and I was trying to break this to you in the post I've been writing. I'm glad that we can now solve this problem together :-)

---

First of all, I really appreciate this carefully thought and well written proposal, as always! Indeed this is a big change, one that we should handle with care. But if we do this right, we'll have a cleaner and more coherent HAXM interface that can pave the way for more extensions and innovations.

Please allow me more time to review the details of the proposal. Before that, let's talk about the issue of backward compatibility. As you know, there are the tens of thousands of users who install HAXM as an accelerator for Android Emulator (especially on Windows), and we don't want a new HAXM release to suddenly break their user experience. Android Emulator already has a "minimum HAXM version" requirement, which is currently set to 6.x. So one option is to raise it to 8.x in a future Android Emulator release, but we don't have control over when the new Android Emulator version is released (which may be a lot later than HAXM 8.0.0), and users who try to run the current Android Emulator with HAXM 8.x will get a mysterious failure and have no idea what's going on.

Therefore, I think it would be better to have a grace period for Android Emulator (and QEMU as well) to adopt the new IOCTL encoding scheme, so when a user tries to run:

• New Emulator/QEMU + old HAXM: QEMU realizes that the new IOCTLs are not supported, and shows a clear error message that the user needs to upgrade to HAXM 8.x.
• Old Emulator/QEMU + new HAXM: QEMU uses the old IOCTLs and they still work.

This means we have to keep the old IOCTL definitions somewhere before eventually getting rid of them. They have to be renamed, e.g. to HAX_LEGACY_IOCTL_xxx, HAX_LEGACY_VM_IOCTL_xxx, etc., and the IOCTL handlers will have to have two cases for each IOCTL. I know this doesn't sound pretty, but it's a possible solution. You could minimize the ugliness by moving the old definitions to separate headers.

You said your alternative macro also maintains backward compatibility, so basically we'll just pass the old IOCTL codes to code_posix and code_windows, and give up the new encoding scheme? I like the new scheme (I'll comment on that soon), so I think that's too big a compromise to make.

[raphaelning]

Regarding the pending changes, I agree with the removal of unused IOCTLs like HAX_VM_IOCTL_VCPU_CREATE_ORIG and HAX_VCPU_IOCTL_KICKOFF--they are well past their grace periods. Removing the in-use HAX_VM_IOCTL_NOTIFY_QEMU_VERSION with this PR is also possible, and I've outlined a strategy in a review comment.

As to the radical approach of adopting the KVM API, I think we have to agree on what we want to gain from doing that, since it'll incur a lot of work:

1. The KVM API is designed for Linux, so I don't think it's possible to port all of its IOCTLs to another host OS like Windows. If we end up porting all the essential ones, the HAXM accelerator module in QEMU (hax-all.c) will look like a simplified version of its KVM counterpart, yet they are still separate modules--I don't think the KVM people want to see them merged, because they don't really care about non-Linux hosts. So achieving an API-level resemblance to KVM may not result in better code reuse on the QEMU side.
2. It makes more sense to me if we want to rework some of our IOCTLs and model them after KVM ones, because the KVM API has a better design. But then, we can do that step by step, with backward compatibility in mind. If we take this approach, we should probably come up with a plan to accommodate major API changes later, e.g. how we can replace the system-wide VM/vCPU device nodes in the future without breaking backward compatibility. I think creating a new IOCTL soon that is modeled after KVM_CHECK_EXTENSION may help (the current HAX_IOCTL_CAPABILITY is not very extensible).

[AlexAltea]

This means we have to keep the old IOCTL definitions somewhere before eventually getting rid of them. They have to be renamed, e.g. to HAX_LEGACY_IOCTL_xxx, HAX_LEGACY_VM_IOCTL_xxx, etc., and the IOCTL handlers will have to have two cases for each IOCTL.

Really good idea. I've done that and force-pushed the last commit. Just a small note, rather than modifying the prefix to: HAX_LEGACY_IOCTL_*, I've added a __LEGACY suffix. The reason is merely aesthetic: When looking at the code, vertically-aligned keywords seem easier to match visually. See:

    case HAX_VCPU_IOCTL_GET_REGS__LEGACY:
    case HAX_VCPU_IOCTL_GET_REGS: {
        // ...
    }

as opposed to:

    case HAX_LEGACY_VCPU_IOCTL_GET_REGS:
    case HAX_VCPU_IOCTL_GET_REGS: {
        // ...
    }

Anyway, just a minor thought: it doesn't matter much, since it will get removed in the future anyway. If you want me to use the HAX_LEGACY_IOCTL_* prefix instead, I can still amend the commit.

Regarding the new grace period, I have added reminder comments in the appropriate places to remind ourselves to remove old interfaces. I've specified 2020-01-01. No need to consider it a "strict deadline", but just a future date where we can re-surface this issue and possibly remove legacy interfaces.

I agree with the removal of unused IOCTLs like HAX_VM_IOCTL_VCPU_CREATE_ORIG and HAX_VCPU_IOCTL_KICKOFF--they are well past their grace periods. Removing the in-use HAX_VM_IOCTL_NOTIFY_QEMU_VERSION with this PR is also possible, and I've outlined a strategy in a review comment.

Done!

---

The KVM API is designed for Linux, so I don't think it's possible to port all of its IOCTLs to another host OS like Windows. [...] So achieving an API-level resemblance to KVM may not result in better code reuse on the QEMU side.

True, that makes quite a lot of sense. After re-reading the KVM API docs, it seems rather hard to make the same API compatible with Windows-hosts, e.g. due to POSIX-specific concepts like fd's.

It makes more sense to me if we want to rework some of our IOCTLs and model them after KVM ones, because the KVM API has a better design. But then, we can do that step by step, with backward compatibility in mind.

Fully agreed. Let's do it that way! :-)

[AlexAltea]

Another thing that needs to be changed is the read/write flags used in Linux/Mac (IO, IOR, IOW, IOWR). They were chosen quite arbitrarily, e.g.:

• HAX_VCPU_IOCTL_GET_REGS as IOWR but it should be IOR.
• HAX_VCPU_IOCTL_SET_REGS as IOWR but it should be IOW.

Regarding the issue with IOCTL renaming, I've replied to your inlined comment. Let me know your thoughts!

[raphaelning]

Another thing that needs to be changed is the read/write flags used in Linux/Mac (IO, IOR, IOW, IOWR).

Yes, thanks! Since we're starting afresh, let's get it right this time. Just curious: do you know if these different access flags are merely for readability, or are actually enforced? E.g. does copy_to_user() fail if the IOCTL is declared as _IOW? What about on Mac, where there's no need to manually copy IOCTL parameters between kernel and user space?

On Windows, it seems we can label the IOCTLs with appropriate access flags as well via the last parameter for CTL_CODE:

RequiredAccess
Indicates the type of access that a caller must request when opening the file object that represents the device (see IRP_MJ_CREATE). The I/O manager will create IRPs and call the driver with a particular I/O control code only if the caller has requested the specified access rights.

And I'd suggest the following mapping:

• IO => FILE_ANY_ACCESS
• IOR => FILE_READ_DATA
• IOW => FILE_WRITE_DATA
• IOWR => FILE_READ_DATA | FILE_WRITE_DATA

I've also noticed that QEMU always opens every HAXM device in RW mode, and reuse the fd/handle for all subsequent IOCTLs on that device. Therefore, the proposed change shouldn't break anything on Windows.

[raphaelning]

Regarding the IOCTL interface revamp, I have a few more ideas. Not all of them need to be done in this PR.

1. The new interface deserves a new API level, so we should bump the HAXM API version (see include/hax.h):
   • HAX_CUR_VERSION to 5;
   • HAX_COMPAT_VERSION to 4. After the grace period, we'll bump this to 5. QEMU actually checks this value via HAX_IOCTL_VERSION, so if someone tries to run an older QEMU that only supports API v4, they'll get a clear error message.
2. I've suggested that we rename HAX_IOCTL_VERSION to HAX_IOCTL_GET_API_VERSION, but doesn't "get" usually imply "read-only" (i.e. IOR, rather than IOWR)? How about HAX_IOCTL_CHECK_API_VERSION? EDIT: "Get" is fine, because HAX_IOCTL_VERSION is indeed a "read-only" IOCTL (the caller does not need to tell HAXM what API versions it supports).
3. It would be useful to have new IOCTL for retrieving the driver version (semver encoded as a hex integer, e.g. 0x800000), HAX_IOCTL_GET_RELEASE_VERSION (IOR), but we can leave it for another PR.
4. For API v5, we can keep HAX_IOCTL_CAPABILITY as a legacy IOCTL, but it's not well designed and will run out of capability flags before long. So we should create a new one, e.g. HAX_IOCTL_CHECK_CAPABILITY, modeled after KVM_CHECK_EXTENSION. When QEMU talks to an API v5 HAXM driver, it should assume that all the legacy capabilities are supported, and use the new IOCTL to check the availability of new capabilities before use.

[AlexAltea]

Just curious: do you know if these different access flags are merely for readability, or are actually enforced? E.g. does copy_to_user() fail if the IOCTL is declared as _IOW?

I don't think the kernel bothers enforcing anything, since the driver itself runs at kernel-level too and could bypass any measures, if it wanted. However, these macros are useful beyond mere readability: I remember seeing some ARM GPU drivers using them to automatically dispatch the copy_from_user and copy_to_user calls (it's something we can consider for HAXM as well). See these macros:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.4/drivers/gpu/arm/midgard/mali_kbase_core_linux.c#1067

[AlexAltea]

Regarding the versions, we could simply call it HAX_IOCTL_GET_VERSION, and return all versioning-related information on a single call: e.g., API version, compatible API version and release version.

About that: what versioning system does HAXM use? Note that a system like Semantic Versioning would simplify the required information, since the release version already describes API compatibility via it's MAJOR.MINOR.PATCH naming:

• MAJOR version when you make incompatible API changes.
• MINOR version when you add functionality in a backwards-compatible manner, and
• PATCH version when you make backwards-compatible bug fixes.

That way, this PR would bump the HAXM version to v7.4.0 (new functionality/API, but compatible with userland applications written for v7.x.x). After the grace period, we would remove legacy interfaces and bump it to v8.0.0 (which implies incompatibility with v7.x.x).

PS: It's just a thought on the matter. If Intel uses a different internal versioning system, that's alright too.

[AlexAltea]

Regarding "capabilities" (i.e. extensions in KVM terms) vs "extensions". We could offer these two ioctl's:

• HAX_IOCTL_CHECK_CAPABILITY: Checking standard optional features.
  This could be done numeric identifiers, e.g. HAX_CAP_${feature} macros rather than flags to prevent running out of flags. Alternatively, we could have an larger bit-field maybe 512-bits.
• HAX_IOCTL_CHECK_EXTENSION: Checking non-standard optional features (from 3rd parties).
  This could be done via string indentifiers, e.g. "HAX_EXT_${vendor}_${feature}". If available, it could return the IOCTL code where the extension is available (as a sanity-check). The reason for string identifiers is preventing overlapping numbers from different vendors at check-time.

While I think non-standard, third-party extensions may be useful for some people who use the HAXM source code for their own purposes, I still encourage people to contribute back to this upstream project and help avoid fragmentation

Absolutely. The main purpose of "extensions" is creating very-concrete applications with HAXM, e.g. creating specific tunnels with specific guest systems or instrumenting specific instructions, which might not be suitable to be upstreamed.

With this HAX_IOCTL_CHECK_EXTENSION mechanism, those extensions implemented in HAXM forks will coexist easier with the original HAXM, thus preventing "hard forks", and allowing exchanges of generic features, e.g. performance/compatibility patches, in both directions.

[raphaelning]

Regarding the versions, we could simply call it HAX_IOCTL_GET_VERSION, and return all versioning-related information on a single call: e.g., API version, compatible API version and release version.

Good idea!

If Intel uses a different internal versioning system, that's alright too.

Thanks for explaining the semver system! Having a separate API version is indeed not the best design. For the release version, I wouldn't say we have been following any well-established versioning convention for HAXM... For "MINOR" and "PATCH" numbers, I think we do try to follow semver, but we've been incrementing "MAJOR" for non-technical reasons (e.g. 7.0 doesn't break backward compatibility with 6.x, but it's the first HAXM release based on the open source codebase, so we wanted a change). I don't know if deviations like this from the semver rules will happen again in the future, so I'd just keep the API version for now. After all, there may be a point in the future when we declare the HAXM API to be stable and do not allow backward-incompatible changes (KVM does that), yet we somehow have another incentive to bump the major version.

[raphaelning]

This could be done numeric identifiers, e.g. HAX_CAP_${feature} macros rather than flags to prevent running out of flags.

Sounds good! The only advantage of using bit fields is to allow specifying a combination of capabilities, but I don't think we ever need that.

As for making provision for 3rd-party extensions, I agree with your reasoning. Please feel free to implement your proposal with this PR or leave it for later. I can review the code, but since this IOCTL is currently unused, it would be best if you could test it on your side.

[krytarowski]

There is also interest in NetBSD and HAXM (I have got a scratch locally, need to rebase and fix bugs).. so getting another Linux-only API will be rejecting the non-Linux world.

[krytarowski]

There is also one person trying to port HAXM to DragonFlyBSD.

[AlexAltea]

so getting another Linux-only API will be rejecting the non-Linux world.

It's rather POSIX-only, but yeah that statement makes sense regardless. I didn't think about this issue when I mentioned "adopting the KVM API". So instead the optimal approach (what this PR does) is just keeping the current API, but just renumbering/renaming IOCTL's to have their definitions shared across all platforms: Windows, Linux, Mac (and hopefully *BSD!).

[krytarowski]

Crazy idea... how about merging HAXM with nvmm from NetBSD? NVMM ships with AMD CPUs support and it has API inspired by the Windows one.

Today HAXM is open-source and radical changes in interface are probably fine as everybody is able to rebuild it from sources.

[krytarowski]

http://m00nbsd.net/4e0798b7f2620c965d0dd9d6a7a2f296.html it got merged with the NetBSD source-code

[krytarowski]

@AlexAltea I will drop you a line in a mail.

[AlexAltea]

how about merging HAXM with nvmm from NetBSD?

Since the main advantage here is AMD CPU support, probably the discussion is better suited for #5. Regarding license, since NVMM is BSD-licensed, this is definitely possible.

Quoting Intel's position on AMD support:

We don't plan to implement, maintain, or validate this ourselves, but we don't have any objections to seeing another team of developers extend HAXM to support AMD-V.

So I don't think there's going to be any opposition if we cherry-pick the necessary parts of NVMM and integrate them into HAXM. Of course, it will require some refactoring, specially since vcpu.c is polluted with VMX-specific code that should go into vmx.c, but it's definitely doable.

radical changes in interface are probably fine as everybody is able to rebuild it from sources.

Unfortunately, this is not the case. It seems a majority of the HAXM usage share comes from Android Studio developers who rely on pre-built QEMU (provided by Google) + HAXM (provided by Intel). Any change to the interface needs to be coordinated with Intel, Google and QEMU developers. And to prevent issues caused mismatching on QEMU-HAXM versions, legacy interfaces will require support during a grace period.

Summarizing:

• BSD support is fine if maintained by non-Intel parties (can be discussed at Add Support for FreeBSD? #106).
• AMD support is fine if maintained by non-Intel parties (can be discussed at AMD CPU support #5).
• Changing the interface is fine given good reasons, and temporary backwards compatibility
  (can be discussed here).

That said, I'm just another contributor here: @raphaelning will probably be the one deciding.

[tuxillo]

@AlexAltea in #qemu I was pointed to: https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg01482.html

Do you have a qemu fork with your changes? What do you use to test your linux port?

[AlexAltea]

@tuxillo The QEMU patch for haxm-linux support can be found at: https://github.com/kryptoslogic/qemu/tree/haxm-linux (specifically: kryptoslogic/qemu@860b93f)
The latest patch in the mailing list is at:
http://lists.nongnu.org/archive/html/qemu-devel/2018-11/msg02768.html

Please, let's keep this thread on-topic ("Updating IOCTL interface"). Any HAXM-on-Linux questions can be discussed on #108 instead. :-)

[raphaelning]

@AlexAltea Thanks for the summary! Indeed, proposals for radical API changes must come with a plan to maintain backward compatibility. But I see that @krytarowski has dropped his idea about adopting the NVMM IOCTL interface, so you can continue with what you planned to do with this PR.

[xianxiaoyin]

Build finished.

[xianxiaoyin]

sorry,please disregard my previous comment.

[yiqisiben]

Can one of the admins verify this patch?

[yiqisiben]

sorry,please disregard my previous comment.