Le 02/03/2020 à 19:13, Paolo Bonzini a écrit : > On 06/02/20 22:32, Kamil Rytarowski wrote: >> +get_qemu_vcpu(CPUState *cpu) >> +{ >> + return (struct qemu_vcpu *)cpu->hax_vcpu; >> +} > > Please make hax_vcpu a void * and rename it to "accel_data".
NVMM reproduces the existing logic in the other accelerators. I agree that it should be "accel_data" with void *, but that should be done in all accelerators in a separate commit, unrelated to NVMM. >> + nseg->attrib.g = __SHIFTOUT(attrib, DESC_G_MASK); > >> + __SHIFTIN((uint32_t)nseg->attrib.g, DESC_G_MASK); > > What are __SHIFTOUT and __SHIFTIN? They are macros in NetBSD. >> + if (qcpu->int_window_exit) { > > Should it assert the condition in the "if" below? No, because if int_window_exit is set, then state->intr.int_window_exiting is set too, so there is no point doing get+set. >> + return false; >> + } >> + >> + if (qcpu->int_shadow || !(env->eflags & IF_MASK)) { >> + struct nvmm_x64_state *state = vcpu->state; >> + >> + /* Exit on interrupt window. */ >> + nvmm_vcpu_getstate(mach, vcpu, NVMM_X64_STATE_INTR); >> + state->intr.int_window_exiting = 1; >> + nvmm_vcpu_setstate(mach, vcpu, NVMM_X64_STATE_INTR); > > ... and should this set qcpu->int_window_exit? Mmh, maybe. Not a big problem though, because at worst it just means we set int_window_exiting to one while it was already one. I'll think about that for a future commit. (I'm not immediately able to test.) >> + >> + return false; >> + } > > Have you tried running kvm-unit-tests? I didn't know kvm-unit-tests (until now). I developed my own tests and ran them in qemu-nvmm. But good to know, I'll try these tests. >> + /* Needed, otherwise infinite loop. */ >> + current_cpu->vcpu_dirty = false; > > Can you explain this? If vcpu_dirty remains true, we land here in the next iteration of the loop: if (cpu->vcpu_dirty) { nvmm_set_registers(cpu); cpu->vcpu_dirty = false; } And the (now updated) register values are lost. The guest stays on the same instruction. >> + break; >> + default: /* More MSRs to add? */ >> + val = 0; > > I would add at least MSR_IA32_TSC. MSR_IA32_TSC is handled by the kernel, that's why it isn't there. The values do get synced: state->msrs[NVMM_X64_MSR_TSC] = env->tsc; ... env->tsc = state->msrs[NVMM_X64_MSR_TSC]; >> + if (qcpu->stop) { >> + cpu->exception_index = EXCP_INTERRUPT; >> + qcpu->stop = false; >> + ret = 1; >> + break; >> + } >> + >> + nvmm_vcpu_pre_run(cpu); >> + >> + if (atomic_read(&cpu->exit_request)) { >> + qemu_cpu_kick_self(); >> + } >> + > > This is racy without something like KVM's immediate_exit mechanism. > This should be fixed in NVMM. I don't immediately see how this is racy. It reproduces the existing logic found in whpx-all.c, and if there is a real problem it can be fixed in a future commit along with WHPX. Maxime