[PATCH v2 0/9] Improve synchronization between QEMU and HVF

[PATCH v2 0/9] Improve synchronization between QEMU and HVF

[Roman Bolshakov]

The series is a prerequisite to implement gdbstub support for HVF and mostly
concerns improvements of cpu_synchronize_* functions wrt to HVF and addresses
old TODO's in the related code.

Changes since v1:
 - Reduced kick loss race (Paolo) and removed SIG_IPI blocking in the
   kick patch
 - Added an old patch from Cameron that improves readibility
 - Moved LMA Guest Entry control sync to macvm_set_cr0() (Paolo)
 - Dropped hvf_vcpu_reset() and PDPTE's initialization in one patch

Cameron Esfahani (1):
  i386: hvf: Make long mode enter and exit clearer

Roman Bolshakov (8):
  i386: hvf: Set env->eip in macvm_set_rip()
  i386: hvf: Move synchronize functions to sysemu
  i386: hvf: Add hvf_cpu_synchronize_pre_loadvm()
  i386: hvf: Implement CPU kick
  i386: hvf: Move Guest LMA reset to macvm_set_cr0()
  i386: hvf: Don't duplicate register reset
  i386: hvf: Clean up synchronize functions
  MAINTAINERS: Add Cameron as HVF co-maintainer

 MAINTAINERS               |   2 +
 cpus.c                    |  25 ++----
 include/hw/core/cpu.h     |   2 +-
 include/sysemu/hvf.h      |   3 +-
 include/sysemu/hw_accel.h |  13 +++
 target/i386/cpu.c         |   3 -
 target/i386/cpu.h         |   1 +
 target/i386/hvf/hvf.c     | 179 ++++++++++++--------------------------
 target/i386/hvf/vmcs.h    |   1 +
 target/i386/hvf/vmx.h     |  17 ++--
 10 files changed, 95 insertions(+), 151 deletions(-)

-- 
2.26.1

[PATCH v2 1/9] i386: hvf: Set env->eip in macvm_set_rip()

[Roman Bolshakov]

cpu_synchronize_state() is currently no-op for hvf but BIOS will hang in
vAPIC option ROM when cpu_synchronize_state() is wired to
hvf_cpu_synchronize_state().

cpu_synchronize_state() state is called from vapic_write() during option
ROM initialization. It sets dirty flag on the cpu. macvm_set_rip() is
then invoked to advance IP after the I/O write to vAPIC port.

macvm_set_rip() only modifies VMCS, it doesn't change env->eip.
Therefore on the next iteration of vCPU loop, vcpu_dirty flag is checked
and hvf_put_registers() overwrites correct RIP in VMCS with the value of
env->eip that points to the I/O write instruction. Execution of the CPU
gets stuck on the instruction.

The issue can be avoided if eip doesn't contain stale value when dirty
flag is set on cpu.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 target/i386/hvf/vmx.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index ce2a1532d5..1e8b29bf7d 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -173,6 +173,7 @@ static inline void macvm_set_rip(CPUState *cpu, uint64_t rip)
 
     /* BUG, should take considering overlap.. */
     wreg(cpu->hvf_fd, HV_X86_RIP, rip);
+    env->eip = rip;
 
     /* after moving forward in rip, we need to clean INTERRUPTABILITY */
    val = rvmcs(cpu->hvf_fd, VMCS_GUEST_INTERRUPTIBILITY);
-- 
2.26.1

[PATCH v2 2/9] i386: hvf: Move synchronize functions to sysemu

[Roman Bolshakov]

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 cpus.c                    | 12 ------------
 include/sysemu/hw_accel.h | 10 ++++++++++
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/cpus.c b/cpus.c
index 41d1c5099f..d94456ed29 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1017,10 +1017,6 @@ void cpu_synchronize_all_states(void)
 
     CPU_FOREACH(cpu) {
         cpu_synchronize_state(cpu);
-        /* TODO: move to cpu_synchronize_state() */
-        if (hvf_enabled()) {
-            hvf_cpu_synchronize_state(cpu);
-        }
     }
 }
 
@@ -1030,10 +1026,6 @@ void cpu_synchronize_all_post_reset(void)
 
     CPU_FOREACH(cpu) {
         cpu_synchronize_post_reset(cpu);
-        /* TODO: move to cpu_synchronize_post_reset() */
-        if (hvf_enabled()) {
-            hvf_cpu_synchronize_post_reset(cpu);
-        }
     }
 }
 
@@ -1043,10 +1035,6 @@ void cpu_synchronize_all_post_init(void)
 
     CPU_FOREACH(cpu) {
         cpu_synchronize_post_init(cpu);
-        /* TODO: move to cpu_synchronize_post_init() */
-        if (hvf_enabled()) {
-            hvf_cpu_synchronize_post_init(cpu);
-        }
     }
 }
 
diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
index 0ec2372477..80bce75921 100644
--- a/include/sysemu/hw_accel.h
+++ b/include/sysemu/hw_accel.h
@@ -14,6 +14,7 @@
 #include "hw/core/cpu.h"
 #include "sysemu/hax.h"
 #include "sysemu/kvm.h"
+#include "sysemu/hvf.h"
 #include "sysemu/whpx.h"
 
 static inline void cpu_synchronize_state(CPUState *cpu)
@@ -24,6 +25,9 @@ static inline void cpu_synchronize_state(CPUState *cpu)
     if (hax_enabled()) {
         hax_cpu_synchronize_state(cpu);
     }
+    if (hvf_enabled()) {
+        hvf_cpu_synchronize_state(cpu);
+    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_state(cpu);
     }
@@ -37,6 +41,9 @@ static inline void cpu_synchronize_post_reset(CPUState *cpu)
     if (hax_enabled()) {
         hax_cpu_synchronize_post_reset(cpu);
     }
+    if (hvf_enabled()) {
+        hvf_cpu_synchronize_post_reset(cpu);
+    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_post_reset(cpu);
     }
@@ -50,6 +57,9 @@ static inline void cpu_synchronize_post_init(CPUState *cpu)
     if (hax_enabled()) {
         hax_cpu_synchronize_post_init(cpu);
     }
+    if (hvf_enabled()) {
+        hvf_cpu_synchronize_post_init(cpu);
+    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_post_init(cpu);
     }
-- 
2.26.1

[PATCH v2 3/9] i386: hvf: Add hvf_cpu_synchronize_pre_loadvm()

[Roman Bolshakov]

hvf lacks an implementation of cpu_synchronize_pre_loadvm().

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 include/sysemu/hvf.h      |  1 +
 include/sysemu/hw_accel.h |  3 +++
 target/i386/hvf/hvf.c     | 11 +++++++++++
 3 files changed, 15 insertions(+)

diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
index 5214ed5202..1d40a8ec01 100644
--- a/include/sysemu/hvf.h
+++ b/include/sysemu/hvf.h
@@ -28,6 +28,7 @@ int hvf_vcpu_exec(CPUState *);
 void hvf_cpu_synchronize_state(CPUState *);
 void hvf_cpu_synchronize_post_reset(CPUState *);
 void hvf_cpu_synchronize_post_init(CPUState *);
+void hvf_cpu_synchronize_pre_loadvm(CPUState *);
 void hvf_vcpu_destroy(CPUState *);
 void hvf_reset_vcpu(CPUState *);
 
diff --git a/include/sysemu/hw_accel.h b/include/sysemu/hw_accel.h
index 80bce75921..e128f8b06b 100644
--- a/include/sysemu/hw_accel.h
+++ b/include/sysemu/hw_accel.h
@@ -73,6 +73,9 @@ static inline void cpu_synchronize_pre_loadvm(CPUState *cpu)
     if (hax_enabled()) {
         hax_cpu_synchronize_pre_loadvm(cpu);
     }
+    if (hvf_enabled()) {
+        hvf_cpu_synchronize_pre_loadvm(cpu);
+    }
     if (whpx_enabled()) {
         whpx_cpu_synchronize_pre_loadvm(cpu);
     }
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index be016b951a..efe9802962 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -325,6 +325,17 @@ void hvf_cpu_synchronize_post_init(CPUState *cpu_state)
     run_on_cpu(cpu_state, do_hvf_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
 
+static void do_hvf_cpu_synchronize_pre_loadvm(CPUState *cpu,
+                                              run_on_cpu_data arg)
+{
+    cpu->vcpu_dirty = true;
+}
+
+void hvf_cpu_synchronize_pre_loadvm(CPUState *cpu)
+{
+    run_on_cpu(cpu, do_hvf_cpu_synchronize_pre_loadvm, RUN_ON_CPU_NULL);
+}
+
 static bool ept_emulation_fault(hvf_slot *slot, uint64_t gpa, uint64_t ept_qual)
 {
     int read, write;
-- 
2.26.1

[PATCH v2 4/9] i386: hvf: Implement CPU kick

[Roman Bolshakov]

HVF doesn't have a CPU kick and without it it's not possible to perform
an action on CPU thread until a VMEXIT happens. The kick is also needed
for timely interrupt delivery.

Existing implementation of CPU kick sends SIG_IPI (aka SIGUSR1) to vCPU
thread, but it's different from what hv_vcpu_interrupt does. The latter
one results in invocation of mp_cpus_kick() in XNU kernel [1].

mp_cpus_kick() sends an IPI through the host LAPIC to the HVF vCPU.
And the kick interrupt leads to VM exit because "external-interrupt
exiting” VM-execution control is enabled for HVF. VMX-preemption timer
is used (if available) to avoid kick loss if the kick is delivered
outside of hv_vcpu_run().

While at it, correct type of hvf_fd to the type of hv_vcpuid_t to avoid
compilation warnings.

1. https://opensource.apple.com/source/xnu/xnu-6153.81.5/osfmk/i386/mp.c

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 cpus.c                 | 13 +++++++++----
 include/hw/core/cpu.h  |  2 +-
 include/sysemu/hvf.h   |  1 +
 target/i386/cpu.h      |  1 +
 target/i386/hvf/hvf.c  | 42 +++++++++++++++++++++++++++---------------
 target/i386/hvf/vmcs.h |  1 +
 6 files changed, 40 insertions(+), 20 deletions(-)

diff --git a/cpus.c b/cpus.c
index d94456ed29..6be42ff734 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1792,10 +1792,15 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
         return;
     }
     cpu->thread_kicked = true;
-    err = pthread_kill(cpu->thread->thread, SIG_IPI);
-    if (err && err != ESRCH) {
-        fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
-        exit(1);
+
+    if (hvf_enabled()) {
+        hvf_vcpu_kick(cpu);
+    } else {
+        err = pthread_kill(cpu->thread->thread, SIG_IPI);
+        if (err && err != ESRCH) {
+            fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
+            exit(1);
+        }
     }
 #else /* _WIN32 */
     if (!qemu_cpu_is_self(cpu)) {
diff --git a/include/hw/core/cpu.h b/include/hw/core/cpu.h
index b3f4b79318..288a2bd57e 100644
--- a/include/hw/core/cpu.h
+++ b/include/hw/core/cpu.h
@@ -438,7 +438,7 @@ struct CPUState {
 
     struct hax_vcpu_state *hax_vcpu;
 
-    int hvf_fd;
+    unsigned hvf_fd;
 
     /* track IOMMUs whose translations we've cached in the TCG TLB */
     GArray *iommu_notifiers;
diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
index 1d40a8ec01..aaa00cbf05 100644
--- a/include/sysemu/hvf.h
+++ b/include/sysemu/hvf.h
@@ -25,6 +25,7 @@ extern bool hvf_allowed;
 
 int hvf_init_vcpu(CPUState *);
 int hvf_vcpu_exec(CPUState *);
+void hvf_vcpu_kick(CPUState *);
 void hvf_cpu_synchronize_state(CPUState *);
 void hvf_cpu_synchronize_post_reset(CPUState *);
 void hvf_cpu_synchronize_post_init(CPUState *);
diff --git a/target/i386/cpu.h b/target/i386/cpu.h
index 7d77efd9e4..4ae6038f22 100644
--- a/target/i386/cpu.h
+++ b/target/i386/cpu.h
@@ -1602,6 +1602,7 @@ typedef struct CPUX86State {
     struct kvm_nested_state *nested_state;
 #endif
 #if defined(CONFIG_HVF)
+    uint64_t hvf_deadline;
     HVFX86LazyFlags hvf_lflags;
     void *hvf_mmio_buf;
 #endif
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index efe9802962..317304aa1d 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -72,6 +72,9 @@
 #include "sysemu/accel.h"
 #include "target/i386/cpu.h"
 
+/* Maximum value of VMX-preemption timer */
+#define HVF_MAX_DEADLINE UINT32_MAX
+
 HVFState *hvf_state;
 
 static void assert_hvf_ok(hv_return_t ret)
@@ -552,10 +555,6 @@ void hvf_vcpu_destroy(CPUState *cpu)
     assert_hvf_ok(ret);
 }
 
-static void dummy_signal(int sig)
-{
-}
-
 int hvf_init_vcpu(CPUState *cpu)
 {
 
@@ -563,21 +562,11 @@ int hvf_init_vcpu(CPUState *cpu)
     CPUX86State *env = &x86cpu->env;
     int r;
 
-    /* init cpu signals */
-    sigset_t set;
-    struct sigaction sigact;
-
-    memset(&sigact, 0, sizeof(sigact));
-    sigact.sa_handler = dummy_signal;
-    sigaction(SIG_IPI, &sigact, NULL);
-
-    pthread_sigmask(SIG_BLOCK, NULL, &set);
-    sigdelset(&set, SIG_IPI);
-
     init_emu();
     init_decoder();
 
     hvf_state->hvf_caps = g_new0(struct hvf_vcpu_caps, 1);
+    env->hvf_deadline = HVF_MAX_DEADLINE;
     env->hvf_mmio_buf = g_new(char, 4096);
 
     r = hv_vcpu_create((hv_vcpuid_t *)&cpu->hvf_fd, HV_VCPU_DEFAULT);
@@ -606,6 +595,7 @@ int hvf_init_vcpu(CPUState *cpu)
           cap2ctrl(hvf_state->hvf_caps->vmx_cap_pinbased,
           VMCS_PIN_BASED_CTLS_EXTINT |
           VMCS_PIN_BASED_CTLS_NMI |
+          VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER |
           VMCS_PIN_BASED_CTLS_VNMI));
     wvmcs(cpu->hvf_fd, VMCS_PRI_PROC_BASED_CTLS,
           cap2ctrl(hvf_state->hvf_caps->vmx_cap_procbased,
@@ -725,7 +715,14 @@ int hvf_vcpu_exec(CPUState *cpu)
             return EXCP_HLT;
         }
 
+        /* Use VMX-preemption timer trick only if available */
+        if (rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS) &
+            VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER) {
+            wvmcs(cpu->hvf_fd, VMCS_PREEMPTION_TIMER_VALUE,
+                  atomic_read(&env->hvf_deadline));
+        }
         hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
+        atomic_set(&env->hvf_deadline, HVF_MAX_DEADLINE);
         assert_hvf_ok(r);
 
         /* handle VMEXIT */
@@ -869,6 +866,7 @@ int hvf_vcpu_exec(CPUState *cpu)
             ret = EXCP_INTERRUPT;
             break;
         case EXIT_REASON_EXT_INTR:
+        case EXIT_REASON_VMX_PREEMPT:
             /* force exit and allow io handling */
             ret = EXCP_INTERRUPT;
             break;
@@ -966,6 +964,20 @@ int hvf_vcpu_exec(CPUState *cpu)
     return ret;
 }
 
+void hvf_vcpu_kick(CPUState *cpu)
+{
+    X86CPU *x86_cpu = X86_CPU(cpu);
+    CPUX86State *env = &x86_cpu->env;
+    hv_return_t err;
+
+    atomic_set(&env->hvf_deadline, 0);
+    err = hv_vcpu_interrupt(&cpu->hvf_fd, 1);
+    if (err) {
+        fprintf(stderr, "qemu:%s error %#x\n", __func__, err);
+        exit(1);
+    }
+}
+
 bool hvf_allowed;
 
 static int hvf_accel_init(MachineState *ms)
diff --git a/target/i386/hvf/vmcs.h b/target/i386/hvf/vmcs.h
index 42de7ebc3a..6615365023 100644
--- a/target/i386/hvf/vmcs.h
+++ b/target/i386/hvf/vmcs.h
@@ -349,6 +349,7 @@
 #define VMCS_PIN_BASED_CTLS_EXTINT            (1 << 0)
 #define VMCS_PIN_BASED_CTLS_NMI               (1 << 3)
 #define VMCS_PIN_BASED_CTLS_VNMI              (1 << 5)
+#define VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER (1 << 6)
 
 #define VMCS_PRI_PROC_BASED_CTLS_INT_WINDOW_EXITING (1 << 2)
 #define VMCS_PRI_PROC_BASED_CTLS_TSC_OFFSET (1 << 3)
-- 
2.26.1

[PATCH v2 5/9] i386: hvf: Make long mode enter and exit clearer

[Roman Bolshakov]

From: Cameron Esfahani <dirty@apple.com>

Intel SDM "9.8.5 Initializing IA-32e Mode" and "9.8.5.4 Switching Out of
IA-32e Mode Operation" define activation and deactivation of long mode
only upon a change of CR0.PG but current code invokes exit_long_mode()
unconditionally until LME is cleared.

Signed-off-by: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.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 1e8b29bf7d..437238f11d 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 | CR0_CD | CR0_NW | CR0_NE | CR0_ET;
 
     if ((cr0 & CR0_PG) && (rvmcs(vcpu, VMCS_GUEST_CR4) & CR4_PAE) &&
@@ -138,11 +139,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) && (cr0 & CR0_PG)) {
-            enter_long_mode(vcpu, cr0, efer);
-        }
-        if (/*(old_cr0 & CR0_PG) &&*/ !(cr0 & CR0_PG)) {
-            exit_long_mode(vcpu, cr0, efer);
+        if (changed_cr0 & CR0_PG) {
+            if (cr0 & CR0_PG) {
+                enter_long_mode(vcpu, cr0, efer);
+            } else {
+                exit_long_mode(vcpu, cr0, efer);
+            }
         }
     }
 
-- 
2.26.1

[PATCH v2 6/9] i386: hvf: Move Guest LMA reset to macvm_set_cr0()

[Roman Bolshakov]

The only useful purpose of hvf_reset_vcpu() is to clear "IA-32e mode
guest" (LMA) VM-Entry control. But it can be moved to macvm_set_cr0()
which is indirectly used by post-init and post-reset to flush emulator
state. That enables clean removal of hvf_reset_vcpu().

LMA is set only if IA32_EFER.LME = 1, according to Intel SDM "9.8.5
Initializing IA-32e Mode" and "9.8.5.4 Switching Out of IA-32e Mode
Operation", otherwise the entry control can be safely cleared.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 target/i386/hvf/hvf.c | 1 -
 target/i386/hvf/vmx.h | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 317304aa1d..1d3a27167d 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -462,7 +462,6 @@ void hvf_reset_vcpu(CPUState *cpu) {
     /* TODO: this shouldn't be needed; there is already a call to
      * cpu_synchronize_all_post_reset in vl.c
      */
-    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
     wvmcs(cpu->hvf_fd, VMCS_GUEST_IA32_EFER, 0);
 
     /* Initialize PDPTE */
diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
index 437238f11d..75ba1e2a5f 100644
--- a/target/i386/hvf/vmx.h
+++ b/target/i386/hvf/vmx.h
@@ -123,6 +123,7 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
     uint64_t old_cr0 = rvmcs(vcpu, VMCS_GUEST_CR0);
     uint64_t changed_cr0 = old_cr0 ^ cr0;
     uint64_t mask = CR0_PG | CR0_CD | CR0_NW | CR0_NE | CR0_ET;
+    uint64_t entry_ctls;
 
     if ((cr0 & CR0_PG) && (rvmcs(vcpu, VMCS_GUEST_CR4) & CR4_PAE) &&
         !(efer & MSR_EFER_LME)) {
@@ -146,6 +147,9 @@ static inline void macvm_set_cr0(hv_vcpuid_t vcpu, uint64_t cr0)
                 exit_long_mode(vcpu, cr0, efer);
             }
         }
+    } else {
+        entry_ctls = rvmcs(vcpu, VMCS_ENTRY_CTLS);
+        wvmcs(vcpu, VMCS_ENTRY_CTLS, entry_ctls & ~VM_ENTRY_GUEST_LMA);
     }
 
     /* Filter new CR0 after we are finished examining it above. */
-- 
2.26.1

[PATCH v2 7/9] i386: hvf: Don't duplicate register reset

[Roman Bolshakov]

hvf_reset_vcpu() duplicates actions performed by x86_cpu_reset(). The
difference is that hvf_reset_vcpu() stores initial values directly to
VMCS while x86_cpu_reset() stores it in CPUX86State and then
cpu_synchronize_all_post_init() or cpu_synchronize_all_post_reset()
flushes CPUX86State into VMCS. That makes hvf_reset_vcpu() a kind of
no-op.

Here's the trace of CPU state modifications during VM start:
  hvf_reset_vcpu (resets VMCS)
  cpu_synchronize_all_post_init (overwrites VMCS fields written by
                                 hvf_reset_vcpu())
  cpu_synchronize_all_states
  hvf_reset_vcpu (resets VMCS)
  cpu_synchronize_all_post_reset (overwrites VMCS fields written by
                                  hvf_reset_vcpu())

General purpose registers, system registers, segment descriptors, flags
and IP are set by hvf_put_segments() in post-init and post-reset,
therefore it's safe to remove them from hvf_reset_vcpu().

PDPTE initialization can be dropped because Intel SDM (26.3.1.6 Checks
on Guest Page-Directory-Pointer-Table Entries) doesn't require PDPTE to
be clear unless PAE is used: "A VM entry to a guest that does not use
PAE paging does not check the validity of any PDPTEs."
And if PAE is used, PDPTE's are initialized from CR3 in macvm_set_cr0().

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 include/sysemu/hvf.h  |  1 -
 target/i386/cpu.c     |  3 --
 target/i386/hvf/hvf.c | 89 -------------------------------------------
 3 files changed, 93 deletions(-)

diff --git a/include/sysemu/hvf.h b/include/sysemu/hvf.h
index aaa00cbf05..a1ab61403f 100644
--- a/include/sysemu/hvf.h
+++ b/include/sysemu/hvf.h
@@ -31,7 +31,6 @@ void hvf_cpu_synchronize_post_reset(CPUState *);
 void hvf_cpu_synchronize_post_init(CPUState *);
 void hvf_cpu_synchronize_pre_loadvm(CPUState *);
 void hvf_vcpu_destroy(CPUState *);
-void hvf_reset_vcpu(CPUState *);
 
 #define TYPE_HVF_ACCEL ACCEL_CLASS_NAME("hvf")
 
diff --git a/target/i386/cpu.c b/target/i386/cpu.c
index 36cbd3d027..6463712524 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6098,9 +6098,6 @@ static void x86_cpu_reset(DeviceState *dev)
     if (kvm_enabled()) {
         kvm_arch_reset_vcpu(cpu);
     }
-    else if (hvf_enabled()) {
-        hvf_reset_vcpu(s);
-    }
 #endif
 }
 
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 1d3a27167d..f1114c2a09 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -455,95 +455,6 @@ static MemoryListener hvf_memory_listener = {
     .log_sync = hvf_log_sync,
 };
 
-void hvf_reset_vcpu(CPUState *cpu) {
-    uint64_t pdpte[4] = {0, 0, 0, 0};
-    int i;
-
-    /* TODO: this shouldn't be needed; there is already a call to
-     * cpu_synchronize_all_post_reset in vl.c
-     */
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_IA32_EFER, 0);
-
-    /* Initialize PDPTE */
-    for (i = 0; i < 4; i++) {
-        wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
-    }
-
-    macvm_set_cr0(cpu->hvf_fd, 0x60000010);
-
-    wvmcs(cpu->hvf_fd, VMCS_CR4_MASK, CR4_VMXE_MASK);
-    wvmcs(cpu->hvf_fd, VMCS_CR4_SHADOW, 0x0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CR4, CR4_VMXE_MASK);
-
-    /* set VMCS guest state fields */
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CS_SELECTOR, 0xf000);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CS_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CS_ACCESS_RIGHTS, 0x9b);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CS_BASE, 0xffff0000);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_DS_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_DS_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_DS_ACCESS_RIGHTS, 0x93);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_DS_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_ES_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_ES_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_ES_ACCESS_RIGHTS, 0x93);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_ES_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_FS_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_FS_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_FS_ACCESS_RIGHTS, 0x93);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_FS_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GS_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GS_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GS_ACCESS_RIGHTS, 0x93);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GS_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_SS_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_SS_LIMIT, 0xffff);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_SS_ACCESS_RIGHTS, 0x93);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_SS_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_LDTR_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_LDTR_LIMIT, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_LDTR_ACCESS_RIGHTS, 0x10000);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_LDTR_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_TR_SELECTOR, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_TR_LIMIT, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_TR_ACCESS_RIGHTS, 0x83);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_TR_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GDTR_LIMIT, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_GDTR_BASE, 0);
-
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_IDTR_LIMIT, 0);
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_IDTR_BASE, 0);
-
-    /*wvmcs(cpu->hvf_fd, VMCS_GUEST_CR2, 0x0);*/
-    wvmcs(cpu->hvf_fd, VMCS_GUEST_CR3, 0x0);
-
-    wreg(cpu->hvf_fd, HV_X86_RIP, 0xfff0);
-    wreg(cpu->hvf_fd, HV_X86_RDX, 0x623);
-    wreg(cpu->hvf_fd, HV_X86_RFLAGS, 0x2);
-    wreg(cpu->hvf_fd, HV_X86_RSP, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RAX, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RBX, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RCX, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RSI, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RDI, 0x0);
-    wreg(cpu->hvf_fd, HV_X86_RBP, 0x0);
-
-    for (int i = 0; i < 8; i++) {
-        wreg(cpu->hvf_fd, HV_X86_R8 + i, 0x0);
-    }
-
-    hv_vcpu_invalidate_tlb(cpu->hvf_fd);
-    hv_vcpu_flush(cpu->hvf_fd);
-}
-
 void hvf_vcpu_destroy(CPUState *cpu)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
-- 
2.26.1

[PATCH v2 8/9] i386: hvf: Clean up synchronize functions

[Roman Bolshakov]

Make them more concise and consitent with the rest of the code in the
file and drop non-relevant TODO.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 target/i386/hvf/hvf.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index f1114c2a09..967bb9a193 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -285,47 +285,43 @@ void hvf_handle_io(CPUArchState *env, uint16_t port, void *buffer,
     }
 }
 
-/* TODO: synchronize vcpu state */
 static void do_hvf_cpu_synchronize_state(CPUState *cpu, run_on_cpu_data arg)
 {
-    CPUState *cpu_state = cpu;
-    if (cpu_state->vcpu_dirty == 0) {
-        hvf_get_registers(cpu_state);
+    if (!cpu->vcpu_dirty) {
+        hvf_get_registers(cpu);
+        cpu->vcpu_dirty = true;
     }
-
-    cpu_state->vcpu_dirty = 1;
 }
 
-void hvf_cpu_synchronize_state(CPUState *cpu_state)
+void hvf_cpu_synchronize_state(CPUState *cpu)
 {
-    if (cpu_state->vcpu_dirty == 0) {
-        run_on_cpu(cpu_state, do_hvf_cpu_synchronize_state, RUN_ON_CPU_NULL);
+    if (!cpu->vcpu_dirty) {
+        run_on_cpu(cpu, do_hvf_cpu_synchronize_state, RUN_ON_CPU_NULL);
     }
 }
 
-static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
+static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu,
+                                              run_on_cpu_data arg)
 {
-    CPUState *cpu_state = cpu;
-    hvf_put_registers(cpu_state);
-    cpu_state->vcpu_dirty = false;
+    hvf_put_registers(cpu);
+    cpu->vcpu_dirty = false;
 }
 
-void hvf_cpu_synchronize_post_reset(CPUState *cpu_state)
+void hvf_cpu_synchronize_post_reset(CPUState *cpu)
 {
-    run_on_cpu(cpu_state, do_hvf_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
+    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_reset, RUN_ON_CPU_NULL);
 }
 
 static void do_hvf_cpu_synchronize_post_init(CPUState *cpu,
                                              run_on_cpu_data arg)
 {
-    CPUState *cpu_state = cpu;
-    hvf_put_registers(cpu_state);
-    cpu_state->vcpu_dirty = false;
+    hvf_put_registers(cpu);
+    cpu->vcpu_dirty = false;
 }
 
-void hvf_cpu_synchronize_post_init(CPUState *cpu_state)
+void hvf_cpu_synchronize_post_init(CPUState *cpu)
 {
-    run_on_cpu(cpu_state, do_hvf_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
+    run_on_cpu(cpu, do_hvf_cpu_synchronize_post_init, RUN_ON_CPU_NULL);
 }
 
 static void do_hvf_cpu_synchronize_pre_loadvm(CPUState *cpu,
-- 
2.26.1

[PATCH v2 9/9] MAINTAINERS: Add Cameron as HVF co-maintainer

[Roman Bolshakov]

Similar patch was sent a while ago but got lost.
While at it, add a status wiki page.

Cc: Cameron Esfahani <dirty@apple.com>
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index dec252f38b..b6d4f62ba2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -417,7 +417,9 @@ F: target/i386/kvm.c
 F: scripts/kvm/vmxcap
 
 X86 HVF CPUs
+M: Cameron Esfahani <dirty@apple.com>
 M: Roman Bolshakov <r.bolshakov@yadro.com>
+W: https://wiki.qemu.org/Features/HVF
 S: Maintained
 F: accel/stubs/hvf-stub.c
 F: target/i386/hvf/
-- 
2.26.1

Re: [PATCH v2 4/9] i386: hvf: Implement CPU kick

[Paolo Bonzini]

On 30/06/20 12:28, Roman Bolshakov wrote:
> @@ -966,6 +964,20 @@ int hvf_vcpu_exec(CPUState *cpu)
>      return ret;
>  }
>  
> +void hvf_vcpu_kick(CPUState *cpu)
> +{
> +    X86CPU *x86_cpu = X86_CPU(cpu);
> +    CPUX86State *env = &x86_cpu->env;
> +    hv_return_t err;
> +
> +    atomic_set(&env->hvf_deadline, 0);
> +    err = hv_vcpu_interrupt(&cpu->hvf_fd, 1);
> +    if (err) {
> +        fprintf(stderr, "qemu:%s error %#x\n", __func__, err);
> +        exit(1);
> +    }

Can a signal interrupt hv_vcpu_run?  If so you actually don't need
hv_vcpu_interrupt at all.  You can also require the preemption time, all
processor that support HVF have it, but never set it by default.  The
deadline can be left at 0 all the time; instead, you toggle the bit in
the pin-based controls.  In the signal handler you do:

	if (atomic_xchg(&env->hvf_in_guest, false)) {
		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
			| VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
	}

In the main loop you do:

	atomic_set(&env->hvf_guest_mode, true);
	smp_mb();
	hv_vcpu_run(...);
	atomic_set(&env->hvf_guest_mode, false);

and in the preemption timer vmexit handler:
	
		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
			& ~VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);

I'll leave out this patch in the meanwhile.

Paolo

> +}
> +
>  bool hvf_allowed;
>  
>  static int hvf_accel_init(MachineState *ms)
> diff --git a/target/i386/hvf/vmcs.h b/target/i386/hvf/vmcs.h
> index 42de7ebc3a..6615365023 100644
> --- a/target/i386/hvf/vmcs.h
> +++ b/target/i386/hvf/vmcs.h
> @@ -349,6 +349,7 @@
>  #define VMCS_PIN_BASED_CTLS_EXTINT            (1 << 0)
>  #define VMCS_PIN_BASED_CTLS_NMI               (1 << 3)
>  #define VMCS_PIN_BASED_CTLS_VNMI              (1 << 5)
> +#define VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER (1 << 6)
>  
>  #define VMCS_PRI_PROC_BASED_CTLS_INT_WINDOW_EXITING (1 << 2)
>  #define VMCS_PRI_PROC_BASED_CTLS_TSC_OFFSET (1 << 3)
> 

Re: [PATCH v2 1/9] i386: hvf: Set env->eip in macvm_set_rip()

[Paolo Bonzini]

On 30/06/20 12:28, Roman Bolshakov wrote:
> cpu_synchronize_state() is currently no-op for hvf but BIOS will hang in
> vAPIC option ROM when cpu_synchronize_state() is wired to
> hvf_cpu_synchronize_state().
> 
> cpu_synchronize_state() state is called from vapic_write() during option
> ROM initialization. It sets dirty flag on the cpu. macvm_set_rip() is
> then invoked to advance IP after the I/O write to vAPIC port.
> 
> macvm_set_rip() only modifies VMCS, it doesn't change env->eip.
> Therefore on the next iteration of vCPU loop, vcpu_dirty flag is checked
> and hvf_put_registers() overwrites correct RIP in VMCS with the value of
> env->eip that points to the I/O write instruction. Execution of the CPU
> gets stuck on the instruction.
> 
> The issue can be avoided if eip doesn't contain stale value when dirty
> flag is set on cpu.
> 
> Cc: Cameron Esfahani <dirty@apple.com>
> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com>
> ---
>  target/i386/hvf/vmx.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/target/i386/hvf/vmx.h b/target/i386/hvf/vmx.h
> index ce2a1532d5..1e8b29bf7d 100644
> --- a/target/i386/hvf/vmx.h
> +++ b/target/i386/hvf/vmx.h
> @@ -173,6 +173,7 @@ static inline void macvm_set_rip(CPUState *cpu, uint64_t rip)
>  
>      /* BUG, should take considering overlap.. */
>      wreg(cpu->hvf_fd, HV_X86_RIP, rip);
> +    env->eip = rip;
>  
>      /* after moving forward in rip, we need to clean INTERRUPTABILITY */
>     val = rvmcs(cpu->hvf_fd, VMCS_GUEST_INTERRUPTIBILITY);
> 

Queued except for patch 4.

Paolo

Re: [PATCH v2 4/9] i386: hvf: Implement CPU kick

[Roman Bolshakov]

On Tue, Jun 30, 2020 at 02:33:42PM +0200, Paolo Bonzini wrote:
> On 30/06/20 12:28, Roman Bolshakov wrote:
> > @@ -966,6 +964,20 @@ int hvf_vcpu_exec(CPUState *cpu)
> >      return ret;
> >  }
> >  
> > +void hvf_vcpu_kick(CPUState *cpu)
> > +{
> > +    X86CPU *x86_cpu = X86_CPU(cpu);
> > +    CPUX86State *env = &x86_cpu->env;
> > +    hv_return_t err;
> > +
> > +    atomic_set(&env->hvf_deadline, 0);
> > +    err = hv_vcpu_interrupt(&cpu->hvf_fd, 1);
> > +    if (err) {
> > +        fprintf(stderr, "qemu:%s error %#x\n", __func__, err);
> > +        exit(1);
> > +    }
> 
> Can a signal interrupt hv_vcpu_run?  If so you actually don't need
> hv_vcpu_interrupt at all.

Existing signal masking and SIG_IPI didn't work IIRC when I tried to add
a primitive version of gdbstub support.

> You can also require the preemption time, all
> processor that support HVF have it, but never set it by default.  The
> deadline can be left at 0 all the time; instead, you toggle the bit in
> the pin-based controls.  In the signal handler you do:
> 
> 	if (atomic_xchg(&env->hvf_in_guest, false)) {
> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> 			| VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> 	}
> 
> In the main loop you do:
> 
> 	atomic_set(&env->hvf_guest_mode, true);
> 	smp_mb();
> 	hv_vcpu_run(...);
> 	atomic_set(&env->hvf_guest_mode, false);
> 
> and in the preemption timer vmexit handler:
> 	
> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> 			& ~VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> 

Ok, I'll look into that. Thanks for the advices!

-Roman

Re: [PATCH v2 4/9] i386: hvf: Implement CPU kick

[Paolo Bonzini]

On 30/06/20 17:50, Roman Bolshakov wrote:
> On Tue, Jun 30, 2020 at 02:33:42PM +0200, Paolo Bonzini wrote:
>> On 30/06/20 12:28, Roman Bolshakov wrote:
>>> @@ -966,6 +964,20 @@ int hvf_vcpu_exec(CPUState *cpu)
>>>      return ret;
>>>  }
>>>  
>>> +void hvf_vcpu_kick(CPUState *cpu)
>>> +{
>>> +    X86CPU *x86_cpu = X86_CPU(cpu);
>>> +    CPUX86State *env = &x86_cpu->env;
>>> +    hv_return_t err;
>>> +
>>> +    atomic_set(&env->hvf_deadline, 0);
>>> +    err = hv_vcpu_interrupt(&cpu->hvf_fd, 1);
>>> +    if (err) {
>>> +        fprintf(stderr, "qemu:%s error %#x\n", __func__, err);
>>> +        exit(1);
>>> +    }
>>
>> Can a signal interrupt hv_vcpu_run?  If so you actually don't need
>> hv_vcpu_interrupt at all.
> 
> Existing signal masking and SIG_IPI didn't work IIRC when I tried to add
> a primitive version of gdbstub support.

You can try pthread_kill followed by hv_vcpu_interrupt if it doesn't.
The signal would be delivered after return to userspace.

Paolo

>> You can also require the preemption time, all
>> processor that support HVF have it, but never set it by default.  The
>> deadline can be left at 0 all the time; instead, you toggle the bit in
>> the pin-based controls.  In the signal handler you do:
>>
>> 	if (atomic_xchg(&env->hvf_in_guest, false)) {
>> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
>> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
>> 			| VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
>> 	}
>>
>> In the main loop you do:
>>
>> 	atomic_set(&env->hvf_guest_mode, true);
>> 	smp_mb();
>> 	hv_vcpu_run(...);
>> 	atomic_set(&env->hvf_guest_mode, false);
>>
>> and in the preemption timer vmexit handler:
>> 	
>> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
>> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
>> 			& ~VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
>>
> 
> Ok, I'll look into that. Thanks for the advices!
> 
> -Roman
> 

Re: [PATCH v2 4/9] i386: hvf: Implement CPU kick

[Roman Bolshakov]

On Tue, Jun 30, 2020 at 06:04:23PM +0200, Paolo Bonzini wrote:
> On 30/06/20 17:50, Roman Bolshakov wrote:
> > On Tue, Jun 30, 2020 at 02:33:42PM +0200, Paolo Bonzini wrote:
> >> Can a signal interrupt hv_vcpu_run?  If so you actually don't need
> >> hv_vcpu_interrupt at all.
> > 
> > Existing signal masking and SIG_IPI didn't work IIRC when I tried to add
> > a primitive version of gdbstub support.
> 
> You can try pthread_kill followed by hv_vcpu_interrupt if it doesn't.
> The signal would be delivered after return to userspace.
> 

I looked at the signal setup for HVF again. I was wrong with regards to
SIG_IPI. It isn't delivered to vCPU because the signal is masked, this
fixes it:

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index d81f569aed..7bf05bca21 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -479,6 +479,7 @@ int hvf_init_vcpu(CPUState *cpu)

     pthread_sigmask(SIG_BLOCK, NULL, &set);
     sigdelset(&set, SIG_IPI);
+    pthread_sigmask(SIG_SETMASK, &set, NULL);

     init_emu();
     init_decoder();

But the signal is delivered only after vmxexit, perhaps a sequence of
pthread_kill() and hv_vcpu_interrupt() is really needed.

So, there are two race windows on kernel-to-user border in v2: just
before checking the deadline and vmenter and just after vmxexit and
re-arm of preemption timer, that's two places where kicks could be lost.
The approach you proposed seems to address them.

Thanks,
Roman

> >> You can also require the preemption time, all
> >> processor that support HVF have it, but never set it by default.  The
> >> deadline can be left at 0 all the time; instead, you toggle the bit in
> >> the pin-based controls.  In the signal handler you do:
> >>
> >> 	if (atomic_xchg(&env->hvf_in_guest, false)) {
> >> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> >> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> >> 			| VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> >> 	}
> >>
> >> In the main loop you do:
> >>
> >> 	atomic_set(&env->hvf_guest_mode, true);
> >> 	smp_mb();
> >> 	hv_vcpu_run(...);
> >> 	atomic_set(&env->hvf_guest_mode, false);
> >>
> >> and in the preemption timer vmexit handler:
> >> 	
> >> 		wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> >> 		      rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> >> 			& ~VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> >>
> > 

Re: [PATCH v2 4/9] i386: hvf: Implement CPU kick

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

Thanks, sounds good! Of course the best solution would be in HVF itself,
similar to KVM and WHPX, but at least it's possible to work around it.

Paolo

Il mer 1 lug 2020, 20:37 Roman Bolshakov <r.bolshakov@yadro.com> ha scritto:

> On Tue, Jun 30, 2020 at 06:04:23PM +0200, Paolo Bonzini wrote:
> > On 30/06/20 17:50, Roman Bolshakov wrote:
> > > On Tue, Jun 30, 2020 at 02:33:42PM +0200, Paolo Bonzini wrote:
> > >> Can a signal interrupt hv_vcpu_run?  If so you actually don't need
> > >> hv_vcpu_interrupt at all.
> > >
> > > Existing signal masking and SIG_IPI didn't work IIRC when I tried to
> add
> > > a primitive version of gdbstub support.
> >
> > You can try pthread_kill followed by hv_vcpu_interrupt if it doesn't.
> > The signal would be delivered after return to userspace.
> >
>
> I looked at the signal setup for HVF again. I was wrong with regards to
> SIG_IPI. It isn't delivered to vCPU because the signal is masked, this
> fixes it:
>
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index d81f569aed..7bf05bca21 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -479,6 +479,7 @@ int hvf_init_vcpu(CPUState *cpu)
>
>      pthread_sigmask(SIG_BLOCK, NULL, &set);
>      sigdelset(&set, SIG_IPI);
> +    pthread_sigmask(SIG_SETMASK, &set, NULL);
>
>      init_emu();
>      init_decoder();
>
> But the signal is delivered only after vmxexit, perhaps a sequence of
> pthread_kill() and hv_vcpu_interrupt() is really needed.
>
> So, there are two race windows on kernel-to-user border in v2: just
> before checking the deadline and vmenter and just after vmxexit and
> re-arm of preemption timer, that's two places where kicks could be lost.
> The approach you proposed seems to address them.
>
> Thanks,
> Roman
>
> > >> You can also require the preemption time, all
> > >> processor that support HVF have it, but never set it by default.  The
> > >> deadline can be left at 0 all the time; instead, you toggle the bit in
> > >> the pin-based controls.  In the signal handler you do:
> > >>
> > >>    if (atomic_xchg(&env->hvf_in_guest, false)) {
> > >>            wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> > >>                  rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> > >>                    | VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> > >>    }
> > >>
> > >> In the main loop you do:
> > >>
> > >>    atomic_set(&env->hvf_guest_mode, true);
> > >>    smp_mb();
> > >>    hv_vcpu_run(...);
> > >>    atomic_set(&env->hvf_guest_mode, false);
> > >>
> > >> and in the preemption timer vmexit handler:
> > >>
> > >>            wvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS,
> > >>                  rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS)
> > >>                    & ~VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER);
> > >>
> > >
>
>

[-- Attachment #2: Type: text/html, Size: 3727 bytes --]