pvpanic plans?

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 <mst@redhat.com>
> >>>
> >>>
> >>> 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 },
> 
> 
> 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

But code duplication is bad. I think IO error for example
is broken in that you can't pause but can run then pause.
Seems strange.
Internal error has same bug as panicked.

So it's the same bug for a bunch of states, let's just
have a way to say "this is same as paused".
How's this?

Il 31/10/2013 17:48, Michael S. Tsirkin ha scritto:
> But code duplication is bad.

So should we make a table of IO_ERROR-like states to avoid code
duplication?  You have to draw a line somewhere...

> I think IO error for example
> is broken in that you can't pause but can run then pause.
> Seems strange.

"cont" moves you out of IO_ERROR.  IO_ERROR is already a non-running
state (all states except RUNNING are non-running), "stop" is a no-op in
non-running states.  I don't like it that much either, but it works.

Paolo

On Thu, Oct 31, 2013 at 05:52:11PM +0100, Paolo Bonzini wrote:
> Il 31/10/2013 17:48, Michael S. Tsirkin ha scritto:
> > But code duplication is bad.
> 
> So should we make a table of IO_ERROR-like states to avoid code
> duplication?  You have to draw a line somewhere...
> 
> > I think IO error for example
> > is broken in that you can't pause but can run then pause.
> > Seems strange.
> 
> "cont" moves you out of IO_ERROR.  IO_ERROR is already a non-running
> state (all states except RUNNING are non-running), "stop" is a no-op in
> non-running states.  I don't like it that much either, but it works.
> 
> Paolo

Interesting.  Why do we have
-    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
then?

Il 31/10/2013 18:00, Michael S. Tsirkin ha scritto:
> Interesting.  Why do we have
> -    { RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
> then?

It's only for non-resumable states (such as pvpanic right now).

It's used here:

    if (qemu_reset_requested()) {
        pause_all_vcpus();
        cpu_synchronize_all_states();
        qemu_system_reset(VMRESET_REPORT);
        resume_all_vcpus();
        if (runstate_needs_reset()) {
            runstate_set(RUN_STATE_PAUSED);
        }
    }

Don't ask me what's happening with that resume_all_vcpus, because I have
no idea.  But I tested it now, and "system_reset" will indeed move you
from "paused (internal-error)" to "paused" with RIP=0xfffffff0.

Paolo