[PATCH v1 0/3] hvf: Support AVX512 guests and cleanup

[PATCH v1 0/3] hvf: Support AVX512 guests and cleanup

[Cameron Esfahani via]

HVF had its own copy of the CR0 and CR4 register definitions.  Remove
them in favor of the definitions in target/i386/cpu.h.

Change long mode enter and exit code to be clearer.

Support AVX512 guests on capable hardware.  This involves two separate
changes:
- Correctly manage the OSXSAVE bit in CPUID[0x01].  cpu_x86_cpuid()
  attempts to account for OSXSAVE, but it refers to env->cr[4] for the
  guest copy of CR4.  That field isn't up to date under HVF.  Instead,
  we track OSXSAVE separately, by adding OSXSAVE to CR4 mask and saving
  the state.  Then, when handling CPUID[0x01] in EXIT_REASON_CPUID, we
  reflect the current state of CR4[OSXSAVE]. 
- macOS lazily enables AVX512 for processes.  Explicitly enable AVX512
  for QEMU.
With these two changes, guests can correctly detect and enable AVX512.

Cameron Esfahani (3):
  hvf: use standard CR0 and CR4 register definitions
  hvf: Make long mode enter and exit code clearer.
  hvf: Support AVX512 guests on capable hardware

 target/i386/cpu.h          |  3 ++
 target/i386/hvf/hvf.c      | 69 ++++++++++++++++++++++++++++++++++++--
 target/i386/hvf/vmx.h      | 32 ++++++++++++------
 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 +-
 target/i386/hvf/x86hvf.c   |  2 +-
 8 files changed, 98 insertions(+), 53 deletions(-)

-- 
2.24.0

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

[Cameron Esfahani via]

Signed-off-by: Cameron Esfahani <dirty@apple.com>
---
 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(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 60d797d594..1286ec6e7a 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -225,6 +225,8 @@ typedef enum X86Seg {
 #define CR0_NE_MASK  (1U << 5)
 #define CR0_WP_MASK  (1U << 16)
 #define CR0_AM_MASK  (1U << 18)
+#define CR0_NW_MASK  (1U << 29)
+#define CR0_CD_MASK  (1U << 30)
 #define CR0_PG_MASK  (1U << 31)
 
 #define CR4_VME_MASK  (1U << 0)
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);
 
     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
@@ -121,9 +121,10 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
     uint64_t pdpte[4] = {0, 0, 0, 0};
     uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER);
     uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0);
-    uint64_t mask = CR0_PG | CR0_CD | CR0_NW | CR0_NE | CR0_ET;
+    uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK |
+                    CR0_NE_MASK | CR0_ET_MASK;
 
-    if ((cr0 & CR0_PG) && (rvmcs(vcpu, VMCS_GUEST_CR4) & CR4_PAE) &&
+    if ((cr0 & CR0_PG_MASK) && (rvmcs(vcpu, VMCS_GUEST_CR4) & CR4_PAE_MASK) &&
         !(efer & MSR_EFER_LME)) {
         address_space_read(&address_space_memory,
                            rvmcs(vcpu, VMCS_GUEST_CR3) & ~0x1f,
@@ -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)) {
             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);
@@ -156,7 +157,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
 
 static inline void macvm_set_cr4(hv_vcpuid_t vcpu, uint64_t cr4)
 {
-    uint64_t guest_cr4 = cr4 | CR4_VMXE;
+    uint64_t guest_cr4 = cr4 | CR4_VMXE_MASK;
 
     wvmcs(vcpu, VMCS_GUEST_CR4, guest_cr4);
     wvmcs(vcpu, VMCS_CR4_SHADOW, cr4);
diff --git a/target/i386/hvf/x86.c b/target/i386/hvf/x86.c
index 3afcedc7fc..668c02de6e 100644
--- a/target/i386/hvf/x86.c
+++ b/target/i386/hvf/x86.c
@@ -119,7 +119,7 @@ bool x86_read_call_gate(struct CPUState *cpu, struct x86_call_gate *idt_desc,
 bool x86_is_protected(struct CPUState *cpu)
 {
     uint64_t cr0 = rvmcs(cpu->hvf_fd, VMCS_GUEST_CR0);
-    return cr0 & CR0_PE;
+    return cr0 & CR0_PE_MASK;
 }
 
 bool x86_is_real(struct CPUState *cpu)
@@ -150,13 +150,13 @@ bool x86_is_long64_mode(struct CPUState *cpu)
 bool x86_is_paging_mode(struct CPUState *cpu)
 {
     uint64_t cr0 = rvmcs(cpu->hvf_fd, VMCS_GUEST_CR0);
-    return cr0 & CR0_PG;
+    return cr0 & CR0_PG_MASK;
 }
 
 bool x86_is_pae_enabled(struct CPUState *cpu)
 {
     uint64_t cr4 = rvmcs(cpu->hvf_fd, VMCS_GUEST_CR4);
-    return cr4 & CR4_PAE;
+    return cr4 & CR4_PAE_MASK;
 }
 
 target_ulong linear_addr(struct CPUState *cpu, target_ulong addr, X86Seg seg)
diff --git a/target/i386/hvf/x86.h b/target/i386/hvf/x86.h
index c95d5b2116..bc0170b2a8 100644
--- a/target/i386/hvf/x86.h
+++ b/target/i386/hvf/x86.h
@@ -100,40 +100,6 @@ typedef struct x86_reg_flags {
     };
 } __attribute__ ((__packed__)) x86_reg_flags;
 
-typedef enum x86_reg_cr0 {
-    CR0_PE =            (1L << 0),
-    CR0_MP =            (1L << 1),
-    CR0_EM =            (1L << 2),
-    CR0_TS =            (1L << 3),
-    CR0_ET =            (1L << 4),
-    CR0_NE =            (1L << 5),
-    CR0_WP =            (1L << 16),
-    CR0_AM =            (1L << 18),
-    CR0_NW =            (1L << 29),
-    CR0_CD =            (1L << 30),
-    CR0_PG =            (1L << 31),
-} x86_reg_cr0;
-
-typedef enum x86_reg_cr4 {
-    CR4_VME =            (1L << 0),
-    CR4_PVI =            (1L << 1),
-    CR4_TSD =            (1L << 2),
-    CR4_DE  =            (1L << 3),
-    CR4_PSE =            (1L << 4),
-    CR4_PAE =            (1L << 5),
-    CR4_MSE =            (1L << 6),
-    CR4_PGE =            (1L << 7),
-    CR4_PCE =            (1L << 8),
-    CR4_OSFXSR =         (1L << 9),
-    CR4_OSXMMEXCPT =     (1L << 10),
-    CR4_VMXE =           (1L << 13),
-    CR4_SMXE =           (1L << 14),
-    CR4_FSGSBASE =       (1L << 16),
-    CR4_PCIDE =          (1L << 17),
-    CR4_OSXSAVE =        (1L << 18),
-    CR4_SMEP =           (1L << 20),
-} x86_reg_cr4;
-
 /* 16 bit Task State Segment */
 typedef struct x86_tss_segment16 {
     uint16_t link;
diff --git a/target/i386/hvf/x86_mmu.c b/target/i386/hvf/x86_mmu.c
index 65d4603dbf..8f38eccffc 100644
--- a/target/i386/hvf/x86_mmu.c
+++ b/target/i386/hvf/x86_mmu.c
@@ -130,7 +130,7 @@ static bool test_pt_entry(struct CPUState *cpu, struct gpt_translation *pt,
 
     uint32_t cr0 = rvmcs(cpu->hvf_fd, VMCS_GUEST_CR0);
     /* check protection */
-    if (cr0 & CR0_WP) {
+    if (cr0 & CR0_WP_MASK) {
         if (pt->write_access && !pte_write_access(pte)) {
             return false;
         }
diff --git a/target/i386/hvf/x86_task.c b/target/i386/hvf/x86_task.c
index 1daac6cc2b..5e41d09b89 100644
--- a/target/i386/hvf/x86_task.c
+++ b/target/i386/hvf/x86_task.c
@@ -174,7 +174,8 @@ void vmx_handle_task_switch(CPUState *cpu, x68_segment_selector tss_sel, int rea
         //ret = task_switch_16(cpu, tss_sel, old_tss_sel, old_tss_base, &next_tss_desc);
         VM_PANIC("task_switch_16");
 
-    macvm_set_cr0(cpu->hvf_fd, rvmcs(cpu->hvf_fd, VMCS_GUEST_CR0) | CR0_TS);
+    macvm_set_cr0(cpu->hvf_fd,
+                  rvmcs(cpu->hvf_fd, VMCS_GUEST_CR0) | CR0_TS_MASK);
     x86_segment_descriptor_to_vmx(cpu, tss_sel, &next_tss_desc, &vmx_seg);
     vmx_write_segment_descriptor(cpu, &vmx_seg, R_TR);
 
-- 
2.24.0

[PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.

[Cameron Esfahani via]

Signed-off-by: Cameron Esfahani <dirty@apple.com>
---
 target/i386/hvf/vmx.h | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index 8ec2e6414e..1a1b150c97 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
     uint64_t pdpte[4] = {0, 0, 0, 0};
     uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER);
     uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0);
+    uint64_t changed_cr0 = old_cr0 ^ cr0;
     uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK |
                     CR0_NE_MASK | CR0_ET_MASK;
 
@@ -139,11 +140,12 @@ 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_MASK) && (cr0 & CR0_PG_MASK)) {
-            enter_long_mode(vcpu, cr0, efer);
-        }
-        if (!(cr0 & CR0_PG_MASK)) {
-            exit_long_mode(vcpu, cr0, efer);
+        if (changed_cr0 & CR0_PG_MASK) {
+            if (cr0 & CR0_PG_MASK) {
+                enter_long_mode(vcpu, cr0, efer);
+            } else {
+                exit_long_mode(vcpu, cr0, efer);
+            }
         }
     }
 
-- 
2.24.0

[PATCH v1 3/3] hvf: Support AVX512 guests on capable hardware

[Cameron Esfahani via]

macOS lazily enables AVX512.  Explicitly enable it if the processor
supports it.

cpu_x86_cpuid() tries to handle OSXSAVE but refers to env->cr[4] for the
guest copy of CR4.  HVF doesn't support caching CPUID values like KVM,
so we need to track it ourselves.

Signed-off-by: Cameron Esfahani <dirty@apple.com>
---
 target/i386/cpu.h        |  1 +
 target/i386/hvf/hvf.c    | 68 ++++++++++++++++++++++++++++++++++++++--
 target/i386/hvf/vmx.h    |  9 +++++-
 target/i386/hvf/x86hvf.c |  2 +-
 4 files changed, 76 insertions(+), 4 deletions(-)

diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 1286ec6e7a..f3864d0fac 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1591,6 +1591,7 @@ typedef struct CPUX86State {
     struct kvm_nested_state *nested_state;
 #endif
 #if defined(CONFIG_HVF)
+    bool osxsave_enabled;
     HVFX86EmulatorState *hvf_emul;
 #endif
 
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index fef1ee7d70..68a85c3b9b 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -65,6 +65,7 @@
 
 #include <Hypervisor/hv.h>
 #include <Hypervisor/hv_vmx.h>
+#include <mach/mach.h>
 
 #include "exec/address-spaces.h"
 #include "hw/i386/apic_internal.h"
@@ -458,7 +459,7 @@ void hvf_reset_vcpu(CPUState *cpu) {
     macvm_set_cr0(cpu->hvf_fd, CR0_CD_MASK | CR0_NW_MASK | CR0_ET_MASK);
     macvm_set_cr0(cpu->hvf_fd, 0x60000010);
 
-    wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK);
+    wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK | CR4_OSXSAVE_MASK);
     wvmcs(cpu->hvf_fd, VMCS_CR4_SHADOW, 0x0);
     wvmcs(cpu->hvf_fd, VMCS_GUEST_CR4, CR4_VMXE_MASK);
 
@@ -541,6 +542,55 @@ static void dummy_signal(int sig)
 {
 }
 
+static bool enable_avx512_thread_state(void)
+{
+    x86_avx512_state_t state;
+    uint32_t ebx;
+    kern_return_t ret;
+    unsigned int count;
+
+    /*
+     * macOS lazily enables AVX512 support.  Enable it explicitly if the
+     * processor supports it.
+     */
+
+    host_cpuid(7, 0, NULL, &ebx, NULL, NULL);
+    if ((ebx & CPUID_7_0_EBX_AVX512F) == 0) {
+        return false;
+    }
+
+    memset(&state, 0, sizeof(x86_avx512_state_t));
+
+    /* Get AVX state */
+    count = x86_AVX_STATE_COUNT;
+    ret = thread_get_state(mach_thread_self(),
+                           x86_AVX_STATE,
+                           (thread_state_t) &state,
+                           &count);
+    if (ret != KERN_SUCCESS) {
+        return false;
+    }
+    if (count != x86_AVX_STATE_COUNT) {
+        return false;
+    }
+    if (state.ash.flavor != x86_AVX_STATE64) {
+        return false;
+    }
+    state.ash.flavor = x86_AVX512_STATE64;
+    state.ash.count = x86_AVX512_STATE64_COUNT;
+
+    /* Now set as AVX512 */
+    ret = thread_set_state(mach_thread_self(),
+                           state.ash.flavor,
+                           (thread_state_t) &state.ufs.as64,
+                           state.ash.count);
+    if (ret != KERN_SUCCESS) {
+        return false;
+    }
+
+    return true;
+}
+
 int hvf_init_vcpu(CPUState *cpu)
 {
 
@@ -826,6 +876,18 @@ int hvf_vcpu_exec(CPUState *cpu)
 
             cpu_x86_cpuid(env, rax, rcx, &rax, &rbx, &rcx, &rdx);
 
+            if (rax == 1) {
+                /*
+                 * cpu_x86_cpuid tries to handle OSXSAVE but refers to
+                 * env->cr[4] for the guest copy of CR4.  This isn't
+                 * updated regularly so we track it ourselves in
+                 * env->osxsave_enabled.
+                 */
+                if ((rcx & CPUID_EXT_XSAVE) && env->osxsave_enabled) {
+                    rcx |= CPUID_EXT_OSXSAVE;
+                }
+            }
+
             wreg(cpu->hvf_fd, HV_X86_RAX, rax);
             wreg(cpu->hvf_fd, HV_X86_RBX, rbx);
             wreg(cpu->hvf_fd, HV_X86_RCX, rcx);
@@ -889,7 +951,7 @@ int hvf_vcpu_exec(CPUState *cpu)
                 break;
             }
             case 4: {
-                macvm_set_cr4(cpu->hvf_fd, RRX(env, reg));
+                macvm_set_cr4(env, cpu->hvf_fd, RRX(env, reg));
                 break;
             }
             case 8: {
@@ -966,6 +1028,8 @@ static int hvf_accel_init(MachineState *ms)
     hv_return_t ret;
     HVFState *s;
 
+    enable_avx512_thread_state();
+
     ret = hv_vm_create(HV_VM_DEFAULT);
     assert_hvf_ok(ret);
 
diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index 1a1b150c97..dccd5ceb0f 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -157,13 +157,20 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
     hv_vcpu_flush(vcpu);
 }
 
-static inline void macvm_set_cr4(hv_vcpuid_t vcpu, uint64_t cr4)
+static inline void macvm_set_cr4(CPUX86State *env, hv_vcpuid_t vcpu,
+                                 uint64_t cr4)
 {
     uint64_t guest_cr4 = cr4 | CR4_VMXE_MASK;
 
     wvmcs(vcpu, VMCS_GUEST_CR4, guest_cr4);
     wvmcs(vcpu, VMCS_CR4_SHADOW, cr4);
 
+    /*
+     * Track whether OSXSAVE is enabled so we can properly return it
+     * for CPUID 1.
+     */
+    env->osxsave_enabled = ((cr4 & CR4_OSXSAVE_MASK) != 0);
+
     hv_vcpu_invalidate_tlb(vcpu);
     hv_vcpu_flush(vcpu);
 }
diff --git a/target/i386/hvf/x86hvf.c b/target/i386/hvf/x86hvf.c
index edefe5319a..bd25bf19c4 100644
--- a/target/i386/hvf/x86hvf.c
+++ b/target/i386/hvf/x86hvf.c
@@ -100,7 +100,7 @@ void hvf_put_segments(CPUState *cpu_state)
     vmx_update_tpr(cpu_state);
     wvmcs(cpu_state->hvf_fd, VMCS_GUEST_IA32_EFER, env->efer);
 
-    macvm_set_cr4(cpu_state->hvf_fd, env->cr[4]);
+    macvm_set_cr4(env, cpu_state->hvf_fd, env->cr[4]);
     macvm_set_cr0(cpu_state->hvf_fd, env->cr[0]);
 
     hvf_set_segment(cpu_state, &seg, &env->segs[R_CS], false);
-- 
2.24.0

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 <dirty@apple.com>
> ---
>  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

Re: [PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.

[Roman Bolshakov]

On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote:
> Signed-off-by: Cameron Esfahani <dirty@apple.com>
> ---
>  target/i386/hvf/vmx.h | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
> index 8ec2e6414e..1a1b150c97 100644
> --- a/target/i386/hvf/vmx.h
> +++ b/target/i386/hvf/vmx.h
> @@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
>      uint64_t pdpte[4] = {0, 0, 0, 0};
>      uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER);
>      uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0);
> +    uint64_t changed_cr0 = old_cr0 ^ cr0;
>      uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK |
>                      CR0_NE_MASK | CR0_ET_MASK;
>  
> @@ -139,11 +140,12 @@ 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_MASK) && (cr0 & CR0_PG_MASK)) {
> -            enter_long_mode(vcpu, cr0, efer);
> -        }
> -        if (!(cr0 & CR0_PG_MASK)) {
> -            exit_long_mode(vcpu, cr0, efer);
> +        if (changed_cr0 & CR0_PG_MASK) {
> +            if (cr0 & CR0_PG_MASK) {
> +                enter_long_mode(vcpu, cr0, efer);
> +            } else {
> +                exit_long_mode(vcpu, cr0, efer);
> +            }
>          }
>      }
>  
> -- 
> 2.24.0
> 

The changes look good but I have a few nitpicks.

The summary line should not have "." at the end, please see
(https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message):
> Whether the "single line summary of change" starts with a capital is a
> matter of taste, but we prefer that the summary does not end in "."

Also, it would be good to mention in the title/commit message that with the
change QEMU is skipping unconditional writes to the guest IA32_EFER.LMA
and VMCS Entry Controls in compatibility mode, instead it does so only
when the actual switch out of long mode happens. (It's worth to mention
any other issues the patch helps to address, if any).

The comment in the previous patch may be dropped here IMO.

Besides that,
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman

Re: [PATCH v1 3/3] hvf: Support AVX512 guests on capable hardware

[Paolo Bonzini]

On 31/03/20 02:16, Cameron Esfahani wrote:
> @@ -458,7 +459,7 @@ void hvf_reset_vcpu(CPUState *cpu) {
>      macvm_set_cr0(cpu->hvf_fd, CR0_CD_MASK | CR0_NW_MASK | CR0_ET_MASK);
>      macvm_set_cr0(cpu->hvf_fd, 0x60000010);
>  
> -    wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK);
> +    wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK | CR4_OSXSAVE_MASK);
>      wvmcs(cpu->hvf_fd, VMCS_CR4_SHADOW, 0x0);
>      wvmcs(cpu->hvf_fd, VMCS_GUEST_CR4, CR4_VMXE_MASK);
>  
> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
> index 1a1b150c97..dccd5ceb0f 100644
> --- a/target/i386/hvf/vmx.h
> +++ b/target/i386/hvf/vmx.h
> @@ -157,13 +157,20 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
>      hv_vcpu_flush(vcpu);
>  }
>  
> -static inline void macvm_set_cr4(hv_vcpuid_t vcpu, uint64_t cr4)
> +static inline void macvm_set_cr4(CPUX86State *env, hv_vcpuid_t vcpu,
> +                                 uint64_t cr4)
>  {
>      uint64_t guest_cr4 = cr4 | CR4_VMXE_MASK;

I think you need to add the host CR4.OSXSAVE bit here too?  (You can
read it from CPUID).

>      wvmcs(vcpu, VMCS_GUEST_CR4, guest_cr4);
>      wvmcs(vcpu, VMCS_CR4_SHADOW, cr4);
>  
> +    /*
> +     * Track whether OSXSAVE is enabled so we can properly return it
> +     * for CPUID 1.
> +     */
> +    env->osxsave_enabled = ((cr4 & CR4_OSXSAVE_MASK) != 0);

This new variable doesn't seem necessary.  Instead you can just set
env->cr[4] here, and everything should work fine.

Paolo

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

[Cameron Esfahani via]

Responses inline

Cameron Esfahani
dirty@apple.com

"We do what we must because we can."

Aperture Science



> On Apr 5, 2020, at 10:58 AM, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> 
> On Mon, Mar 30, 2020 at 05:16:04PM -0700, Cameron Esfahani wrote:
>> Signed-off-by: Cameron Esfahani <dirty@apple.com>
>> ---
>> 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.
> 

I'll fix it in next patch update, pending feedback from next issue.

>> 
>>     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.
> 

The reason I removed that code was because checkpatch.pl scolded me for a patch with code commented out.

I assumed that I'd get a similar warning from patchew.org about some erroneous coding styles.

So I thought the easiest thing would be to remove that code as well.

But I'll defer to you or Paolo: should I remove that commented code with this patch?

>>             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

Re: [PATCH v1 2/3] hvf: Make long mode enter and exit code clearer.

[Cameron Esfahani via]

I'll update with your feedback.

Cameron Esfahani
dirty@apple.com

"We do what we must because we can."

Aperture Science



> On Apr 5, 2020, at 11:51 AM, Roman Bolshakov <r.bolshakov@yadro.com> wrote:
> 
> On Mon, Mar 30, 2020 at 05:16:05PM -0700, Cameron Esfahani wrote:
>> Signed-off-by: Cameron Esfahani <dirty@apple.com>
>> ---
>> target/i386/hvf/vmx.h | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>> 
>> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
>> index 8ec2e6414e..1a1b150c97 100644
>> --- a/target/i386/hvf/vmx.h
>> +++ b/target/i386/hvf/vmx.h
>> @@ -121,6 +121,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
>>     uint64_t pdpte[4] = {0, 0, 0, 0};
>>     uint64_t efer = rvmcs(vcpu, VMCS_GUEST_IA32_EFER);
>>     uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0);
>> +    uint64_t changed_cr0 = old_cr0 ^ cr0;
>>     uint64_t mask = CR0_PG_MASK | CR0_CD_MASK | CR0_NW_MASK |
>>                     CR0_NE_MASK | CR0_ET_MASK;
>> 
>> @@ -139,11 +140,12 @@ 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_MASK) && (cr0 & CR0_PG_MASK)) {
>> -            enter_long_mode(vcpu, cr0, efer);
>> -        }
>> -        if (!(cr0 & CR0_PG_MASK)) {
>> -            exit_long_mode(vcpu, cr0, efer);
>> +        if (changed_cr0 & CR0_PG_MASK) {
>> +            if (cr0 & CR0_PG_MASK) {
>> +                enter_long_mode(vcpu, cr0, efer);
>> +            } else {
>> +                exit_long_mode(vcpu, cr0, efer);
>> +            }
>>         }
>>     }
>> 
>> -- 
>> 2.24.0
>> 
> 
> The changes look good but I have a few nitpicks.
> 
> The summary line should not have "." at the end, please see
> (https://wiki.qemu.org/Contribute/SubmitAPatch#Write_a_meaningful_commit_message):
>> Whether the "single line summary of change" starts with a capital is a
>> matter of taste, but we prefer that the summary does not end in "."
> 
> Also, it would be good to mention in the title/commit message that with the
> change QEMU is skipping unconditional writes to the guest IA32_EFER.LMA
> and VMCS Entry Controls in compatibility mode, instead it does so only
> when the actual switch out of long mode happens. (It's worth to mention
> any other issues the patch helps to address, if any).
> 
> The comment in the previous patch may be dropped here IMO.
> 
> Besides that,
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Thanks,
> Roman

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

[Paolo Bonzini]

On 08/04/20 08:09, Cameron Esfahani wrote:
>>>
>>>     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.
>>
> The reason I removed that code was because checkpatch.pl scolded me for a patch with code commented out.
> 
> I assumed that I'd get a similar warning from patchew.org about some erroneous coding styles.
> 
> So I thought the easiest thing would be to remove that code as well.
> 
> But I'll defer to you or Paolo: should I remove that commented code with this patch?

checkpatch errors are not absolutely a no-no, especially if the code is
pre-existing and/or it goes away later in the patch.  In this case,
since you have already written the patch it's okay to keep it as is.

Paolo

Re: [PATCH v1 3/3] hvf: Support AVX512 guests on capable hardware

[Roman Bolshakov]

On Mon, Mar 30, 2020 at 05:16:06PM -0700, Cameron Esfahani wrote:
> macOS lazily enables AVX512.  Explicitly enable it if the processor
> supports it.
> 
> cpu_x86_cpuid() tries to handle OSXSAVE but refers to env->cr[4] for the
> guest copy of CR4.  HVF doesn't support caching CPUID values like KVM,
> so we need to track it ourselves.
> 

Hi Cameron,

Side question, how did you test it? I tried the latest ubuntu and
archlinux iso with -cpu host and noticed that kernel complains about TSC
misbehaviour early during boot.

Also, I didn't look thoroughly into it but do you happen to know if
there could be a case that we're getting AVX512 support without AVX2
support? Wouldn't that be an issue?

Thanks,
Roman