Re: [PATCH v4 3/4] Introduce the NVMM impl

[Paolo Bonzini]

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

> +    nseg->attrib.g = __SHIFTOUT(attrib, DESC_G_MASK);

> +        __SHIFTIN((uint32_t)nseg->attrib.g, DESC_G_MASK);

What are __SHIFTOUT and __SHIFTIN?

> 
> +    if (qcpu->int_window_exit) {

Should it assert the condition in the "if" below?

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

> +
> +        return false;
> +    }

Have you tried running kvm-unit-tests?

> +
> +    /* Needed, otherwise infinite loop. */
> +    current_cpu->vcpu_dirty = false;

Can you explain this?

> +        break;
> +    default: /* More MSRs to add? */
> +        val = 0;

I would add at least MSR_IA32_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.

Paolo