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

[Maxime Villard]

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