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

Add support for multicore Windows 10 guests #272

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

Conversation

nevilad
Copy link
Contributor

@nevilad nevilad commented Mar 10, 2020

When passing host cache values to the guest in cpuid 4, change maximum number of addresable IDs according to the number of virtual CPUs.

There was a comment:
// [31:26] cores per package - 1
Are there any comments why hax assumed 1 core per package?

@nevilad nevilad mentioned this pull request Mar 10, 2020
@wcwang
Copy link
Contributor

wcwang commented Mar 11, 2020

Thanks for your patch. We have verified that the Windows guest can boot up with single core as below.

qemu-system-x86_64.exe -L C:\Binaries\qemu_pr204 -name win10x86 -cpu Broadwell -machine pc-i440fx-3.0 -usb -device usb-tablet -m 4G -vga std -rtc base=localtime -accel hax -cdrom C:\Binaries\windows\Win10_1903_V1_English_x86.iso C:\Binaries\windows\win1032.qcow2

haxm_warning: CPUID 4.0, eax 0x1c004121, ebx 0x1c0003f, ecx 0x3f, edx 0x0
haxm_warning: CPUID 4:0, eax 0x4121, ebx 0x1c0003f, ecx 0x3f, edx 0x0 AFTER

However, when specifying multiple cores, the Windows guest ran into BSOD as before.

qemu-system-x86_64.exe -L C:\Binaries\qemu_pr204 -name win10x86 -cpu Broadwell -smp 2,cores=2,threads=1 -machine pc-i440fx-3.0 -usb -device usb-tablet -m 4G -vga std -rtc base=localtime -accel hax -cdrom C:\Binaries\windows\Win10_1903_V1_English_x86.iso C:\Binaries\windows\win1032.qcow2

haxm_warning: CPUID 4.0, eax 0x1c004121, ebx 0x1c0003f, ecx 0x3f, edx 0x0
haxm_warning: CPUID 4:0, eax 0x40004121, ebx 0x1c0003f, ecx 0x3f, edx 0x0 AFTER

@nevilad
Copy link
Contributor Author

nevilad commented Mar 11, 2020

The original version, which did not modify host values, did not work for both 1 core and multiple cores?
This version, which modifies addresable IDs, works on 1 core, but not multiple?
The code in this PR is done as in qemu. This means that Windows on multicore systems may check other things. I was unable to figure it out yet and need time to investigate it.

@wcwang
Copy link
Contributor

wcwang commented Mar 12, 2020

The original version, which did not modify host values, did not work for both 1 core and multiple cores?

We verified that the original version also had same issue, which will result in BSOD on multiple cores.

This version, which modifies addresable IDs, works on 1 core, but not multiple?

Whether it is patched with PR #272 or not, the current version on master branch works on 1 core, but not on multiple cores.

The code in this PR is done as in qemu. This means that Windows on multicore systems may check other things. I was unable to figure it out yet and need time to investigate it.

Thanks for your effort.

of virtual CPUs

Signed-off-by: Alexey Romko <nevilad@yahoo.com>
@nevilad
Copy link
Contributor Author

nevilad commented Mar 14, 2020

Now it works on 2 cores.
There was a comment:
// [31:26] cores per package - 1
Are there any comments why hax assumed 1 core per package?

@wcwang
Copy link
Contributor

wcwang commented Mar 16, 2020

We had searched the development history but found no clues. I think maybe the earlier versions considered the complexity for this case. From vCPU perspective, each time a vCPU may run as a different physical host CPU. The comments here may be to simplify implementation. We are verifying your update and will continue to discuss it in details. Thanks.

@wcwang
Copy link
Contributor

wcwang commented Mar 19, 2020

We have tested the patch and it did work on Windows. Multiple cores can be supported and Windows guest can boot up. However, when we tested HAXM on macOS, since merging PR #204, whether pulling the patch in PR #272 or not, all guests cannot be launched with multiple cores, even running an empty QEMU as below.

$ .\qemu-system-x86_64 -chardev stdio,id=seabios -device isa-debugcon,iobase=0x402,chardev=seabios -no-reboot -smp cores=2 -accel hax

The output of QEMU has been attached here for your reference. Thanks.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 19, 2020

I have no MacOs to debug, so will ask some questions:

since merging PR #204, whether pulling the patch in PR #272 or not, all guests cannot be launched with multiple cores, even running an empty QEMU as below.

Does an empty QEMU with multiple cores work without PR #204?

The output of QEMU has been attached here for your reference. Thanks.

Whats happening with the VM - it exits or writes something to the screen? Or crashes? Can you attach hax log with HAX_LOGD output? The problem should be in SeaBios, since it is the only code that runs.

@wcwang
Copy link
Contributor

wcwang commented Mar 20, 2020

Does an empty QEMU with multiple cores work without PR #204?

Yes, it can boot up without PR #242

Whats happening with the VM - it exits or writes something to the screen? Or crashes?

The results were random. Seldom QEMU exited quickly after launching. The guest screen flashed once and there was no impact on host. In most cases, QEMU launched and the text "Guest has not initialized the display (yet)" was printed in the QEMU window.

Can you attach hax log with HAX_LOGD output?

The log mechanism on macOS still has some problems. We will attach the available logs after fixing the issue. Thanks.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 20, 2020

Yes, it can boot up without PR #242

You mean PR #272?

The log mechanism on macOS still has some problems. We will attach the available logs after fixing the issue. Thanks.

When it will be fixed?
Can you run hax on windows host with an empty guest and same BIOS (seaBios)?

@nevilad nevilad changed the title Correct cpuid 4 output Add support for multicore Windows 10 guests Mar 20, 2020
Comment on lines +2693 to +2701
struct vm_t *vm = vcpu->vm;
hax_list_head *list;
int vcpu_count = 0;

hax_mutex_lock(vm->vm_lock);
hax_list_for_each(list, (hax_list_head *)(&vm->vcpu_list)) {
vcpu_count++;
}
hax_mutex_unlock(vm->vm_lock);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should be moved to a function.

@wcwang
Copy link
Contributor

wcwang commented Mar 23, 2020

You mean PR #272?

Sorry for typo. It is PR #204. QEMU can only boot up without PR #204. Even after applying the latest update of PR #272, QEMU cannot boot up with multiple cores on macOS yet.

When it will be fixed?

As we are working on the next release, maybe this issue will be postponed to next month.

Can you run hax on windows host with an empty guest and same BIOS (seaBios)?

Yes, QEMU can boot up for multiple cores with an empty guest and same BIOS on Windows host under below cases:

Furthermore, with PR #204, after I reverted the patch 329dc40 (Add cr8 to hax ioctl interface), QEMU can boot up for multiple cores with an empty guest on macOS.

According to our release schedule, we will consider to resolve the multiple cores issue after the next release. In order to avoid the regressions on macOS, maybe I have to revert the cr8 patch first. Thanks for your understanding.

@nevilad
Copy link
Contributor Author

nevilad commented Mar 23, 2020

In order to avoid the regressions on macOS, maybe I have to revert the cr8 patch first.

I think it is better not to merge this PR to have working MacOS. This will lead to nonworking multicore WIndows 10 guest, but it did not work in previous versions too. Cr8 support is vitable for supporting Win10 x64 and MacOS, so I recommend not to remove it.

When do you plan to release new haxm version (how many time do I have to debug this issue)?

@wcwang
Copy link
Contributor

wcwang commented Mar 24, 2020

I think it is better not to merge this PR to have working MacOS. This will lead to nonworking multicore WIndows 10 guest, but it did not work in previous versions too.

Yes, macOS did not work even after applying this patch.

Cr8 support is vitable for supporting Win10 x64 and MacOS, so I recommend not to remove it.

Not true. As I mentioned, after merging PR #204 and reverting the patch 329dc40 , QEMU can boot up for multiple cores with an empty guest on both Windows and macOS. Originally, QEMU can boot up for multiple cores before merging PR #204 . Therefore, there is a regression after merging CR8 patch.

When do you plan to release new haxm version (how many time do I have to debug this issue)?

Today is the last day to deliver release and we also need about 2 weeks to complete release test. Never mind, once the multiple cores issue resolved, we can make another release for you. Thanks for your understanding and your future contribution will be welcome.

@wayne-ma
Copy link
Contributor

Ok to verify

@nevilad
Copy link
Contributor Author

nevilad commented Apr 21, 2020

Haxm is released, we can continue with this PR?
We found the problem is in cr8 handling commit 329dc40. I think we can find the problem by commenting out some parts of the commit.
@wcwang can you build hax from #288 and ry to run under MacOS? I removed cr8 from vcpu there, I think structure layout changes may be the source of the problem, since cr8 in the mentioned commit is only saved from user mode, but not used.

@wcwang
Copy link
Contributor

wcwang commented Apr 22, 2020

Thanks for your tracking. Currently, our team is busy in improving the performance on Windows. I think we can continue with this PR but the time may be delayed a bit. I will try to build HAXM from #288 on macOS later. Thanks again.

@nevilad
Copy link
Contributor Author

nevilad commented Jun 10, 2020

@wcwang when does your team finish with improving the performance on Windows and continue with this PR?

@wcwang
Copy link
Contributor

wcwang commented Jun 12, 2020

@nevilad, thanks for your tracking. Recently, we put lots of effort on debugging the performance issue on Windows but have not found the solution. We expect HAXM will not be impacted even when L1TF mitigation is enabled. Sorry for not collaborating with you to make the this pull request move on. Thanks for your understanding.

// Bit mask to identify the reserved bits in paging structure high
// order address field
physical_address_size = (uint8_t)state->_eax & 0xff;
pw_reserved_bits_high_mask =
~((1 << (physical_address_size - 32)) - 1);

state->_ebx = state->_ecx = state->_edx = 0;

if (vcpu_count > 1)
state->_ecx |= vcpu_count - 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

According to Intel SDM, when the initial EAX value is 0x80000008, ECX is reserved to 0. Why set state->_ecx here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, SDM says it's reserved to 0. I've done this since qemu does the same:

    case 0x80000008:
        /* virtual & phys address size in low 2 bytes. */
        if (env->features[FEAT_8000_0001_EDX] & CPUID_EXT2_LM) {
            /* 64 bit processor */
            *eax = cpu->phys_bits; /* configurable physical bits */
            if  (env->features[FEAT_7_0_ECX] & CPUID_7_0_ECX_LA57) {
                *eax |= 0x00003900; /* 57 bits virtual */
            } else {
                *eax |= 0x00003000; /* 48 bits virtual */
            }
        } else {
            *eax = cpu->phys_bits;
        }
        *ebx = env->features[FEAT_8000_0008_EBX];
        *ecx = 0;
        *edx = 0;
        if (cs->nr_cores * cs->nr_threads > 1) {
            *ecx |= (cs->nr_cores * cs->nr_threads) - 1;
        }
        break;

@nevilad
Copy link
Contributor Author

nevilad commented Aug 6, 2020

@wcwang how is your work on the performance issue on Windows going?

@nevilad
Copy link
Contributor Author

nevilad commented Jan 6, 2021

Can we continue with this PR?

@wcwang
Copy link
Contributor

wcwang commented Jan 7, 2021

Thanks for @nevilad's tracking. Currently we are working on some new features in CPUID module. Anyway, I would like to collaborate with you to continue with the PR if possible. Have you looked through the new implementation of CPUID module? Could you re-implement this feature based on the latest code? Then I will try to review and verify it when ready. Thanks a ton again.

@nevilad
Copy link
Contributor Author

nevilad commented Jan 8, 2021

The new code is OK, but it does not allow me to use subleafs. Do you plan to add this support or should I do that? It's about changing cpuid_manager_t - add subleaf and flags.

@wcwang
Copy link
Contributor

wcwang commented Jan 8, 2021

Thanks for your review. Maybe currently we do not have such plan to enhance CPUID features. Your idea sounds quite great, but maybe it will change a lot of framework code. Our team is working on another feature of Android, I think we'd better choose another time to continue this PR. During this period, I think you may consider your implementation first and then we can discuss later. Thanks for your understanding.

@nevilad
Copy link
Contributor Author

nevilad commented Jan 8, 2021

OK, will wait till you're ready with the feature!

@HaxmCI HaxmCI added CI:Build Pass CI:Build Pass CI:Mac Test Pass CI:Mac Test Pass labels Mar 2, 2021
@nevilad
Copy link
Contributor Author

nevilad commented Apr 12, 2021

I see a new haxm is released, can we continue with this PR?

@wcwang
Copy link
Contributor

wcwang commented Apr 13, 2021

@nevilad, thanks for your tracking. You may also notice that we still have a release to be delivered, i.e., v7.7.0. Our current task priority is still in the feature of the Android guest OS. When we have plans to optimize the Windows guest, we will consider this PR at the first time. Thanks for your understanding.

@nevilad
Copy link
Contributor Author

nevilad commented Apr 13, 2021

When do you plan to release v7.7.0? Can we continue with this PR after v7.7.0 release?

@nevilad
Copy link
Contributor Author

nevilad commented Apr 21, 2021

@wcwang When do you plan to release v7.7.0? Can we continue with this PR after v7.7.0 release?

@wcwang
Copy link
Contributor

wcwang commented Apr 21, 2021

@nevilad, sorry for late response. Maybe we will postpone the release v7.7.0 as there is another snapshot issue of Android guest. After this release, our task priority may still be in the feature of the Android guest OS. I am not sure when we can continue this PR. Anyway, thanks for your tracking and contributions.

@HaxmCI HaxmCI added CI:Build Fail CI:Build Fail and removed CI:Build Pass CI:Build Pass CI:Mac Test Pass CI:Mac Test Pass labels May 24, 2021
@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.

5 participants