Re: [PATCH v1 1/3] hvf: use standard CR0 and CR4 register definitions

[Roman Bolshakov]

On Mon, Mar 30, 2020 at 05:16:04PM -0700, Cameron Esfahani wrote:
> Signed-off-by: Cameron Esfahani <address@hidden>
> ---
>  target/i386/cpu.h          |  2 ++
>  target/i386/hvf/hvf.c      |  1 +
>  target/i386/hvf/vmx.h      | 15 ++++++++-------
>  target/i386/hvf/x86.c      |  6 +++---
>  target/i386/hvf/x86.h      | 34 ----------------------------------
>  target/i386/hvf/x86_mmu.c  |  2 +-
>  target/i386/hvf/x86_task.c |  3 ++-
>  7 files changed, 17 insertions(+), 46 deletions(-)
> 

Hi Cameron,

I'm sorry for delay.

This is fun, I had very similar changeset I forgot to send quite a while
ago:
https://github.com/roolebo/qemu/commits/hvf-common-cr-constants

> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index d72543dc31..fef1ee7d70 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -455,6 +455,7 @@ void hvf_reset_vcpu(CPUState *cpu) {
>          wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
>      }
>  
> +    macvm_set_cr0(cpu->hvf_fd, CR0_CD_MASK | CR0_NW_MASK | CR0_ET_MASK);
>      macvm_set_cr0(cpu->hvf_fd, 0x60000010);

The second macvm_set_cr0() is a duplicate of the first one and should be
dropped.

>  
>      wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK);
> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
> index 03d2c79b9c..8ec2e6414e 100644
> --- a/target/i386/hvf/vmx.h
> +++ b/target/i386/hvf/vmx.h
> @@ -138,17 +139,17 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, 
> uint64_t cr0)
>      wvmcs(vcpu, VMCS_CR0_SHADOW, cr0);
>  
>      if (efer & MSR_EFER_LME) {
> -        if (!(old_cr0 & CR0_PG) && (cr0 & CR0_PG)) {
> +        if (!(old_cr0 & CR0_PG_MASK) && (cr0 & CR0_PG_MASK)) {
>              enter_long_mode(vcpu, cr0, efer);
>          }
> -        if (/*(old_cr0 & CR0_PG) &&*/ !(cr0 & CR0_PG)) {
> +        if (!(cr0 & CR0_PG_MASK)) {

IMO the patch should only change CR0_PG to CR0_PG_MASK without removal
of the commented condition.

In the next patch you're improving how long mode exit is done and
replacement of the comment with an implementation fits better there.

>              exit_long_mode(vcpu, cr0, efer);
>          }
>      }
>  
>      /* Filter new CR0 after we are finished examining it above. */
> -    cr0 = (cr0 & ~(mask & ~CR0_PG));
> -    wvmcs(vcpu, VMCS_GUEST_CR0, cr0 | CR0_NE | CR0_ET);
> +    cr0 = (cr0 & ~(mask & ~CR0_PG_MASK));
> +    wvmcs(vcpu, VMCS_GUEST_CR0, cr0 | CR0_NE_MASK | CR0_ET_MASK);
>  
>      hv_vcpu_invalidate_tlb(vcpu);
>      hv_vcpu_flush(vcpu);
> -- 
> 2.24.0
> 

Best regards,
Roman