Re: [Qemu-devel] [PATCH 3/3] Plumb the HAXM-based hardware acceleration support

[Paolo Bonzini]

> diff --git a/cpu-exec.c b/cpu-exec.c
> index 4188fed..4bd238b 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c

All this should not be needed anymore with unrestricted guest support.

> diff --git a/cpus.c b/cpus.c
> index fc78502..6e0f572 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -35,6 +35,7 @@
>  #include "sysemu/dma.h"
>  #include "sysemu/hw_accel.h"
>  #include "sysemu/kvm.h"
> +#include "sysemu/hax.h"
>  #include "qmp-commands.h"
>  #include "exec/exec-all.h"
>  
> @@ -1221,6 +1222,52 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)
>      return NULL;
>  }
>  
> +static void *qemu_hax_cpu_thread_fn(void *arg)
> +{
> +    CPUState *cpu = arg;
> +    int r;
> +    qemu_thread_get_self(cpu->thread);
> +    qemu_mutex_lock(&qemu_global_mutex);
> +
> +    cpu->thread_id = qemu_get_thread_id();
> +    cpu->created = true;
> +    cpu->halted = 0;
> +    current_cpu = cpu;
> +
> +    hax_init_vcpu(cpu);
> +    qemu_cond_signal(&qemu_cpu_cond);
> +
> +    while (1) {
> +        if (cpu_can_run(cpu)) {
> +            r = hax_smp_cpu_exec(cpu);
> +            if (r == EXCP_DEBUG) {
> +                cpu_handle_guest_debug(cpu);
> +            }
> +        }
> +
> +        while (cpu_thread_is_idle(cpu)) {
> +            qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
> +        }
> +
> +        qemu_wait_io_event_common(cpu);
> +    }
> +    return NULL;
> +}
> +
> +
> +static void qemu_cpu_kick_no_halt(void)
> +{
> +    CPUState *cpu;
> +    /* Ensure whatever caused the exit has reached the CPU threads before
> +     * writing exit_request.
> +     */
> +    atomic_mb_set(&exit_request, 1);
> +    cpu = atomic_mb_read(&tcg_current_cpu);
> +    if (cpu) {
> +        cpu_exit(cpu);
> +    }
> +}
> +
>  static void qemu_cpu_kick_thread(CPUState *cpu)
>  {
>  #ifndef _WIN32
> @@ -1235,28 +1282,52 @@ static void qemu_cpu_kick_thread(CPUState *cpu)
>          fprintf(stderr, "qemu:%s: %s", __func__, strerror(err));
>          exit(1);
>      }
> -#else /* _WIN32 */
> -    abort();
> -#endif
> -}
>  
> -static void qemu_cpu_kick_no_halt(void)
> -{
> -    CPUState *cpu;
> -    /* Ensure whatever caused the exit has reached the CPU threads before
> -     * writing exit_request.
> +#ifdef CONFIG_DARWIN
> +    /* The cpu thread cannot catch it reliably when shutdown the guest on 
> Mac.
> +     * We can double check it and resend it
>       */
> -    atomic_mb_set(&exit_request, 1);
> -    cpu = atomic_mb_read(&tcg_current_cpu);
> -    if (cpu) {
> -        cpu_exit(cpu);
> +    if (!exit_request)
> +        qemu_cpu_kick_no_halt();

This must be a conflict resolved wrong.  exit_request is never read by
the HAX code.

> +    if (hax_enabled() && hax_ug_platform())
> +        cpu->exit_request = 1;
> +#endif
> +#else /* _WIN32 */
> +    if (!qemu_cpu_is_self(cpu)) {
> +        CONTEXT tcgContext;
> +
> +        if (SuspendThread(cpu->hThread) == (DWORD)-1) {
> +            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
> +                    GetLastError());
> +            exit(1);
> +        }
> +
> +        /* On multi-core systems, we are not sure that the thread is actually
> +         * suspended until we can get the context.
> +         */
> +        tcgContext.ContextFlags = CONTEXT_CONTROL;
> +        while (GetThreadContext(cpu->hThread, &tcgContext) != 0) {
> +            continue;
> +        }
> +
> +        qemu_cpu_kick_no_halt();
> +        if (hax_enabled() && hax_ug_platform())
> +            cpu->exit_request = 1;
> +
> +        if (ResumeThread(cpu->hThread) == (DWORD)-1) {
> +            fprintf(stderr, "qemu:%s: GetLastError:%lu\n", __func__,
> +                    GetLastError());
> +            exit(1);
> +        }

This is weird too.  The SuspendThread/ResumeThread dance comes from an
old version of QEMU.  It is not needed anymore and, again,
qemu_cpu_kick_no_halt would only be useful if hax_ug_platform() is false.

Here, Linux/KVM uses a signal and pthread_kill.  It's probably good for
HAX on Darwin too, but not on Windows.  It's possible that
SuspendThread/ResumeThread just does the right thing (sort of by
chance), in which case you can just keep it (removing
qemu_cpu_kick_no_halt).  However, there is a hax_raise_event in patch 2
that is unused.  If you can figure out how to use it it would be better.



> @@ -1617,6 +1618,21 @@ static void ram_block_add(RAMBlock *new_block, Error 
> **errp)
>          } else {
>              new_block->host = phys_mem_alloc(new_block->max_length,
>                                               &new_block->mr->align);
> +            /*
> +             * In Hax, the qemu allocate the virtual address, and HAX kernel
> +             * populate the memory with physical memory. Currently we have no
> +             * paging, so user should make sure enough free memory in advance
> +             */
> +            if (hax_enabled()) {
> +                int ret;
> +                ret = hax_populate_ram((uint64_t)(uintptr_t)new_block->host,
> +                                       new_block->max_length);
> +                if (ret < 0) {
> +                    error_setg(errp, "Hax failed to populate ram");
> +                    return;
> +                }
> +            }

Please try removing this block and instead starting QEMU with
-mem-prealloc.  If it works, remove hax_populate_ram and just set
mem_prealloc to 1 in hax_accel_init.

>              if (!new_block->host) {
>                  error_setg_errno(errp, errno,
>                                   "cannot set up guest memory '%s'",
> diff --git a/hw/intc/apic_common.c b/hw/intc/apic_common.c
> index d78c885..dd4cdc8 100644
> --- a/hw/intc/apic_common.c
> +++ b/hw/intc/apic_common.c
> @@ -316,9 +316,10 @@ static void apic_common_realize(DeviceState *dev, Error 
> **errp)
>  
>      /* Note: We need at least 1M to map the VAPIC option ROM */
>      if (!vapic && s->vapic_control & VAPIC_ENABLE_MASK &&
> -        ram_size >= 1024 * 1024) {
> +        kvm_enabled() && ram_size >= 1024 * 1024) {

This should rather be !hax_enabled(), despite the historical name
mentions KVM.

>          vapic = sysbus_create_simple("kvmvapic", -1, NULL);
>      }
> +
>      s->vapic = vapic;
>      if (apic_report_tpr_access && info->enable_tpr_reporting) {
>          info->enable_tpr_reporting(s, true);
> diff --git a/target-i386/seg_helper.c b/target-i386/seg_helper.c
> index fb79f31..25b6003 100644
> --- a/target-i386/seg_helper.c
> +++ b/target-i386/seg_helper.c
> @@ -25,6 +25,7 @@
>  #include "exec/exec-all.h"
>  #include "exec/cpu_ldst.h"
>  #include "exec/log.h"
> +#include "sysemu/hax.h"
>  
>  //#define DEBUG_PCALL
>  
> @@ -1336,6 +1337,10 @@ bool x86_cpu_exec_interrupt(CPUState *cs, int 
> interrupt_request)
>              !(env->hflags & HF_SMM_MASK)) {
>              cpu_svm_check_intercept_param(env, SVM_EXIT_SMI, 0);
>              cs->interrupt_request &= ~CPU_INTERRUPT_SMI;
> +#ifdef CONFIG_HAX
> +            if (hax_enabled())
> +                cs->hax_vcpu->resync = 1;
> +#endif

Not needed for UG mode.

>              do_smm_enter(cpu);
>              ret = true;
>          } else if ((interrupt_request & CPU_INTERRUPT_NMI) &&
> diff --git a/target-i386/translate.c b/target-i386/translate.c
> index 324103c..e027896 100644
> --- a/target-i386/translate.c
> +++ b/target-i386/translate.c

Same.

> @@ -4060,6 +4066,7 @@ int main(int argc, char **argv, char **envp)
>      machine_class = select_machine();
>  
>      set_memory_options(&ram_slots, &maxram_size, machine_class);
> +    hax_pre_init(ram_size);

It should be possible to merge this with hax_accel_init.

>      os_daemonize();
>  
> @@ -4418,8 +4425,8 @@ int main(int argc, char **argv, char **envp)
>  
>      cpu_ticks_init();
>      if (icount_opts) {
> -        if (kvm_enabled() || xen_enabled()) {
> -            error_report("-icount is not allowed with kvm or xen");
> +        if (kvm_enabled() || xen_enabled() || hax_enabled()) {
> +            error_report("-icount is not allowed with kvm or xen or hax");

Let's say it's only allowed with TCG. :)  Again, thanks to UG mode if
hax_enabled() you'll have !tcg_enabled().

Paolo

>              exit(1);
>          }
>          configure_icount(icount_opts, &error_abort);
> @@ -4555,6 +4562,10 @@ int main(int argc, char **argv, char **envp)
>  
>      numa_post_machine_init();
>  
> +    if (hax_enabled()) {
> +        hax_sync_vcpus();
> +    }
> +
>      if (qemu_opts_foreach(qemu_find_opts("fw_cfg"),
>                            parse_fw_cfg, fw_cfg_find(), NULL) != 0) {
>          exit(1);
>