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

[Maxime Villard <max@m00nbsd.net>]

Hi

Le 03/02/2020 à 12:51, Philippe Mathieu-Daudé a écrit :
>> +static void
>> +nvmm_io_callback(struct nvmm_io *io)
>> +{
>> +    MemTxAttrs attrs = { 0 };
>> +    int ret;
>> +
>> +    ret = address_space_rw(&address_space_io, io->port, attrs, io->data,
>> +        io->size, !io->in);
>> +    if (ret != MEMTX_OK) {
>> +        error_report("NVMM: I/O Transaction Failed "
>> +            "[%s, port=%u, size=%zu]", (io->in ? "in" : "out"),
>> +            io->port, io->size);
>> +    }
>> +
>> +    /* XXX Needed, otherwise infinite loop. */
> 
> This seems OK, why the XXX in comment?
> 
>> +    current_cpu->vcpu_dirty = false;
>> +}

Because the other implementations don't do that and avoid the infinite loop
somehow. I didn't completely understand why, so I left an XXX.

>> +static int
>> +nvmm_vcpu_loop(CPUState *cpu)
>> +{
>> +    struct CPUX86State *env = (CPUArchState *)cpu->env_ptr;
>> +    struct nvmm_machine *mach = get_nvmm_mach();
>> +    struct qemu_vcpu *qcpu = get_qemu_vcpu(cpu);
>> +    struct nvmm_vcpu *vcpu = &qcpu->vcpu;
>> +    X86CPU *x86_cpu = X86_CPU(cpu);
>> +    struct nvmm_vcpu_exit *exit = vcpu->exit;
>> +    int ret;
>> +
>> +    /*
>> +     * Some asynchronous events must be handled outside of the inner
>> +     * VCPU loop. They are handled here.
>> +     */
>> +    if (cpu->interrupt_request & CPU_INTERRUPT_INIT) {
>> +        nvmm_cpu_synchronize_state(cpu);
>> +        do_cpu_init(x86_cpu);
>> +        /* XXX: reset the INT/NMI windows */
> 
> What is the problem?

The int/nmi windows are not set back to the reset state. Not complicated
to do but I never got around to doing it. This can easily be addressed
in a future patch.

Maxime