Re: Reset problem vs. MMIO emulation, hypercalls, etc...

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Fri, 2012-08-03 at 14:41 -0300, Marcelo Tosatti wrote:

> > Hrm, except that doing KVM_RUN with a signal is very cumbersome to do
> > and I couldn't quite find the logic in qemu to do it ... but I might
> > just have missed it. I can see indeed that in the migration case you
> > want to actually complete the operation rather than just "abort it".
> > 
> > Any chance you can point me to the code that performs that trick qemu
> > side for migration ?
> 
> kvm-all.c:
> 
>         kvm_arch_pre_run(env, run);
>         if (env->exit_request) {
>             DPRINTF("interrupt exit requested\n");
>             /*
>              * KVM requires us to reenter the kernel after IO exits to
>              * complete
>              * instruction emulation. This self-signal will ensure that
>              * we
>              * leave ASAP again.
>              */
>             qemu_cpu_kick_self();
>         }
> 
> 
> > Anthony seems to think that for reset we can just abort the operation
> > state in the kernel when the MP state changes.

Ok, I see now, this is scary really... set a flag somewhere, pray that
we are in the right part of the loop on elsewhere... oh well.

In fact there's some fun (through harmless) bits to be found, look at
x86 for example:

        if (env->exception_injected == EXCP08_DBLE) {
            /* this means triple fault */
            qemu_system_reset_request();
            env->exit_request = 1;
            return 0;
        }

Well, qemu_system_reset_request() calls cpu_stop_current() which calls
cpu_exit() which also does:

   env->exit_request = 1;
 
So obviously it will be well set by that time :-)

Now I can see how having it set during kvm_arch_process_async_events()
will work as this is called right before the run loop. Similarily,
EXIT_MMIO and EXIT_IO would work so they are a non issue.

Our problem I suspect with PAPR doing resets via hcalls is that
our kvm_arch_handle_exit() does:

    case KVM_EXIT_PAPR_HCALL:
        dprintf("handle PAPR hypercall\n");
        run->papr_hcall.ret = spapr_hypercall(env, run->papr_hcall.nr,
                                              run->papr_hcall.args);
        ret = 1;
        break;

See the ret = 1 here ? That means that the caller (kvm_cpu_exec.c main
loop) will exit immediately upon return. If we had instead returned 0,
it would have looped, seen the "exit_requested" set by
qemu_system_reset_request(), which would have then done the signal + run
trick, completed the hypercall, and returned this time in a clean state.

So it seems (I don't have the machine at hand to test right now) that by
just changing that ret = 1 to ret = 0, we might be fixing our problem,
including the case where another vcpu is taking a hypercall at the time
of the reset (this other CPU will also need to complete the operation
before stopping).

David, is there any reason why you did ret = 1 to begin with ? For
things like reset etc... the exit will happen as a result of
env->exit_requested being set by cpu_exit().

Are there other cases where you wish the hcall to go back to the main
loop ? In that case, shouldn't it set env->exit_requested rather than
returning 1 at that point ? (H_CEDE for example ?)

Paul, David, I don't have time to test that before Tuesday (I'm away on
monday) but if you have a chance, revert the kernel change we did and
try this qemu patch for reset:

--- a/target-ppc/kvm.c
+++ b/target-ppc/kvm.c
@@ -766,7 +766,7 @@ int kvm_arch_handle_exit(CPUPPCState *env, struct
kvm_run *r
         dprintf("handle PAPR hypercall\n");
         run->papr_hcall.ret = spapr_hypercall(env, run->papr_hcall.nr,
                                               run->papr_hcall.args);
-        ret = 1;
+        ret = 0;
         break;
 #endif
     default:

It might also need something like:

diff --git a/hw/spapr_hcall.c b/hw/spapr_hcall.c
index a5990a9..d4d7fb0 100644
--- a/hw/spapr_hcall.c
+++ b/hw/spapr_hcall.c
@@ -545,6 +545,7 @@ static target_ulong h_cede(CPUPPCState *env,
sPAPREnvironmen
     hreg_compute_hflags(env);
     if (!cpu_has_work(env)) {
         env->halted = 1;
+        env->exit_request = 1;
     }
     return H_SUCCESS;
 }

Though I don't think H_CEDE ever goes down to qemu, does it ?

Cheers,
Ben.