Skip to content
This repository has been archived by the owner on Jan 28, 2023. It is now read-only.

Add set_cpuid ioctl #271

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

Add set_cpuid ioctl #271

wants to merge 1 commit into from

Conversation

nevilad
Copy link
Contributor

@nevilad nevilad commented Mar 8, 2020

Add set_cpuid ioctl that enables setting cpuid values to be returned by hax.
It currently disallows to setting cpuids for processors newer than Penryn, since hax does not support newer MSRs etc. This is done through checking of maximim cpuid codes in 0 and 0x80000000 ranges. There are also checks that disallow clear some bits like APIC availability, since this is not supported by other parts of hax.
The qemu patch checks if qemu is runing with nondefault cpu (has a -cpu switch) and if not, calls set_cpuid ioctl. It also checks for nonsupported values. When running with unsupported values or failing to set cpuids, qemu exits.
It is possible to add non-supported bits to cpuid feature bits through +command line switch in qemu. This is not handled here, but it is possible in hax to compare the received bits with hardcoded support masks and the actual CPU values to see, that all bits are supported. KVM does this not, so I've done it the same way.
The ioctl structs look the same way as in KVM set_cpuid2 ioctl. There is a constant size header and variable size entries. I've tested it on windows host and guest. I've created the code in hax for linux/netbsd/darwin too, but didn't test it.

when cpuids are set:

  • HYPERVISOR bit is not returned by hax in cpuid 1. KVM behaves the same way.
  • hax does not handle 0x40000000 leaf which is too a way for the guest to understand that in runs in a VM.
  • version changes in cpuid 1 are not working, but this should be OK, since the user can change the CPU to another.
  • Maximum number of addressable IDs for logical processors in this physical package in cpuid 1 in hax is 1, and in qemu 0. I don't know which implications that can have.
    0001-Set-cpuid-values-to-hax-through-set_cpuid-ioctl-when.patch.txt

@krytarowski
Copy link
Contributor

krytarowski commented Mar 8, 2020

Qemu does not include hax code for linux/netbsd, what are the clients for those OSes?

Please elaborate.. I use qemu+HAXM/NetBSD on a daily basis.

@krytarowski
Copy link
Contributor

I've created the code in hax for linux/netbsd/darwin too, but looks like netbsd/darwin are unable to receive dynamically sized structs through ioctl, at least I've didn't found a ioctl for darwin, that receives the size argument.

It looks like the proper approach is to go for passing struct iovec with a pointer + size to a dynamically sized buffer. Later in the kernel code, perform additional copyin() for NetBSD, copy_from_user() on Linux etc.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 8, 2020

Please elaborate.. I use qemu+HAXM/NetBSD on a daily basis.

Qemu 2.12 makefile.objs has ifdef CONFIG_DARWIN statement, bit none for linux/netbsd/posix, same with sources - has hax-darwin.c/h but none for others. And ifdef CONFIG_DARWIN in includes.

Qemu 4.2 has ifeq ($(CONFIG_POSIX),y) and ifdef CONFIG_POSIX statements, but the comments in hax-all.c/hax-posix.c are:

  • HAX common code for both windows and darwin
  • HAX module interface - darwin version

May I assume that:

  1. Netbsd/linux/darwin are all POSIX?
  2. Qemu added support for them after 2.12?
  3. Netbsd/linux/darwin should all have same ioctl definitions?

@krytarowski
Copy link
Contributor

Qemu 4.2 has ifeq ($(CONFIG_POSIX),y) and ifdef CONFIG_POSIX statements, but the comments in hax-all.c/hax-posix.c are:

HAX common code for both windows and darwin
HAX module interface - darwin version

These comments might be out of sync.

May I assume that:
Netbsd/linux/darwin are all POSIX?

Yes.

Qemu added support for them after 2.12?

I don't remember exact versions, but everything is upstreamed for a year or so.

Netbsd/linux/darwin should all have same ioctl definitions?

Yes. This also means that please avoid using extensions for ioctl(2) definitions supported only in one or two OSs.

@nevilad nevilad force-pushed the set_cpuid_ioctl branch 4 times, most recently from 768487c to df78681 Compare March 9, 2020 11:08
@nevilad
Copy link
Contributor Author

nevilad commented Mar 9, 2020

Added POSIX support to qemu patch
0001-Set-cpuid-values-to-hax-through-set_cpuid-ioctl-when.patch.txt

There were modifications in netbsd code. I can't check if these compile.

@wcwang
Copy link
Contributor

wcwang commented Mar 9, 2020

Thanks for your contribution!

For the IOCTL interface of setting CPUID, we are also working on this feature and would like to share our patches later and discuss with you together. There is only one question from the first round review, did you consider the case that a CPUID feature is in your supported scope, i.e., is_cpuid_supported() returns true, but HAXM has not supported it logically, such as FEATURE(TSC_DEADLINE)?

@nevilad
Copy link
Contributor Author

nevilad commented Mar 9, 2020

No:

It is possible to add non-supported bits to cpuid feature bits through +command line switch in qemu. This is not handled here, but it is possible in hax to compare the received bits with hardcoded support masks and the actual CPU values to see, that all bits are supported. KVM does this not, so I've done it the same way.

The main reason - KVM does the same way. My checking code disallows to set cpuids for processors newer than Penryn, hax already supports features for this and older CPUs, except CPUID_PSE36 and CPUID_EXT3_LAHF_LM. The user has to change it's command line, when it's guest will not work.
I need this feature for MacOS support, it needs Penryn CPU. It works for me.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 9, 2020

CPUID_PSE36 (36-bit Page Size Extension) was introduced in Pentium II Xeon. I've found that it was used in Windows NT 4.0 Enterprise Edition, but newer Microsoft OSes, including Windows 2000, support only PAE. Linux skipped PSE-36 usage entirely. I dont know OSes on which it is possible to test support of this feature. I think it is safe to add it to cpuid_1_features_edx.
CPUID_EXT3_LAHF_LM means CPU is supporting lahf instruction in long mode. It it safe to add it to cpuid_8000_0001_features_ecx, since it does not need any virtualization support.
When these flags are added, hax will support all features of Penryn and older CPUs. If so, it would be possible to check the received cpuids against supported by hax and by the CPU.

I thought about adding a flag to the set_cpuid ioctl - check against real supported features or not. But KVM does this not, and as @AlexAltea mentioned, it would be good to adopt the KVM API #106 (comment) and created #121. KVM API does not have such a flag.

@krytarowski
Copy link
Contributor

I can test on NetBSD, but I need a test scenario.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 12, 2020

Intel is working on the same feature, so this PR may change.
@wcwang when you will be able to upload your patch?

Tests for this PR are:

  • build hax with this patch. Intel build does not include NetBSD, so I even don't know if it compiles on this OS.
  • run installation of a guest with -cpu Nehalem switch in the command line. Qemu should say that level 11 is not supported.
  • run installation of a guest with -cpu Penryn switch in the command line. Installation should succeed. Check the CPU in the guest. There should be the string "Intel Core 2 Duo P9xxx (Penryn Class Core 2)".

Running already installed guests with a differrent cpu is dangerous, since these can have problems with booting after such a change. I tested the feature on a windows10 x86 guest installed with the default cpu. Then changed to Penryn - it worked. Then changed to core2duo and it worked too. I began to change the cpu between these, boot the guest and check cpu name string. It looked like windows cached it, showing an older string after cpu change. When I ran the guest without -cpu switch, the guest was unable to boot and tried to run repair.

The actual qemu patch is in #271 (comment).

@krytarowski
Copy link
Contributor

OK. Once there will be a patch ready for merge, please ping me.

@wcwang
Copy link
Contributor

wcwang commented Mar 12, 2020

when you will be able to upload your patch?

The patch is under internal review and will be expected uploaded in a week.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 12, 2020

Is there any place where I can see internal Intel work list on hax? This would be usefull to prevent intersections in feature addition like here.

@wcwang
Copy link
Contributor

wcwang commented Mar 13, 2020

Is there any place where I can see internal Intel work list on hax? This would be usefull to prevent intersections in feature addition like here.

Sure, we will consider to establish a wiki page for community reference. Thanks for your suggestion.

@wcwang
Copy link
Contributor

wcwang commented Mar 20, 2020

@nevilad, we have submitted the PR #277 for CPUID feature. You may go through the patches first. Our feature is targeted to the Android Emulator configuration, and maybe it cannot cover your implementation completely. If you have any suggestion or requirement, we can discuss the features later. Thanks.

@wcwang wcwang force-pushed the master branch 2 times, most recently from 563eb1b to 6b942e3 Compare November 25, 2022 03:23
@wcwang wcwang force-pushed the master branch 2 times, most recently from b73a231 to da1b8ec Compare January 26, 2023 02:48
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CI:Build Fail CI:Build Fail
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants