[PATCH 0/8] Improve synchronization between QEMU and HVF

[PATCH 0/8] 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.

Unfortunately live snapshots don't seem to work yet but they don't work with
tcg (on macOS) either.

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: Don't duplicate register reset
  i386: hvf: Drop hvf_reset_vcpu()
  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/hvf/hvf.c     | 159 ++++++++++++--------------------------
 target/i386/hvf/vmx.h     |   1 +
 8 files changed, 77 insertions(+), 131 deletions(-)

-- 
2.26.1

[PATCH 1/8] 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 2/8] 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 7317ae06b9..26709677d3 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

Re: [PATCH 2/8] i386: hvf: Move synchronize functions to sysemu

[Claudio Fontana]

Reviewed-by: Claudio Fontana <cfontana@suse.de>

On 6/25/20 12:58 AM, Roman Bolshakov wrote:
> 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 7317ae06b9..26709677d3 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);
>      }
> 

[PATCH 3/8] 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 4/8] 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].

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/hvf/hvf.c | 11 +++++++++++
 4 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/cpus.c b/cpus.c
index 26709677d3..36f38ce5c8 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1783,10 +1783,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/hvf/hvf.c b/target/i386/hvf/hvf.c
index efe9802962..4d254a477a 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -966,6 +966,17 @@ int hvf_vcpu_exec(CPUState *cpu)
     return ret;
 }
 
+void hvf_vcpu_kick(CPUState *cpu)
+{
+    hv_return_t err;
+
+    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)
-- 
2.26.1

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

[Claudio Fontana]

Hi Roman,

On 6/25/20 12:58 AM, Roman Bolshakov wrote:
> 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].
> 
> 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/hvf/hvf.c | 11 +++++++++++
>  4 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 26709677d3..36f38ce5c8 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1783,10 +1783,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);

could this be moved to qemu_cpu_kick, where we have already the ifs for accelerator types tcg and hax?

Not terribly important if then my cpus-refactoring goes forward, but on its own that should be the proper place for if (hvf_enabled()) I think.



> +    } 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/hvf/hvf.c b/target/i386/hvf/hvf.c
> index efe9802962..4d254a477a 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -966,6 +966,17 @@ int hvf_vcpu_exec(CPUState *cpu)
>      return ret;
>  }
>  
> +void hvf_vcpu_kick(CPUState *cpu)
> +{
> +    hv_return_t err;
> +
> +    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)
> 

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

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 09:07:04AM +0200, Claudio Fontana wrote:
> Hi Roman,
> 
> On 6/25/20 12:58 AM, Roman Bolshakov wrote:
> > 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].
> > 
> > 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/hvf/hvf.c | 11 +++++++++++
> >  4 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 26709677d3..36f38ce5c8 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1783,10 +1783,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);
> 
> could this be moved to qemu_cpu_kick, where we have already the ifs for accelerator types tcg and hax?
> 

Hi Claudio,

I did this because of cpu->thread_kicked which is not set or tested in
qemu_cpu_kick(). It's not used for tcg and mttcg but hax does seem to
use the qemu_cpu_kick_thread() and additionally sets cpu->exit_request
in qemu_cpu_kick(). There's a difference between hax/kvm and hvf, they
use different ways of siginalling the kick. hax/kvm use POSIX signals
while HVF sends an IPI from the host LAPIC to deliver the kick. The
patch highlights the difference.

As far as I understand if thread_kicked is set, multiple cpu kicks are
coalesced until thread_kicked is cleared. So, the answer to your
question: It could be moved to qemu_cpu_kick but then kick debouncing
should be duplicated inside hvf_vcpu_kick().

Regards,
Roman

> Not terribly important if then my cpus-refactoring goes forward, but on its own that should be the proper place for if (hvf_enabled()) I think.
> 
> 
> 
> > +    } 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)) {

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

[Paolo Bonzini]

On 25/06/20 00:58, Roman Bolshakov wrote:
> 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].
> 
> 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/hvf/hvf.c | 11 +++++++++++
>  4 files changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/cpus.c b/cpus.c
> index 26709677d3..36f38ce5c8 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1783,10 +1783,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/hvf/hvf.c b/target/i386/hvf/hvf.c
> index efe9802962..4d254a477a 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -966,6 +966,17 @@ int hvf_vcpu_exec(CPUState *cpu)
>      return ret;
>  }
>  
> +void hvf_vcpu_kick(CPUState *cpu)
> +{
> +    hv_return_t err;
> +
> +    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)
> 

The documentation isn't clear on whether hv_vcpu_interrupt is able to
interrupt a *subsequent* hv_vcpu_run, similar to WHPX
WHvCancelRunVirtualProcessor (is it possible to decompile
hv_vcpu_interrupt and see what it does?).  If not, you can reduce a bit
the race window by setting a variable in cpu, like

	atomic_set(&cpu->deadline, 0);
	hv_vcpu_interrupt(...)

and in the vCPU thread

	hv_vcpu_run_until(..., atomic_read(&cpu->deadline));
	atomic_set(&cpu->deadline, HV_DEADLINE_FOREVER);

Paolo

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

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 12:28:26PM +0200, Paolo Bonzini wrote:
> On 25/06/20 00:58, Roman Bolshakov wrote:
> > 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].
> > 
> > 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/hvf/hvf.c | 11 +++++++++++
> >  4 files changed, 22 insertions(+), 5 deletions(-)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 26709677d3..36f38ce5c8 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1783,10 +1783,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/hvf/hvf.c b/target/i386/hvf/hvf.c
> > index efe9802962..4d254a477a 100644
> > --- a/target/i386/hvf/hvf.c
> > +++ b/target/i386/hvf/hvf.c
> > @@ -966,6 +966,17 @@ int hvf_vcpu_exec(CPUState *cpu)
> >      return ret;
> >  }
> >  
> > +void hvf_vcpu_kick(CPUState *cpu)
> > +{
> > +    hv_return_t err;
> > +
> > +    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)
> > 
> 
> The documentation isn't clear on whether hv_vcpu_interrupt is able to
> interrupt a *subsequent* hv_vcpu_run, similar to WHPX
> WHvCancelRunVirtualProcessor (is it possible to decompile
> hv_vcpu_interrupt and see what it does?).

hv_vcpu_interrupt sends a KICK IPI using mp_cpus_kick() only if the
destination vCPU thread is running as far as I undrestand the
mp_cpus_kick():

void
mp_cpus_kick(cpumask_t cpus)
{
	cpu_t           cpu;
	boolean_t       intrs_enabled = FALSE;

	intrs_enabled = ml_set_interrupts_enabled(FALSE);
	mp_safe_spin_lock(&x86_topo_lock);

	for (cpu = 0; cpu < (cpu_t) real_ncpus; cpu++) {
		if ((cpu == (cpu_t) cpu_number())
		    || ((cpu_to_cpumask(cpu) & cpus) == 0)
		    || !cpu_is_running(cpu)) {
			continue;
		}

		lapic_send_ipi(cpu, LAPIC_VECTOR(KICK));
	}

	simple_unlock(&x86_topo_lock);
	ml_set_interrupts_enabled(intrs_enabled);
}

So, the kick is not delivered to self and in case if destination cpu is
not running. I think it can't interrupt subsequent hv_vcpu_run.

> If not, you can reduce a bit the race window by setting a variable in
> cpu, like
> 
> 	atomic_set(&cpu->deadline, 0);
> 	hv_vcpu_interrupt(...)
> 
> and in the vCPU thread
> 
> 	hv_vcpu_run_until(..., atomic_read(&cpu->deadline));
> 	atomic_set(&cpu->deadline, HV_DEADLINE_FOREVER);
> 

Sure, could you please explain who'll be racing? There's a race if a
kick was sent after VMEXIT, right? So essentially we need a way to
"requeue" a kick that was received outside of hv_vcpu_run to avoid loss
of it?

hv_vcpu_run_until is only available on macOS 10.15+ and we can't use yet
because of three release support rule.
(https://developer.apple.com/documentation/hypervisor/3181548-hv_vcpu_run_until?language=objc)

BTW, I'm totally okay to send v2 if kicks are lost and/or the patch
needs improvements. (and I can address EFER to VMCS Entry Controls
synchronization as well)

Paolo, do you know any particular test in kvm-unit-tests that can
exhibit the issue?

Thanks,
Roman

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

[Paolo Bonzini]

On 25/06/20 17:57, Roman Bolshakov wrote:
> So, the kick is not delivered to self and in case if destination cpu is
> not running. I think it can't interrupt subsequent hv_vcpu_run.

Yes.

>> If not, you can reduce a bit the race window by setting a variable in
>> cpu, like
>>
>> 	atomic_set(&cpu->deadline, 0);
>> 	hv_vcpu_interrupt(...)
>>
>> and in the vCPU thread
>>
>> 	hv_vcpu_run_until(..., atomic_read(&cpu->deadline));
>> 	atomic_set(&cpu->deadline, HV_DEADLINE_FOREVER);
>>
> 
> Sure, could you please explain who'll be racing? There's a race if a
> kick was sent after VMEXIT, right? So essentially we need a way to
> "requeue" a kick that was received outside of hv_vcpu_run to avoid loss
> of it?

Yes.  Note that this is not a new bug, it's pre-existing and it's common
to all hypervisors except KVM/WHPX.  I mean not the QEMU code, it's the
kernel APIs that are broken. :)

One way to do so is to keep the signal, and have the signal handler
enable the preemption timer (with a deadline of 0) in the pin-based
interrupt controls.  Hopefully macOS allows that, especially on 10.15+
where hv_vcpu_run_until probably uses the preemption timer.

> hv_vcpu_run_until is only available on macOS 10.15+ and we can't use yet
> because of three release support rule.
> (https://developer.apple.com/documentation/hypervisor/3181548-hv_vcpu_run_until?language=objc)
> 
> BTW, I'm totally okay to send v2 if kicks are lost and/or the patch
> needs improvements. (and I can address EFER to VMCS Entry Controls
> synchronization as well)
> 
> Paolo, do you know any particular test in kvm-unit-tests that can
> exhibit the issue?

No, it's a race and it's extremely rare, but I point it out because it's
a kernel issue that Apple might want to fix anyway.  It might also be
(depending on how the kernel side is written) that the next scheduler
tick will end up unblocking the vCPU and papering over it.

Paolo

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

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 08:34:14PM +0200, Paolo Bonzini wrote:
> On 25/06/20 17:57, Roman Bolshakov wrote:
> > So, the kick is not delivered to self and in case if destination cpu is
> > not running. I think it can't interrupt subsequent hv_vcpu_run.
> 
> Yes.
> 
> >> If not, you can reduce a bit the race window by setting a variable in
> >> cpu, like
> >>
> >> 	atomic_set(&cpu->deadline, 0);
> >> 	hv_vcpu_interrupt(...)
> >>
> >> and in the vCPU thread
> >>
> >> 	hv_vcpu_run_until(..., atomic_read(&cpu->deadline));
> >> 	atomic_set(&cpu->deadline, HV_DEADLINE_FOREVER);
> >>
> > 
> > Sure, could you please explain who'll be racing? There's a race if a
> > kick was sent after VMEXIT, right? So essentially we need a way to
> > "requeue" a kick that was received outside of hv_vcpu_run to avoid loss
> > of it?
> 
> Yes.  Note that this is not a new bug, it's pre-existing and it's common
> to all hypervisors except KVM/WHPX.  I mean not the QEMU code, it's the
> kernel APIs that are broken. :)
> 
> One way to do so is to keep the signal, and have the signal handler
> enable the preemption timer (with a deadline of 0) in the pin-based
> interrupt controls.  Hopefully macOS allows that, especially on 10.15+
> where hv_vcpu_run_until probably uses the preemption timer.
> 
> > hv_vcpu_run_until is only available on macOS 10.15+ and we can't use yet
> > because of three release support rule.
> > (https://developer.apple.com/documentation/hypervisor/3181548-hv_vcpu_run_until?language=objc)
> > 
> > BTW, I'm totally okay to send v2 if kicks are lost and/or the patch
> > needs improvements. (and I can address EFER to VMCS Entry Controls
> > synchronization as well)
> > 
> > Paolo, do you know any particular test in kvm-unit-tests that can
> > exhibit the issue?
> 
> No, it's a race and it's extremely rare, but I point it out because it's
> a kernel issue that Apple might want to fix anyway.  It might also be
> (depending on how the kernel side is written) that the next scheduler
> tick will end up unblocking the vCPU and papering over it.
> 

Hi Paolo,

I implemented what you proposed using VMX-preemption timer in Pin-based
controls and regular hv_vcpu_run(). It works fine without noticable
regressions, I'll send that in v2.

hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
VM performance significantly compared to explicit setting of
VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
Pro.

macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
declaration for hv_vcpu_run_until(), that's not available 10.15 -
HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
VMX-preeemption counter). Perhaps the performance issue is addressed
there.

Regards,
Roman

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

[Paolo Bonzini]

On 29/06/20 13:31, Roman Bolshakov wrote:
> I implemented what you proposed using VMX-preemption timer in Pin-based
> controls and regular hv_vcpu_run(). It works fine without noticable
> regressions, I'll send that in v2.
> 
> hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
> VM performance significantly compared to explicit setting of
> VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
> observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
> Pro.
> 
> macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
> declaration for hv_vcpu_run_until(), that's not available 10.15 -
> HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
> VMX-preeemption counter). Perhaps the performance issue is addressed
> there.

Possibly.  I'm worried that the preemption-timer trick will fail to run
there, but we'll see.

Paolo

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

[Roman Bolshakov]

On Mon, Jun 29, 2020 at 03:03:20PM +0200, Paolo Bonzini wrote:
> On 29/06/20 13:31, Roman Bolshakov wrote:
> > I implemented what you proposed using VMX-preemption timer in Pin-based
> > controls and regular hv_vcpu_run(). It works fine without noticable
> > regressions, I'll send that in v2.
> > 
> > hv_vcpu_run_until() was also evaluated on macOS 10.15.5 but it degrades
> > VM performance significantly compared to explicit setting of
> > VMX-preepmtion timer value and hv_vcpu_run(). The performance issue was
> > observed on Broadwell-based MacBook Air and Ivy Bridge-based MacBook
> > Pro.
> > 
> > macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
> > declaration for hv_vcpu_run_until(), that's not available 10.15 -
> > HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
> > VMX-preeemption counter). Perhaps the performance issue is addressed
> > there.
> 
> Possibly.  I'm worried that the preemption-timer trick will fail to run
> there, but we'll see.
> 

Well, I've got new VM-exits (caused by zero preemption timer) on either
of my laptops.

-Roman

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

[Paolo Bonzini]

On 29/06/20 15:29, Roman Bolshakov wrote:
>>> macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
>>> declaration for hv_vcpu_run_until(), that's not available 10.15 -
>>> HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
>>> VMX-preeemption counter). Perhaps the performance issue is addressed
>>> there.
>> Possibly.  I'm worried that the preemption-timer trick will fail to run
>> there, but we'll see.
>>
> Well, I've got new VM-exits (caused by zero preemption timer) on either
> of my laptops.

If you have already tried 11.0 that's great.  I was worried that it
would forcibly clear the preemption timer bit when passed
HV_DEADLINE_FOREVER.

Paolo

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

[Roman Bolshakov]

On Mon, Jun 29, 2020 at 03:35:16PM +0200, Paolo Bonzini wrote:
> On 29/06/20 15:29, Roman Bolshakov wrote:
> >>> macOS 11.0 Beta deprecated hv_vcpu_run() and introduced a special
> >>> declaration for hv_vcpu_run_until(), that's not available 10.15 -
> >>> HV_DEADLINE_FOREVER (UINT64_MAX, which is bigger than maximum value of
> >>> VMX-preeemption counter). Perhaps the performance issue is addressed
> >>> there.
> >> Possibly.  I'm worried that the preemption-timer trick will fail to run
> >> there, but we'll see.
> >>
> > Well, I've got new VM-exits (caused by zero preemption timer) on either
> > of my laptops.
> 
> If you have already tried 11.0 that's great.  I was worried that it
> would forcibly clear the preemption timer bit when passed
> HV_DEADLINE_FOREVER.
> 

I did not but I can check how it works on it then, note that I'm not
using hv_vcpu_run_until() because it doesn't have HV_DEADLINE_FOREVER
and it performs poorly on macOS 10.15. My approach is based
hv_vcpu_run() and should hopefully work almost anywhere where
Hypervisor.framework is available because Hypervisor framework exposes
timer value
(https://developer.apple.com/documentation/hypervisor/vmcs_guest_vmx_timer_value)
since macOS 10.10.3+.

I can also test how hv_vcpu_run_until() performs with HV_DEADLINE_FOREVER
on the Beta. And if the performance issues with VMX-preemption timer and
hv_vcpu_run_until() are fixed there.

-Roman

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

[Paolo Bonzini]

On 29/06/20 16:04, Roman Bolshakov wrote:
> My approach is based
> hv_vcpu_run() and should hopefully work almost anywhere where
> Hypervisor.framework is available because Hypervisor framework exposes
> timer value
> (https://developer.apple.com/documentation/hypervisor/vmcs_guest_vmx_timer_value)
> since macOS 10.10.3+.

There are a few other constants for which it would be unwise to write
from userspace, so that's not a big consolation. :)

> I can also test how hv_vcpu_run_until() performs with HV_DEADLINE_FOREVER
> on the Beta. And if the performance issues with VMX-preemption timer and
> hv_vcpu_run_until() are fixed there.

Thanks!  The main thing to test on Big Sur would be: 1) whether the
preemption timer bit in the pin controls "sticks" to 0 after setting it
2) whether the bit reads back as zero after
hv_vcpu_run_until(HV_DEADLINE_FOREVER).

Thanks,

Paolo

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

[Roman Bolshakov]

On Mon, Jun 29, 2020 at 04:18:46PM +0200, Paolo Bonzini wrote:
> On 29/06/20 16:04, Roman Bolshakov wrote:
> > My approach is based
> > hv_vcpu_run() and should hopefully work almost anywhere where
> > Hypervisor.framework is available because Hypervisor framework exposes
> > timer value
> > (https://developer.apple.com/documentation/hypervisor/vmcs_guest_vmx_timer_value)
> > since macOS 10.10.3+.
> 
> There are a few other constants for which it would be unwise to write
> from userspace, so that's not a big consolation. :)
> 

Hi Paolo,

So, I've tried Big Sur Beta and it has exactly the same performance
issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.

> > I can also test how hv_vcpu_run_until() performs with HV_DEADLINE_FOREVER
> > on the Beta. And if the performance issues with VMX-preemption timer and
> > hv_vcpu_run_until() are fixed there.
> 
> Thanks!  The main thing to test on Big Sur would be: 1) whether the
> preemption timer bit in the pin controls "sticks" to 0 after setting it

It does not. If it's set, it stays there.

> 2) whether the bit reads back as zero after
> hv_vcpu_run_until(HV_DEADLINE_FOREVER).
> 

Likewise, it's not cleared if set.

As far as I understand, hv_vcpu_run_until(HV_DEADLINE_FOREVER) works
like hv_vcpu_run() without VMX-preemption timer. Otherwise
hv_vcpu_run_until() implicitly sets VMX-preemption timer Pin-based
control and sets the timer value.

Thanks,
Roman

Here's the patch over v2 that adds support of hv_vcpu_run_until() on Big Sur:
diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 317304aa1d..ad202f7358 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -72,8 +72,12 @@
 #include "sysemu/accel.h"
 #include "target/i386/cpu.h"
 
+#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
+#define HVF_MAX_DEADLINE HV_DEADLINE_FOREVER
+#else
 /* Maximum value of VMX-preemption timer */
 #define HVF_MAX_DEADLINE UINT32_MAX
+#endif
 
 HVFState *hvf_state;
 
@@ -693,6 +697,7 @@ int hvf_vcpu_exec(CPUState *cpu)
     CPUX86State *env = &x86_cpu->env;
     int ret = 0;
     uint64_t rip = 0;
+    hv_return_t r;
 
     if (hvf_process_events(cpu)) {
         return EXCP_HLT;
@@ -718,10 +723,22 @@ int hvf_vcpu_exec(CPUState *cpu)
         /* Use VMX-preemption timer trick only if available */
         if (rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS) &
             VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER) {
+#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
+            r = hv_vcpu_run_until(cpu->hvf_fd,
+                                  atomic_read(&env->hvf_deadline));
+        } else {
+            /*
+             * Equivalent to behaviour of hv_vcpu_run() with VMX-preemption
+             * timer disabled, prone to kick loss.
+             */
+            r = hv_vcpu_run_until(cpu->hvf_fd, HVF_MAX_DEADLINE);
+        }
+#else
             wvmcs(cpu->hvf_fd, VMCS_PREEMPTION_TIMER_VALUE,
                   atomic_read(&env->hvf_deadline));
         }
-        hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
+        r = hv_vcpu_run(cpu->hvf_fd);
+#endif
         atomic_set(&env->hvf_deadline, HVF_MAX_DEADLINE);
         assert_hvf_ok(r);
 

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

[Paolo Bonzini]

Ok, I'll review the patch to see what you've implemented.  Thanks!

Paolo

On 30/06/20 12:12, Roman Bolshakov wrote:
> On Mon, Jun 29, 2020 at 04:18:46PM +0200, Paolo Bonzini wrote:
>> On 29/06/20 16:04, Roman Bolshakov wrote:
>>> My approach is based
>>> hv_vcpu_run() and should hopefully work almost anywhere where
>>> Hypervisor.framework is available because Hypervisor framework exposes
>>> timer value
>>> (https://developer.apple.com/documentation/hypervisor/vmcs_guest_vmx_timer_value)
>>> since macOS 10.10.3+.
>>
>> There are a few other constants for which it would be unwise to write
>> from userspace, so that's not a big consolation. :)
>>
> 
> Hi Paolo,
> 
> So, I've tried Big Sur Beta and it has exactly the same performance
> issue with hv_vcpu_run_until() while hv_vcpu_run() works as good as it
> worked on 10.15.5. I've submitted FB7827341 to Apple wrt the issue.
> 
>>> I can also test how hv_vcpu_run_until() performs with HV_DEADLINE_FOREVER
>>> on the Beta. And if the performance issues with VMX-preemption timer and
>>> hv_vcpu_run_until() are fixed there.
>>
>> Thanks!  The main thing to test on Big Sur would be: 1) whether the
>> preemption timer bit in the pin controls "sticks" to 0 after setting it
> 
> It does not. If it's set, it stays there.
> 
>> 2) whether the bit reads back as zero after
>> hv_vcpu_run_until(HV_DEADLINE_FOREVER).
>>
> 
> Likewise, it's not cleared if set.
> 
> As far as I understand, hv_vcpu_run_until(HV_DEADLINE_FOREVER) works
> like hv_vcpu_run() without VMX-preemption timer. Otherwise
> hv_vcpu_run_until() implicitly sets VMX-preemption timer Pin-based
> control and sets the timer value.
> 
> Thanks,
> Roman
> 
> Here's the patch over v2 that adds support of hv_vcpu_run_until() on Big Sur:
> diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
> index 317304aa1d..ad202f7358 100644
> --- a/target/i386/hvf/hvf.c
> +++ b/target/i386/hvf/hvf.c
> @@ -72,8 +72,12 @@
>  #include "sysemu/accel.h"
>  #include "target/i386/cpu.h"
>  
> +#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
> +#define HVF_MAX_DEADLINE HV_DEADLINE_FOREVER
> +#else
>  /* Maximum value of VMX-preemption timer */
>  #define HVF_MAX_DEADLINE UINT32_MAX
> +#endif
>  
>  HVFState *hvf_state;
>  
> @@ -693,6 +697,7 @@ int hvf_vcpu_exec(CPUState *cpu)
>      CPUX86State *env = &x86_cpu->env;
>      int ret = 0;
>      uint64_t rip = 0;
> +    hv_return_t r;
>  
>      if (hvf_process_events(cpu)) {
>          return EXCP_HLT;
> @@ -718,10 +723,22 @@ int hvf_vcpu_exec(CPUState *cpu)
>          /* Use VMX-preemption timer trick only if available */
>          if (rvmcs(cpu->hvf_fd, VMCS_PIN_BASED_CTLS) &
>              VMCS_PIN_BASED_CTLS_VMX_PREEMPT_TIMER) {
> +#if defined(__MAC_10_16) && __MAC_OS_X_VERSION_MIN_REQUIRED >= __MAC_10_16
> +            r = hv_vcpu_run_until(cpu->hvf_fd,
> +                                  atomic_read(&env->hvf_deadline));
> +        } else {
> +            /*
> +             * Equivalent to behaviour of hv_vcpu_run() with VMX-preemption
> +             * timer disabled, prone to kick loss.
> +             */
> +            r = hv_vcpu_run_until(cpu->hvf_fd, HVF_MAX_DEADLINE);
> +        }
> +#else
>              wvmcs(cpu->hvf_fd, VMCS_PREEMPTION_TIMER_VALUE,
>                    atomic_read(&env->hvf_deadline));
>          }
> -        hv_return_t r  = hv_vcpu_run(cpu->hvf_fd);
> +        r = hv_vcpu_run(cpu->hvf_fd);
> +#endif
>          atomic_set(&env->hvf_deadline, HVF_MAX_DEADLINE);
>          assert_hvf_ok(r);
>  
> 

[PATCH 5/8] 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 most of the 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().

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

diff --git a/target/i386/hvf/hvf.c b/target/i386/hvf/hvf.c
index 4d254a477a..2c4028d08c 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -456,90 +456,12 @@ 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_ENTRY_CTLS, 0);
-    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)
-- 
2.26.1

[PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Roman Bolshakov]

It's worth to have a custom accel-specific reset in x86_cpu_reset() only
if something related to CPUState has to be reset and that can't be done
in post-init or post-reset.

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 | 23 +++++++++++------------
 3 files changed, 11 insertions(+), 16 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 b1b311baa2..bfa3eed9b6 100644
--- a/target/i386/cpu.c
+++ b/target/i386/cpu.c
@@ -6089,9 +6089,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 2c4028d08c..0b2be8de47 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -303,6 +303,17 @@ void hvf_cpu_synchronize_state(CPUState *cpu_state)
 static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg)
 {
     CPUState *cpu_state = cpu;
+    uint64_t pdpte[4] = {0, 0, 0, 0};
+    int i;
+
+    /* Reset IA-32e mode guest (LMA) */
+    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
+
+    /* Initialize PDPTE */
+    for (i = 0; i < 4; i++) {
+        wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
+    }
+
     hvf_put_registers(cpu_state);
     cpu_state->vcpu_dirty = false;
 }
@@ -452,18 +463,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;
-
-    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
-
-    /* Initialize PDPTE */
-    for (i = 0; i < 4; i++) {
-        wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
-    }
-}
-
 void hvf_vcpu_destroy(CPUState *cpu)
 {
     X86CPU *x86_cpu = X86_CPU(cpu);
-- 
2.26.1

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Paolo Bonzini]

On 25/06/20 00:58, Roman Bolshakov wrote:
> +    uint64_t pdpte[4] = {0, 0, 0, 0};
> +    int i;
> +
> +    /* Reset IA-32e mode guest (LMA) */
> +    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
> +

Where is the place (if any...) that calls macvm_set_cr0 and
macvm_set_cr4 from cpu_synchronize_*?  If you have such a place it
should take care of resetting LMA as well.  Assuming that no entry
controls are ever set is quite fragile.

Paolo

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 12:31:49PM +0200, Paolo Bonzini wrote:
> On 25/06/20 00:58, Roman Bolshakov wrote:
> > +    uint64_t pdpte[4] = {0, 0, 0, 0};
> > +    int i;
> > +
> > +    /* Reset IA-32e mode guest (LMA) */
> > +    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS, 0);
> > +
> 
> Where is the place (if any...) that calls macvm_set_cr0 and
> macvm_set_cr4 from cpu_synchronize_*?  If you have such a place it
> should take care of resetting LMA as well.  Assuming that no entry
> controls are ever set is quite fragile.
> 

Hi Paolo,

Yes, there's such a place. post-init and post-reset invoke
hvf_put_registers() and the latter one calls hvf_put_segments().
hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
using the CR0/CR4 from env. So, the reset is relying on generic QEMU
CPUX86State now. LMA in EFER is reset there as well.

I don't know any alternative for PDPTE and VMCS Entry Controls in
CPUX86State, that's why I left explicit reset of the VMCS fields in
post-reset.

Is there an outstanding issue I'm missing?

Regards,
Roman

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Paolo Bonzini]

On 25/06/20 14:36, Roman Bolshakov wrote:
> 
> Yes, there's such a place. post-init and post-reset invoke
> hvf_put_registers() and the latter one calls hvf_put_segments().
> hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
> using the CR0/CR4 from env. So, the reset is relying on generic QEMU
> CPUX86State now. LMA in EFER is reset there as well.

Ok, do you want to send a follow-up or a v2 of this?

> I don't know any alternative for PDPTE and VMCS Entry Controls in
> CPUX86State, that's why I left explicit reset of the VMCS fields in
> post-reset.

VMCS entry controls should be handled by macvm_set_cr0 as well, because
QEMU does not use any except for the LMA bit.  They are initialized zero

    wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));

but in practice the last argument ends up being zero all the time.

PDPTEs are not a problem, because they are not used after reset (only if
CR4.PAE=CR4.PG=1 and EFER.LME=0).

Thanks,

Paolo

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 03:30:38PM +0200, Paolo Bonzini wrote:
> On 25/06/20 14:36, Roman Bolshakov wrote:
> > 
> > Yes, there's such a place. post-init and post-reset invoke
> > hvf_put_registers() and the latter one calls hvf_put_segments().
> > hvf_put_segments() sets CR4 and CR0 via macvm_set_cr0/macvm_set_cr4
> > using the CR0/CR4 from env. So, the reset is relying on generic QEMU
> > CPUX86State now. LMA in EFER is reset there as well.
> 
> Ok, do you want to send a follow-up or a v2 of this?
> 

I'm still trying to understand what I should do :)

> > I don't know any alternative for PDPTE and VMCS Entry Controls in
> > CPUX86State, that's why I left explicit reset of the VMCS fields in
> > post-reset.
> 
> VMCS entry controls should be handled by macvm_set_cr0 as well, because
> QEMU does not use any except for the LMA bit.  They are initialized zero
> 
>     wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
> 	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));
> 
> but in practice the last argument ends up being zero all the time.
> 

macvm_set_cr0() sets/clears LMA in entry controls only in case of
transitions into/out of long mode in enter_long_mode() in
exit_long_mode(), respectively. But macvm_set_cr0() doesn't load
EFER.LMA from CPUX86State into VMCS entry controls during reset and
that's where hvf_put_registers() might not behave properly.

As far as I understand you propose to drop explicit LMA reset in
post-reset and rather impove synchronization between efer and entry
controls in macvm_set_cr0(), right? In that case I don't see a
regression in the series, and if possible I'd prefer a follow up patch
for the issue.

> PDPTEs are not a problem, because they are not used after reset (only if
> CR4.PAE=CR4.PG=1 and EFER.LME=0).
> 

Ok, good, then we're leaving PDPTE initialization as is in post-reset.

Thanks,
Roman

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Paolo Bonzini]

On 25/06/20 17:02, Roman Bolshakov wrote:
> macvm_set_cr0() sets/clears LMA in entry controls only in case of
> transitions into/out of long mode in enter_long_mode() in
> exit_long_mode(), respectively. But macvm_set_cr0() doesn't load
> EFER.LMA from CPUX86State into VMCS entry controls during reset and
> that's where hvf_put_registers() might not behave properly.
> 
> As far as I understand you propose to drop explicit LMA reset in
> post-reset and rather impove synchronization between efer and entry
> controls in macvm_set_cr0(), right? In that case I don't see a
> regression in the series, and if possible I'd prefer a follow up patch
> for the issue.

Indeed it's not a regression.  Thanks!

Paolo

Re: [PATCH 6/8] i386: hvf: Drop hvf_reset_vcpu()

[Roman Bolshakov]

On Thu, Jun 25, 2020 at 03:30:38PM +0200, Paolo Bonzini wrote:
> On 25/06/20 14:36, Roman Bolshakov wrote:
> > I don't know any alternative for PDPTE and VMCS Entry Controls in
> > CPUX86State, that's why I left explicit reset of the VMCS fields in
> > post-reset.
> 
> VMCS entry controls should be handled by macvm_set_cr0 as well, because
> QEMU does not use any except for the LMA bit.  They are initialized zero
> 
>     wvmcs(cpu->hvf_fd, VMCS_ENTRY_CTLS,
> 	  cap2ctrl(hvf_state->hvf_caps->vmx_cap_entry, 0));
> 
> but in practice the last argument ends up being zero all the time.
> 
> PDPTEs are not a problem, because they are not used after reset (only if
> CR4.PAE=CR4.PG=1 and EFER.LME=0).
> 

Paolo, I'm not sure if I properly understand it yet but I don't see any
specific requirements on PDPTE's reset. SDM says that it should be valid
only if PAE is used as documented in 26.3.1.6 Checks on Guest
Page-Directory-Pointer-Table Entries:
 "A VM entry to a guest that does not use PAE paging does not check the
 validity of any PDPTEs.

 A VM entry that checks the validity of the PDPTEs uses the same checks
 that are used when CR3 is loaded with MOV to CR3 when PAE paging is in
 use. If MOV to CR3 would cause a general-protection exception due to
 the PDPTEs that would be loaded (e.g., because a reserved bit is set),
 the VM entry fails."

That means we can drop PDPTE initialization in hv_vcpu_reset() as well.
Perhaps, I can try that and check if Windows XP with PAE support works
well alright across resets. macvm_set_cr0() takes care of setting PDPTEs
upon the entry into PAE mode.

Regards,
Roman

[PATCH 7/8] 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 0b2be8de47..18fe843834 100644
--- a/target/i386/hvf/hvf.c
+++ b/target/i386/hvf/hvf.c
@@ -282,27 +282,24 @@ 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;
     uint64_t pdpte[4] = {0, 0, 0, 0};
     int i;
 
@@ -314,26 +311,25 @@ static void do_hvf_cpu_synchronize_post_reset(CPUState *cpu, run_on_cpu_data arg
         wvmcs(cpu->hvf_fd, VMCS_GUEST_PDPTE0 + i * 2, pdpte[i]);
     }
 
-    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 8/8] 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 01e6b3fefe..f54a50cdb2 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 0/8] Improve synchronization between QEMU and HVF

[Paolo Bonzini]

On 25/06/20 00:58, Roman Bolshakov wrote:
> 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.
> 
> Unfortunately live snapshots don't seem to work yet but they don't work with
> tcg (on macOS) either.

Queued, thanks!  Synchronization of special registers is the obvious
next step (hint, hint!).

Cameron perhaps can also guide us with respect to the CPU kick race that
I mentioned in my review of patch 4.  A full fix is not possible without
help from Hypervisor.framework, but using hv_vcpu_run_until will already
tighten the window for the race.

Thanks,

Paolo