Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini <pbonzini@redhat.com>]

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