Re: [PATCH] Add PAT, cr8 and EFER for 32-bit qemu to hax ioctl interface

[Colin Xu]

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 <address@hidden>
> ---
>   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, &regs);
>       } 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, &regs);
>       }
>   
> @@ -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
--
Best Regards,
Colin Xu