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

[Vincent Palatin <vpalatin@chromium.org>]

On Tue, Nov 8, 2016 at 9:37 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>> 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.

Removed in v2

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

Maybe, it already exists in the predating Android branch.
I will need to sort out this.

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

Yes I knew I was re-introducing removed code, that's why my original
message was reading "I'm not so happy with the qemu_cpu_kick_thread
mess in cpus.c, if somebody can help/advise."
To be fair, your original commit message removing it was saying that
this code was no longer useful for TCG, as there is no working support
for KVM on Windows, I was not sure whether it might be useful in this
case.

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

I still need to figure out this mess, I haven't made a working solution yet.

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

it's not working, later hax_set_ram() is unhappy about what it is
finding the mappings.
By the way, even if it had worked at startup, under memory pressure,
Windows might have evicted the physical pages (which is not supported
by the HAXM kernel module)

I can try to move this in os_mem_prealloc() if you feel it's cleaner.


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

Updated in v2, thanks for the hint.

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

Removed in v2.

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

Done in v2, I have added a new patch simplifying the init
(i have also realized it contained Android specific code leftovers and
was misusing the 'allowed' property of the AccelClass)

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

Updated in v2.
I actually did not realize earlier that the interpreter mode was
running with 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);
>>