From patchwork Fri Aug 3 22:30:08 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Benjamin Herrenschmidt X-Patchwork-Id: 1272501 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id A22F13FC71 for ; Fri, 3 Aug 2012 22:30:29 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753846Ab2HCWa1 (ORCPT ); Fri, 3 Aug 2012 18:30:27 -0400 Received: from gate.crashing.org ([63.228.1.57]:42524 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753577Ab2HCWa0 (ORCPT ); Fri, 3 Aug 2012 18:30:26 -0400 Received: from [127.0.0.1] (localhost.localdomain [127.0.0.1]) by gate.crashing.org (8.14.1/8.13.8) with ESMTP id q73MU7fD005279; Fri, 3 Aug 2012 17:30:09 -0500 Message-ID: <1344033008.24037.67.camel@pasglop> Subject: Re: Reset problem vs. MMIO emulation, hypercalls, etc... From: Benjamin Herrenschmidt To: Marcelo Tosatti Cc: Avi Kivity , kvm@vger.kernel.org, Alexander Graf , Paul Mackerras , kvm-ppc@vger.kernel.org, David Gibson Date: Sat, 04 Aug 2012 08:30:08 +1000 In-Reply-To: <20120803174113.GA13174@amt.cnet> References: <1343791031.16975.41.camel@pasglop> <501A740F.2000000@redhat.com> <1343938818.6911.9.camel@pasglop> <20120803174113.GA13174@amt.cnet> X-Mailer: Evolution 3.2.3-0ubuntu6 Mime-Version: 1.0 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.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: } Though I don't think H_CEDE ever goes down to qemu, does it ? Cheers, Ben. --- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- 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;