Enable setting CPUID feature for guest VCPUs #277

wcwang · 2020-03-20T12:06:25Z

Add a new IOCTL HAX_VCPU_IOCTL_SET_CPUID to enable setting CPUID features for guest vCPUs. Instead of initializing guest CPUID once by the static features set, this feature will support dynamically setting guest CPUID features during the runtime of VM. The implementation refers to Intel SDM Vol. 2A 3.2 CPUID.

Implements corresponding interfaces and data structures in CPUID module
Enables Windows and macOS platforms support
Adds the capability flag HAX_CAP_CPUID
Adds API documentation for HAX_VCPU_IOCTL_SET_CPUID

nevilad · 2020-03-20T14:51:06Z

platforms/windows/hax_entry.c

+            if (inBufLength < sizeof(struct hax_cpuid) +
+                cpuid->total * sizeof(struct hax_cpuid_entry)) {


This will be a crash, if inBufLength is zero, due to access to cpuid->total field.

Good catch. I will update the code immediately. Thanks for your review.

nevilad · 2020-03-22T12:06:07Z

core/include/vcpu.h

+    // * The CPUID feature set is always same for each vCPU. A CPUID instruction
+    //   executed on any core will get the same result.
+    // * All vCPUs share the unique memory, which is actually allocated by the
+    //   first vCPU created by VM. If any vCPU sets features in this field, all
+    //   vCPUs will change accordingly.


Why save this data in vCPU if all of them share same memory? Why not in vm?

Yes, saving in VM has the same effect. But we considered that this IOCTL should be belonging to VCPU as it is CPUID.

nevilad · 2020-03-22T20:12:17Z

core/vcpu.c

+    cpuid_get_guest_features(vcpu->guest_cpuid, &cpuid_1_features_ecx,
+                             &cpuid_1_features_edx,
+                             &cpuid_8000_0001_features_ecx,
+                             &cpuid_8000_0001_features_edx);


SET_CPUID interface allows to set all output registers for any cpuid codes. But the only used data is ecx and edx for code 1 and 80000001. Why?

Good question. Other function code can be added one by one in future. We did not open the setting channels for all codes as HAXM still has not supported for all features in current implementation.

I think #271 should be called set_cpuid and this feature set_cpuid_feature_flags. How Android Studio will use this feature? Who adds ioctl calls there?

nevilad · 2020-03-22T20:25:05Z

core/cpuid.c

+    if (entry->ecx != cpuid->feature_1_ecx ||
+        entry->edx != cpuid->feature_1_edx) {


There are 3 types of action with the actual supported features and the ones provided in ioctl:

Use intersection and ignore mismatch. This is currently used.

Use intersection and return error in case of mismatch. The user wanted the provided features and we can't support these.

Use provided features, no intersection with supported. It is users responsibility when the provided features are not supported by the cpu. This is the case of KVM, but it supports a lot of features, so the problems will be in case when the CPU does not support them.
Would be good add this as flags to the interface, at least in future.

Thanks for your suggestion. We will consider these cases in future.

nevilad · 2020-03-22T20:26:56Z

core/cpuid.c

+    hax_log(HAX_LOGI, "%s: eax: %08lx, ebx: %08lx, ecx: %08lx, edx: %08lx\n",
+            __func__, entry->eax, entry->ebx, entry->ecx, entry->edx);
+
+    cpuid->feature_8000_0001_edx = entry->edx;


ecx is not merged. CUrrently supported by haxm bits are zero but this will change in future.

You are right. This version is based on the current implementation and this feature will be easy to extend in future.

We will update the implementation of CPUID and include a hash table for feature set, which will add ecx or other register settings. It will not change the HAX_VCPU_IOCTL_SET_CPUID interface. The patch will be ready once the current release completes.

A hash table for searching entries by find_cpuid_entry? There are currently only two codes with such flags, a vector is sufficient for this task.

The same data structure hax_cpuid_entry vector for storing in hax_cpuid_t for future extensions in this version. The find_cpuid_entry() will not change.

nevilad · 2020-03-22T20:31:24Z

core/cpuid.c

+    const uint32_t kFixedFeatures =
+        FEATURE(MCE)  |
+        FEATURE(APIC) |
+        FEATURE(MTRR) |
+        FEATURE(PAT);
+
+    cpuid->feature_1_edx |= kFixedFeatures;


Silently adding flags to the ones provided by the user without even log this. I think when the user want to clear these bits this should be a error.

Actually these flags were referred from your commits. As for logs, it will always output the unchanged bits as warning in the caller functions, such as cpuid_set_0000_0001().

nevilad · 2020-03-22T20:39:37Z

Seems like you need a way to set only ecx/edx for cpuid codes 1 and 80000001. How Android Studio will use this feature?
Qemu will set all registers for all supported by the virtual cpu cpuid codes. This feature is not usable for this purpose.

wcwang · 2020-03-24T01:42:22Z

Seems like you need a way to set only ecx/edx for cpuid codes 1 and 80000001. How Android Studio will use this feature?

This PR is only the first version for CPUID feature, as HAXM is open source, you are welcome to suggest the interfaces or implementations if there is any requirement. Android Emulator will use this feature as below user space patch 987306.

Who adds ioctl calls there?

We will continue to provide the user space code to add IOCTL calls for Android Emulator.

wayne-ma · 2020-03-24T11:27:08Z

Ok to verify

nevilad · 2020-03-24T18:58:45Z

The Android patch is called "Enable whitelisted CPU options by default if host supports them" and mentions the need for CPUID_EXT_AES, CPUID_EXT_PCLMULQDQ and CPUID_FXSR features. By default hax returns intersection of supported by CPU and hax features. This PR removes feature bits if they are not supported by CPU and hax. So using this PR it is only possible to remove already supported features. Is this OK?
So far I understand these bits are cpuid_1_features_edx FEATURE(FXSR) and cpuid_1_features_ecx FEATURE(AESNI)/FEATURE(PCLMULQDQ). So these are already supported, no need to set something from usermode.

nevilad · 2020-03-24T19:18:51Z

platforms/windows/hax_entry.c

+            if (inBufLength == 0 || inBufLength < sizeof(struct hax_cpuid) +
+                cpuid->total * sizeof(struct hax_cpuid_entry)) {


This is better:
if (inBufLength < sizeof(struct hax_cpuid) || inBufLength < sizeof(struct hax_cpuid) +
cpuid->total * sizeof(struct hax_cpuid_entry)) {
total is first member with 4 bytes size. So any inBufLength less than 4 bytes will crash.

Thanks for your point. Actually, below expressions should be more exact as sizeof(struct hax_cpuid) is 8. Anyway, I will adopt your suggestion.

if (inBufLength < sizeof(cpuid->total) || ...) or if (inBufLength < sizeof(uint32_t) || ...)

wcwang · 2020-03-25T02:32:16Z

So using this PR it is only possible to remove already supported features. Is this OK?

I totally agree with your opinion. In the earlier design of this PR, we would have set the features out of current supported scope. But we found that if user set any value, which feature has not been implemented by HAXM, and then it would result in regression. Just as the feature FEATURE(TSC_DEADLINE), I once asked you in my review comments for PR #271. We will attempt to extend the supported feature set in future after more verification. In future, probably we will separate the default feature set from the supported feature set. Then user can add or remove features by calling this IOCTL. Currently, it is OK to only remove supported features.

So far I understand these bits are cpuid_1_features_edx FEATURE(FXSR) and cpuid_1_features_ecx FEATURE(AESNI)/FEATURE(PCLMULQDQ). So these are already supported, no need to set something from usermode.

Yes, you are right. Actually, this feature requirement came before the FEATURE(PCLMULQDQ) was added. We intended to enable such features by calling this interface. Due to aforementioned reasons, we screened the unsupported features out in this version. We will consider improving the current framework to meet more requirements. Any of your further input will be important for us and your contribution will be welcome. Thanks.

Add a new IOCTL HAX_VCPU_IOCTL_SET_CPUID to enable setting CPUID features for guest vCPUs. Instead of initializing guest CPUID once by the static features set, this feature will support dynamically setting guest CPUID features during the runtime of VM. The implementation refers to Intel SDM Vol. 2A 3.2 CPUID. * This version implements corresponding interfaces and data structures in CPUID module and completes Windows platform support. * Based on the current implmentation, it only supports to set the guest CPUID features when the initial value of EAX register is 01H or 80000001H. Signed-off-by: Wenchao Wang <[email protected]>

Enables setting CPUID features for guest vCPUs in macOS platform. Signed-off-by: Wenchao Wang <[email protected]>

Enables setting CPUID features for guest vCPUs in Linux platform. Signed-off-by: Wenchao Wang <[email protected]>

Add the capability flag HAX_CAP_CPUID to indicate the CPUID feature is already supported. This capability information is used for user space query. Signed-off-by: Wenchao Wang <[email protected]>

Android Emulator may dynamically extend the CPUID feature set of VCPU at guest runtime via x86_cpu_update_enable_features() ($AOSP/external/qemu/target/i386/cpu.c). It will invoke this IOCTL to set guest CPUID features, which is statically initialized in HAXM. The HAX_VCPU_IOCTL_SET_CPUID specification includes following aspects: * IOCTL definition and description * Parameters * Error codes * Interface availability Signed-off-by: Wenchao Wang <[email protected]>

krytarowski · 2020-03-25T14:51:53Z

NetBSD support is missing. I can test it.. but is there a qemu support?

nevilad · 2020-03-25T15:46:43Z

This feature is made for Android Studio. Qemu support will not be added. I update my PR and then it can be tested.

krytarowski · 2020-03-25T16:48:08Z

OK. I'm going to submit a patch soon.

krytarowski · 2020-03-25T16:51:23Z

This PR also broke building on NetBSD.

krytarowski · 2020-03-25T16:59:42Z

#281

wcwang · 2020-03-26T07:38:25Z

@krytarowski Thanks for your reply. It supports QEMU and I have shared the user space patches for QEMU here. You input will be welcome.

wcwang mentioned this pull request Mar 20, 2020

Add set_cpuid ioctl #271

Open

nevilad reviewed Mar 20, 2020

View reviewed changes

nevilad reviewed Mar 22, 2020

View reviewed changes

wcwang force-pushed the cpuid-support branch 3 times, most recently from 9152ace to 6f733a3 Compare March 24, 2020 11:01

nevilad reviewed Mar 24, 2020

View reviewed changes

HaxmCI added CI:Build Pass CI:Build Pass CI:Mac Test Pass CI:Mac Test Pass labels Mar 25, 2020

wcwang added 5 commits March 25, 2020 10:55

mac: cpuid: Add HAX_VCPU_IOCTL_SET_CPUID support

084657f

Enables setting CPUID features for guest vCPUs in macOS platform. Signed-off-by: Wenchao Wang <[email protected]>

linux: cpuid: Add HAX_VCPU_IOCTL_SET_CPUID support

c7da519

Enables setting CPUID features for guest vCPUs in Linux platform. Signed-off-by: Wenchao Wang <[email protected]>

Add new capability HAX_CAP_CPUID

72e53fa

Add the capability flag HAX_CAP_CPUID to indicate the CPUID feature is already supported. This capability information is used for user space query. Signed-off-by: Wenchao Wang <[email protected]>

wcwang force-pushed the cpuid-support branch from 6f733a3 to 8f042b0 Compare March 25, 2020 02:55

coxuintel self-requested a review March 25, 2020 03:05

coxuintel approved these changes Mar 25, 2020

View reviewed changes

wcwang merged commit 93998a7 into master Mar 25, 2020

wcwang deleted the cpuid-support branch March 25, 2020 03:07

wcwang mentioned this pull request Apr 7, 2020

OVML support still doesn't work in version 7.5.6.. #286

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable setting CPUID feature for guest VCPUs #277

Enable setting CPUID feature for guest VCPUs #277

wcwang commented Mar 20, 2020

nevilad Mar 20, 2020 •

edited

Loading

wcwang Mar 23, 2020

nevilad Mar 22, 2020

wcwang Mar 23, 2020

nevilad Mar 22, 2020

wcwang Mar 23, 2020

nevilad Mar 23, 2020

nevilad Mar 22, 2020

wcwang Mar 23, 2020

nevilad Mar 22, 2020

wcwang Mar 23, 2020

wcwang Apr 3, 2020

nevilad Apr 3, 2020

wcwang Apr 7, 2020 •

edited

Loading

nevilad Mar 22, 2020

wcwang Mar 23, 2020

nevilad commented Mar 22, 2020

wcwang commented Mar 24, 2020

wayne-ma commented Mar 24, 2020

nevilad commented Mar 24, 2020

nevilad Mar 24, 2020

wcwang Mar 25, 2020

wcwang commented Mar 25, 2020

krytarowski commented Mar 25, 2020

nevilad commented Mar 25, 2020

krytarowski commented Mar 25, 2020

krytarowski commented Mar 25, 2020

krytarowski commented Mar 25, 2020

wcwang commented Mar 26, 2020

		if (inBufLength < sizeof(struct hax_cpuid) +
		cpuid->total * sizeof(struct hax_cpuid_entry)) {

		if (entry->ecx != cpuid->feature_1_ecx \|\|
		entry->edx != cpuid->feature_1_edx) {

		if (inBufLength == 0 \|\| inBufLength < sizeof(struct hax_cpuid) +
		cpuid->total * sizeof(struct hax_cpuid_entry)) {

Enable setting CPUID feature for guest VCPUs #277

Enable setting CPUID feature for guest VCPUs #277

Conversation

wcwang commented Mar 20, 2020

nevilad Mar 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcwang Apr 7, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nevilad commented Mar 22, 2020

wcwang commented Mar 24, 2020

wayne-ma commented Mar 24, 2020

nevilad commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcwang commented Mar 25, 2020

krytarowski commented Mar 25, 2020

nevilad commented Mar 25, 2020

krytarowski commented Mar 25, 2020

krytarowski commented Mar 25, 2020

krytarowski commented Mar 25, 2020

wcwang commented Mar 26, 2020

nevilad Mar 20, 2020 •

edited

Loading

wcwang Apr 7, 2020 •

edited

Loading