From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:35002) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbvaI-0005qi-Dz for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:59:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VbvaC-00037c-Fs for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:59:02 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48160) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VbvaC-00037O-8Z for qemu-devel@nongnu.org; Thu, 31 Oct 2013 12:58:56 -0400 Date: Thu, 31 Oct 2013 19:01:38 +0200 From: "Michael S. Tsirkin" Message-ID: <20131031170138.GA10940@redhat.com> References: <20131031145234.GC9948@redhat.com> <52726FAA.6000502@redhat.com> <20131031150955.GE9948@redhat.com> <52727695.8080007@redhat.com> <20131031154556.GB10424@redhat.com> <52727D97.3070209@redhat.com> <20131031161404.GA10710@redhat.com> <52728294.9050305@redhat.com> <20131031162652.GA10789@redhat.com> <52728790.7010404@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <52728790.7010404@redhat.com> Subject: Re: [Qemu-devel] pvpanic plans? List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: pkrempa@redhat.com, lersek@redhat.com, marcel.a@redhat.com, libvir-list@redhat.com, Hu Tao , qemu-devel@nongnu.org, armbru@redhat.com, rhod@redhat.com, kraxel@redhat.com, anthony@codemonkey.ws, lcapitulino@redhat.com, afaerber@suse.de On Thu, Oct 31, 2013 at 05:38:40PM +0100, Paolo Bonzini wrote: > Il 31/10/2013 17:26, Michael S. Tsirkin ha scritto: > > On Thu, Oct 31, 2013 at 05:17:24PM +0100, Paolo Bonzini wrote: > >> Il 31/10/2013 17:14, Michael S. Tsirkin ha scritto: > >>>> PANICKED->DEBUG was added by commit bc7d0e667. That commit can be > >>>> reverted if the panicked state is removed from runstate_needs_reset. > >>> > >>> Okay so let's drop the code duplication and explicitly make > >>> them the same? > >>> > >>> Signed-off-by: Michael S. Tsirkin > >>> > >>> > >>> diff --git a/vl.c b/vl.c > >>> index 46c29c4..e12d317 100644 > >>> --- a/vl.c > >>> +++ b/vl.c > >>> @@ -638,10 +638,6 @@ static const RunStateTransition runstate_transitions_def[] = { > >>> { RUN_STATE_WATCHDOG, RUN_STATE_RUNNING }, > >>> { RUN_STATE_WATCHDOG, RUN_STATE_FINISH_MIGRATE }, > >>> > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED }, > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE }, > >>> - { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG }, > >>> - > >>> { RUN_STATE_MAX, RUN_STATE_MAX }, > >>> }; > >>> > >>> @@ -660,6 +656,12 @@ static void runstate_init(void) > >>> > >>> for (p = &runstate_transitions_def[0]; p->from != RUN_STATE_MAX; p++) { > >>> runstate_valid_transitions[p->from][p->to] = true; > >>> + /* Panicked state is same as paused, we only made it different so > >>> + * management can detect a panic. > >>> + */ > >>> + if (p->from == RUN_STATE_PAUSED) { > >>> + runstate_valid_transitions[RUN_STATE_GUEST_PANICKED][p->to] = true; > >> > >> It makes only sense to me if you do that for IO_ERROR and WATCHDOG as > >> well, and perhaps there are others I'm missing. Just add a comment > >> before runstate_transitions_def's entries for PANICKED, IO_ERROR and > >> WATCHDOG. > >> > >> But again, it is somewhat separate from the issue at hand, which is to > >> finally make pvpanic usable and hopefully before 1.7. > >> > >> Paolo > > > > The issue is that you can't continue from panicked state. > > You should be able to do that without going through paused. > > Yes, that's what my patch (posted the link before) does: > > - { RUN_STATE_GUEST_PANICKED, RUN_STATE_PAUSED }, > + { RUN_STATE_GUEST_PANICKED, RUN_STATE_RUNNING }, > { RUN_STATE_GUEST_PANICKED, RUN_STATE_FINISH_MIGRATE }, > - { RUN_STATE_GUEST_PANICKED, RUN_STATE_DEBUG }, > Anyway I agree with your patch. How about we drop RUN_STATE_DEBUG and drop RUN_STATE_GUEST_PANICKED from need reset list? > Comments don't compile, but are also easier to understand than code. > Special logic in runstate_init is unnecessarily complicated, for a table > that hardly sees any change. English works better, whoever modifies the > table has it under their eyes. > > Paolo