| Message ID | 1786823015.3514736.1582923592218@mail.yahoo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add PAT, cr8 and EFER for 32-bit qemu to hax ioctl interface | expand |
Sorry for missing this one. If I remembered correctly, this patch together with the HAXM patch in github will cause some regression in SMP case. So we'd like to fully understand the technique details to make proper change, not only for a very specific purpose, i.e. boot Windows on Windows. This patch together with PR#204 doens't change the IOCTL interface itself, but extend set/get regs with a version check, so the description here isn't quite clear. And the change looks just sync the qemu/haxm status but no more. Could you provide more details that why Windows can't boot without the change. Like CR8 (TPR), is there logic that need to be handled when TPR is read/write? On 2/29/2020 04:59, Alexey Romko wrote: > Add PAT, cr8 and EFER for 32-bit qemu to hax ioctl interface, part of HAX PR 204 > > Signed-off-by: Alexey Romko <nevilad@yahoo.com> > --- > target/i386/hax-all.c | 32 ++++++++++++++++++++++++++++---- > target/i386/hax-i386.h | 2 +- > target/i386/hax-interface.h | 2 ++ > 3 files changed, 31 insertions(+), 5 deletions(-) > > > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > index a8b6e5aeb8..0bdd309665 100644 > --- a/target/i386/hax-all.c > +++ b/target/i386/hax-all.c > @@ -45,7 +45,7 @@ > } while (0) > > /* Current version */ > -const uint32_t hax_cur_version = 0x4; /* API v4: unmapping and MMIO moves */ > +const uint32_t hax_cur_version = 0x5; /* API v5: supports CR8/EFER/PAT */ > /* Minimum HAX kernel version */ > const uint32_t hax_min_version = 0x4; /* API v4: supports unmapping */ > > @@ -137,6 +137,7 @@ static int hax_version_support(struct hax_state *hax) > return 0; > } > > + hax_global.cur_api_version = version.cur_version; > return 1; > } > > @@ -845,12 +846,24 @@ static int hax_sync_vcpu_register(CPUArchState *env, int set) > regs._cr2 = env->cr[2]; > regs._cr3 = env->cr[3]; > regs._cr4 = env->cr[4]; > + > + if( hax_global.cur_api_version >= 0x5 ) { > + CPUState *cs = env_cpu(env); > + X86CPU *x86_cpu = X86_CPU(cs); > + regs._cr8 = cpu_get_apic_tpr(x86_cpu->apic_state); > + } > + > hax_set_segments(env, ®s); > } else { > env->cr[0] = regs._cr0; > env->cr[2] = regs._cr2; > env->cr[3] = regs._cr3; > env->cr[4] = regs._cr4; > + > + //if( hax_global.cur_api_version >= 0x5 ) { > + //no need to update TPR from regs._cr8, since all changes are notified. > + //} > + > hax_get_segments(env, ®s); > } > > @@ -881,14 +894,18 @@ static int hax_get_msrs(CPUArchState *env) > msrs[n++].entry = MSR_IA32_SYSENTER_ESP; > msrs[n++].entry = MSR_IA32_SYSENTER_EIP; > msrs[n++].entry = MSR_IA32_TSC; > -#ifdef TARGET_X86_64 > msrs[n++].entry = MSR_EFER; > +#ifdef TARGET_X86_64 > msrs[n++].entry = MSR_STAR; > msrs[n++].entry = MSR_LSTAR; > msrs[n++].entry = MSR_CSTAR; > msrs[n++].entry = MSR_FMASK; > msrs[n++].entry = MSR_KERNELGSBASE; > #endif > + if( hax_global.cur_api_version >= 0x5 ) { > + msrs[n++].entry = MSR_PAT; > + } > + > md.nr_msr = n; > ret = hax_sync_msr(env, &md, 0); > if (ret < 0) { > @@ -909,10 +926,10 @@ static int hax_get_msrs(CPUArchState *env) > case MSR_IA32_TSC: > env->tsc = msrs[i].value; > break; > -#ifdef TARGET_X86_64 > case MSR_EFER: > env->efer = msrs[i].value; > break; > +#ifdef TARGET_X86_64 > case MSR_STAR: > env->star = msrs[i].value; > break; > @@ -929,6 +946,9 @@ static int hax_get_msrs(CPUArchState *env) > env->kernelgsbase = msrs[i].value; > break; > #endif > + case MSR_PAT: > + env->pat = msrs[i].value; > + break; > } > } > > @@ -947,14 +967,18 @@ static int hax_set_msrs(CPUArchState *env) > hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp); > hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip); > hax_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc); > -#ifdef TARGET_X86_64 > hax_msr_entry_set(&msrs[n++], MSR_EFER, env->efer); > +#ifdef TARGET_X86_64 > hax_msr_entry_set(&msrs[n++], MSR_STAR, env->star); > hax_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar); > hax_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar); > hax_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask); > hax_msr_entry_set(&msrs[n++], MSR_KERNELGSBASE, env->kernelgsbase); > #endif > + if( hax_global.cur_api_version >= 0x5 ) { > + hax_msr_entry_set(&msrs[n++], MSR_PAT, env->pat); > + } > + > md.nr_msr = n; > md.done = 0; > > diff --git a/target/i386/hax-i386.h b/target/i386/hax-i386.h > index 54e9d8b057..9515803184 100644 > --- a/target/i386/hax-i386.h > +++ b/target/i386/hax-i386.h > @@ -34,7 +34,7 @@ struct hax_vcpu_state { > > struct hax_state { > hax_fd fd; /* the global hax device interface */ > - uint32_t version; > + uint32_t cur_api_version; > struct hax_vm *vm; > uint64_t mem_quota; > bool supports_64bit_ramblock; > diff --git a/target/i386/hax-interface.h b/target/i386/hax-interface.h > index 537ae084e9..c87aedbc2e 100644 > --- a/target/i386/hax-interface.h > +++ b/target/i386/hax-interface.h > @@ -218,6 +218,8 @@ struct vcpu_state_t { > uint32_t _activity_state; > uint32_t pad; > interruptibility_state_t _interruptibility_state; > + > + uint64_t _cr8; > }; > > /* HAX exit status */ > -- > 2.15.0.windows.1 >
Hi, >If I remembered correctly, this patch together with the HAXM patch in github will cause some regression in SMP case Only on MacOS host. Since there are problems in logging in MacOS, we can't figure yet the source of the problem. We can find the problem by trial and error, but this can be done after haxm is released. > why Windows can't boot without the change. Windows can boot without any MSR/CR8 saves to haxm, but when you want to use snapshots you should save and restore these since windows changes and uses these. Currently snapshots are not working on windows (see https://bugs.launchpad.net/qemu/+bug/1855617). I've tested snapshots on a modified version of haxm+qemu when a agent in the guest executes vmcall instruction, this event is returned by haxm to qemu, registers and MSRs are synchronized and snapshot created. Snapshot restore is done in standart way. >Like CR8 (TPR), is there logic that need to be handled when TPR is read/write? I've created CR8 read/write handling in haxm, but it is not yet submitted in a PR. Originally the haxm PR was for x86 and x64 windows, but the x64 did not work. I decided to do a one time interface change and not commit all these changes by pieces. CR8 is needed by MacOS too. I plan to submit a PR for it's support after haxm release too. Colin Xu <colin.xu@intel.com> wrote: Sorry for missing this one. If I remembered correctly, this patch together with the HAXM patch in github will cause some regression in SMP case. So we'd like to fully understand the technique details to make proper change, not only for a very specific purpose, i.e. boot Windows on Windows. This patch together with PR#204 doens't change the IOCTL interface itself, but extend set/get regs with a version check, so the description here isn't quite clear. And the change looks just sync the qemu/haxm status but no more. Could you provide more details that why Windows can't boot without the change. Like CR8 (TPR), is there logic that need to be handled when TPR is read/write? On 2/29/2020 04:59, Alexey Romko wrote: > Add PAT, cr8 and EFER for 32-bit qemu to hax ioctl interface, part of HAX PR 204 > > Signed-off-by: Alexey Romko <nevilad@yahoo.com> > --- > target/i386/hax-all.c | 32 ++++++++++++++++++++++++++++---- > target/i386/hax-i386.h | 2 +- > target/i386/hax-interface.h | 2 ++ > 3 files changed, 31 insertions(+), 5 deletions(-) > > > diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c > index a8b6e5aeb8..0bdd309665 100644 > --- a/target/i386/hax-all.c > +++ b/target/i386/hax-all.c > @@ -45,7 +45,7 @@ > } while (0) > > /* Current version */ > -const uint32_t hax_cur_version = 0x4; /* API v4: unmapping and MMIO moves */ > +const uint32_t hax_cur_version = 0x5; /* API v5: supports CR8/EFER/PAT */ > /* Minimum HAX kernel version */ > const uint32_t hax_min_version = 0x4; /* API v4: supports unmapping */ > > @@ -137,6 +137,7 @@ static int hax_version_support(struct hax_state *hax) > return 0; > } > > + hax_global.cur_api_version = version.cur_version; > return 1; > } > > @@ -845,12 +846,24 @@ static int hax_sync_vcpu_register(CPUArchState *env, int set) > regs._cr2 = env->cr[2]; > regs._cr3 = env->cr[3]; > regs._cr4 = env->cr[4]; > + > + if( hax_global.cur_api_version >= 0x5 ) { > + CPUState *cs = env_cpu(env); > + X86CPU *x86_cpu = X86_CPU(cs); > + regs._cr8 = cpu_get_apic_tpr(x86_cpu->apic_state); > + } > + > hax_set_segments(env, ®s); > } else { > env->cr[0] = regs._cr0; > env->cr[2] = regs._cr2; > env->cr[3] = regs._cr3; > env->cr[4] = regs._cr4; > + > + //if( hax_global.cur_api_version >= 0x5 ) { > + //no need to update TPR from regs._cr8, since all changes are notified. > + //} > + > hax_get_segments(env, ®s); > } > > @@ -881,14 +894,18 @@ static int hax_get_msrs(CPUArchState *env) > msrs[n++].entry = MSR_IA32_SYSENTER_ESP; > msrs[n++].entry = MSR_IA32_SYSENTER_EIP; > msrs[n++].entry = MSR_IA32_TSC; > -#ifdef TARGET_X86_64 > msrs[n++].entry = MSR_EFER; > +#ifdef TARGET_X86_64 > msrs[n++].entry = MSR_STAR; > msrs[n++].entry = MSR_LSTAR; > msrs[n++].entry = MSR_CSTAR; > msrs[n++].entry = MSR_FMASK; > msrs[n++].entry = MSR_KERNELGSBASE; > #endif > + if( hax_global.cur_api_version >= 0x5 ) { > + msrs[n++].entry = MSR_PAT; > + } > + > md.nr_msr = n; > ret = hax_sync_msr(env, &md, 0); > if (ret < 0) { > @@ -909,10 +926,10 @@ static int hax_get_msrs(CPUArchState *env) > case MSR_IA32_TSC: > env->tsc = msrs[i].value; > break; > -#ifdef TARGET_X86_64 > case MSR_EFER: > env->efer = msrs[i].value; > break; > +#ifdef TARGET_X86_64 > case MSR_STAR: > env->star = msrs[i].value; > break; > @@ -929,6 +946,9 @@ static int hax_get_msrs(CPUArchState *env) > env->kernelgsbase = msrs[i].value; > break; > #endif > + case MSR_PAT: > + env->pat = msrs[i].value; > + break; > } > } > > @@ -947,14 +967,18 @@ static int hax_set_msrs(CPUArchState *env) > hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp); > hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip); > hax_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc); > -#ifdef TARGET_X86_64 > hax_msr_entry_set(&msrs[n++], MSR_EFER, env->efer); > +#ifdef TARGET_X86_64 > hax_msr_entry_set(&msrs[n++], MSR_STAR, env->star); > hax_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar); > hax_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar); > hax_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask); > hax_msr_entry_set(&msrs[n++], MSR_KERNELGSBASE, env->kernelgsbase); > #endif > + if( hax_global.cur_api_version >= 0x5 ) { > + hax_msr_entry_set(&msrs[n++], MSR_PAT, env->pat); > + } > + > md.nr_msr = n; > md.done = 0; > > diff --git a/target/i386/hax-i386.h b/target/i386/hax-i386.h > index 54e9d8b057..9515803184 100644 > --- a/target/i386/hax-i386.h > +++ b/target/i386/hax-i386.h > @@ -34,7 +34,7 @@ struct hax_vcpu_state { > > struct hax_state { > hax_fd fd; /* the global hax device interface */ > - uint32_t version; > + uint32_t cur_api_version; > struct hax_vm *vm; > uint64_t mem_quota; > bool supports_64bit_ramblock; > diff --git a/target/i386/hax-interface.h b/target/i386/hax-interface.h > index 537ae084e9..c87aedbc2e 100644 > --- a/target/i386/hax-interface.h > +++ b/target/i386/hax-interface.h > @@ -218,6 +218,8 @@ struct vcpu_state_t { > uint32_t _activity_state; > uint32_t pad; > interruptibility_state_t _interruptibility_state; > + > + uint64_t _cr8; > }; > > /* HAX exit status */ > -- > 2.15.0.windows.1 >
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c index a8b6e5aeb8..0bdd309665 100644 --- a/target/i386/hax-all.c +++ b/target/i386/hax-all.c @@ -45,7 +45,7 @@ } while (0) /* Current version */ -const uint32_t hax_cur_version = 0x4; /* API v4: unmapping and MMIO moves */ +const uint32_t hax_cur_version = 0x5; /* API v5: supports CR8/EFER/PAT */ /* Minimum HAX kernel version */ const uint32_t hax_min_version = 0x4; /* API v4: supports unmapping */ @@ -137,6 +137,7 @@ static int hax_version_support(struct hax_state *hax) return 0; } + hax_global.cur_api_version = version.cur_version; return 1; } @@ -845,12 +846,24 @@ static int hax_sync_vcpu_register(CPUArchState *env, int set) regs._cr2 = env->cr[2]; regs._cr3 = env->cr[3]; regs._cr4 = env->cr[4]; + + if( hax_global.cur_api_version >= 0x5 ) { + CPUState *cs = env_cpu(env); + X86CPU *x86_cpu = X86_CPU(cs); + regs._cr8 = cpu_get_apic_tpr(x86_cpu->apic_state); + } + hax_set_segments(env, ®s); } else { env->cr[0] = regs._cr0; env->cr[2] = regs._cr2; env->cr[3] = regs._cr3; env->cr[4] = regs._cr4; + + //if( hax_global.cur_api_version >= 0x5 ) { + //no need to update TPR from regs._cr8, since all changes are notified. + //} + hax_get_segments(env, ®s); } @@ -881,14 +894,18 @@ static int hax_get_msrs(CPUArchState *env) msrs[n++].entry = MSR_IA32_SYSENTER_ESP; msrs[n++].entry = MSR_IA32_SYSENTER_EIP; msrs[n++].entry = MSR_IA32_TSC; -#ifdef TARGET_X86_64 msrs[n++].entry = MSR_EFER; +#ifdef TARGET_X86_64 msrs[n++].entry = MSR_STAR; msrs[n++].entry = MSR_LSTAR; msrs[n++].entry = MSR_CSTAR; msrs[n++].entry = MSR_FMASK; msrs[n++].entry = MSR_KERNELGSBASE; #endif + if( hax_global.cur_api_version >= 0x5 ) { + msrs[n++].entry = MSR_PAT; + } + md.nr_msr = n; ret = hax_sync_msr(env, &md, 0); if (ret < 0) { @@ -909,10 +926,10 @@ static int hax_get_msrs(CPUArchState *env) case MSR_IA32_TSC: env->tsc = msrs[i].value; break; -#ifdef TARGET_X86_64 case MSR_EFER: env->efer = msrs[i].value; break; +#ifdef TARGET_X86_64 case MSR_STAR: env->star = msrs[i].value; break; @@ -929,6 +946,9 @@ static int hax_get_msrs(CPUArchState *env) env->kernelgsbase = msrs[i].value; break; #endif + case MSR_PAT: + env->pat = msrs[i].value; + break; } } @@ -947,14 +967,18 @@ static int hax_set_msrs(CPUArchState *env) hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_ESP, env->sysenter_esp); hax_msr_entry_set(&msrs[n++], MSR_IA32_SYSENTER_EIP, env->sysenter_eip); hax_msr_entry_set(&msrs[n++], MSR_IA32_TSC, env->tsc); -#ifdef TARGET_X86_64 hax_msr_entry_set(&msrs[n++], MSR_EFER, env->efer); +#ifdef TARGET_X86_64 hax_msr_entry_set(&msrs[n++], MSR_STAR, env->star); hax_msr_entry_set(&msrs[n++], MSR_LSTAR, env->lstar); hax_msr_entry_set(&msrs[n++], MSR_CSTAR, env->cstar); hax_msr_entry_set(&msrs[n++], MSR_FMASK, env->fmask); hax_msr_entry_set(&msrs[n++], MSR_KERNELGSBASE, env->kernelgsbase); #endif + if( hax_global.cur_api_version >= 0x5 ) { + hax_msr_entry_set(&msrs[n++], MSR_PAT, env->pat); + } + md.nr_msr = n; md.done = 0; diff --git a/target/i386/hax-i386.h b/target/i386/hax-i386.h index 54e9d8b057..9515803184 100644 --- a/target/i386/hax-i386.h +++ b/target/i386/hax-i386.h @@ -34,7 +34,7 @@ struct hax_vcpu_state { struct hax_state { hax_fd fd; /* the global hax device interface */ - uint32_t version; + uint32_t cur_api_version; struct hax_vm *vm; uint64_t mem_quota; bool supports_64bit_ramblock; diff --git a/target/i386/hax-interface.h b/target/i386/hax-interface.h index 537ae084e9..c87aedbc2e 100644 --- a/target/i386/hax-interface.h +++ b/target/i386/hax-interface.h @@ -218,6 +218,8 @@ struct vcpu_state_t { uint32_t _activity_state; uint32_t pad; interruptibility_state_t _interruptibility_state; + + uint64_t _cr8; }; /* HAX exit status */
Add PAT, cr8 and EFER for 32-bit qemu to hax ioctl interface, part of HAX PR 204 Signed-off-by: Alexey Romko <nevilad@yahoo.com> --- target/i386/hax-all.c | 32 ++++++++++++++++++++++++++++---- target/i386/hax-i386.h | 2 +- target/i386/hax-interface.h | 2 ++ 3 files changed, 31 insertions(+), 5 deletions(-) -- 2.15.0.windows.1