Re: [Qemu-devel] pvpanic plans?

[Christian Borntraeger]

On 31/10/13 15:30, Michael S. Tsirkin wrote:
 On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
 Hi All,

 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?

 Thanks.
 
 
 I think the biggest issue is the new PANICKED state.

I thought the problem was that the new device broke windows and all
the following hazzle.

 Guests already have simple ways to halt the CPU,
 and actually do.  I think a new state was a mistake.
 So how about the following? Does it break anything?
 (Untested).

Please note that on s390 we also do the panic state (on a disabled wait)


target-s390x/kvm.c
...

monitor_protocol_event(QEVENT_GUEST_PANICKED, data);
qobject_decref(data);
vm_stop(RUN_STATE_GUEST_PANICKED);
...

Currently it is possible to restart libvirt, e.g. after an update and then it 
will
be able to fetch the full state of a guest via QMP. It will then also be able to
detect that this guest panicked some time ago.
I think one issue when removing the PANICKED state is that libvirt can then no 
longer detect that state, correct?

Christian


 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 
 diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
 index 226e298..2055afc 100644
 --- a/hw/misc/pvpanic.c
 +++ b/hw/misc/pvpanic.c
 @@ -51,7 +51,6 @@ static void handle_event(int event)
 
  if (event  PVPANIC_PANICKED) {
  panicked_mon_event(pause);
 -vm_stop(RUN_STATE_GUEST_PANICKED);
  return;
  }
  }
 
 

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
 Hi All,
 
 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?
 
 Thanks.


I think the biggest issue is the new PANICKED state.
Guests already have simple ways to halt the CPU,
and actually do.  I think a new state was a mistake.
So how about the following? Does it break anything?
(Untested).

Signed-off-by: Michael S. Tsirkin m...@redhat.com

diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
index 226e298..2055afc 100644
--- a/hw/misc/pvpanic.c
+++ b/hw/misc/pvpanic.c
@@ -51,7 +51,6 @@ static void handle_event(int event)
 
 if (event  PVPANIC_PANICKED) {
 panicked_mon_event(pause);
-vm_stop(RUN_STATE_GUEST_PANICKED);
 return;
 }
 }

Re: [Qemu-devel] pvpanic plans?

[Eric Blake]

On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
 On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
 Hi All,

 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?

 Thanks.
 
 
 I think the biggest issue is the new PANICKED state.
 Guests already have simple ways to halt the CPU,
 and actually do.  I think a new state was a mistake.
 So how about the following? Does it break anything?
 (Untested).
 
 Signed-off-by: Michael S. Tsirkin m...@redhat.com
 
 diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
 index 226e298..2055afc 100644
 --- a/hw/misc/pvpanic.c
 +++ b/hw/misc/pvpanic.c
 @@ -51,7 +51,6 @@ static void handle_event(int event)
  
  if (event  PVPANIC_PANICKED) {
  panicked_mon_event(pause);
 -vm_stop(RUN_STATE_GUEST_PANICKED);

Don't you still need to halt the guest on a panic event, for management
to have a chance to choose what to do about the panic?  I'm suspecting
this patch does break things.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

On Thu, Oct 31, 2013 at 3:32 PM, Eric Blake ebl...@redhat.com wrote:
 On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
 On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
 Hi All,

 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?

 Thanks.


 I think the biggest issue is the new PANICKED state.
 Guests already have simple ways to halt the CPU,
 and actually do.  I think a new state was a mistake.
 So how about the following? Does it break anything?
 (Untested).

 Signed-off-by: Michael S. Tsirkin m...@redhat.com

 diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
 index 226e298..2055afc 100644
 --- a/hw/misc/pvpanic.c
 +++ b/hw/misc/pvpanic.c
 @@ -51,7 +51,6 @@ static void handle_event(int event)

  if (event  PVPANIC_PANICKED) {
  panicked_mon_event(pause);
 -vm_stop(RUN_STATE_GUEST_PANICKED);

 Don't you still need to halt the guest on a panic event, for management
 to have a chance to choose what to do about the panic?  I'm suspecting
 this patch does break things.

I would be happy to apply a patch that just reverted the whole dang
mess of this device.

Regards,

Anthony Liguori

 --
 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

Il 31/10/2013 15:32, Eric Blake ha scritto:
 On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
 On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
 Hi All,
 
 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?
 
 I think the biggest issue is the new PANICKED state. Guests 
 already have simple ways to halt the CPU, and actually do.  I 
 think a new state was a mistake. So how about the following?
 Does it break anything? (Untested).
 
 Don't you still need to halt the guest on a panic event, for 
 management to have a chance to choose what to do about the panic? 
 I'm suspecting this patch does break things.

Yes, it does.  But I think that, once we make the pvpanic device is
optional, to a large extent there is no bug.  Adding the pvpanic
device to the VM will make libvirt obey oncrash instead of the
in-guest setting, and that's it.

Two months have passed and no casualties have been reported due to
pvpanic.  Let's just remove the auto-pvpanic from all machine types in
1.7 (yes, that's backwards incompatible in a strict sense), document
it in the release notes, and hope that the old QEMU versions with
mandatory pvpanic die of old age.

All the advantages/disadvantages from my original messages still
apply.  Let's ignore the disadvantages and just KISS.

Paolo
-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.22 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQIcBAEBAgAGBQJScmuVAAoJEBvWZb6bTYbyS6MP/Rb9japkUmjKy+bUW2Bf5vHD
hOrCL/20LC17OdIdzOngjfcY6OyPrtZ6dtjzvSjsWoxjW+QNuTwLM5h+0kOmvlM/
UCqX9MZiKFysVphnwIFy2fzTKmAVw5WqUlS17XMsiKxpvCquy5Fss5PWB3k3orMD
kRCoTTmQDID95T38OqezvzDaRjNvoHXOTuWFOhbzWt2OFUwg1WDy1GLKV/pSx0/o
GS2/RYo5iQkEtZk0muj0fmWEZX1K2nfaYmVQh39LJhFrWQLabHskYz1+n/OTmwQT
3DiUkfeqrxLahlyHbIOguLjLY8gH2fZa0gzHEB7D0b1aiuqQmsxM3ELdl8pJJzex
1ZGKt9X1u58v/SvfDHW14uCMAiv8jgQDnDOa6pgi7DezBQym+90+RnzC2Z5BcHIp
hsPEB0Bc47REPu69GOSj7XQ1uan5yQS38jSv8D11nJEW8VfXiV4smOZhivrEOicR
mdYYV6BNc985vcVOmvmkTJ5VkUOKeMzMDAJkSDqN6P0fQKTOJpCtJag3TcjHVRB4
ORXUrdO9NuICGjwB75T86INkxEsXFaw1aHIKpcxMk2PN6u/Zc+n7GXf65ReGorbL
2QUrepzUUt+mTYXJha9h7gEudRnixe5zK8AgqtSHAJPKP++LrFz4lrmzCHC6e3V1
6K2AhZl4EjBJaf6AMy70
=GBBt
-END PGP SIGNATURE-

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 08:32:43AM -0600, Eric Blake wrote:
 On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
  On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
  Hi All,
 
  I know it's been a long time since this thread. But qemu 1.7 is
  releasing, do you have any consensus on this?
 
  Thanks.
  
  
  I think the biggest issue is the new PANICKED state.
  Guests already have simple ways to halt the CPU,
  and actually do.  I think a new state was a mistake.
  So how about the following? Does it break anything?
  (Untested).
  
  Signed-off-by: Michael S. Tsirkin m...@redhat.com
  
  diff --git a/hw/misc/pvpanic.c b/hw/misc/pvpanic.c
  index 226e298..2055afc 100644
  --- a/hw/misc/pvpanic.c
  +++ b/hw/misc/pvpanic.c
  @@ -51,7 +51,6 @@ static void handle_event(int event)
   
   if (event  PVPANIC_PANICKED) {
   panicked_mon_event(pause);
  -vm_stop(RUN_STATE_GUEST_PANICKED);
 
 Don't you still need to halt the guest on a panic event, for management
 to have a chance to choose what to do about the panic?

Guest can just call hlt to do this. Most guests do this on a panic
already.

 I'm suspecting
 this patch does break things.

http://xkcd.com/1172/

 -- 
 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org
 

Re: [Qemu-devel] pvpanic plans?

[Eric Blake]

On 10/31/2013 08:47 AM, Michael S. Tsirkin wrote:

  if (event  PVPANIC_PANICKED) {
  panicked_mon_event(pause);
 -vm_stop(RUN_STATE_GUEST_PANICKED);

 Don't you still need to halt the guest on a panic event, for management
 to have a chance to choose what to do about the panic?
 
 Guest can just call hlt to do this. Most guests do this on a panic
 already.

On the one hand, the fact that the guest already has to inform the host
means we are already trusting the guest behavior on a panic.  On the
other hand, assuming that the guest will ALWAYS halt after triggering a
panic is putting a lot more trust in the guest, compared to qemu
explicitly halting the guest so that management has a chance to choose
to dump the guest's state at the moment the panic was flagged.

The biggest argument for either removing all auto-pvpanic, or reverting
pvpanic altogether, is that no one seems to be actively using pvpanic in
the field yet.  I wish we could get more feedback from Fujitsu as the
original patch authors on what they are looking for in a working
solution, rather than repeatedly second-guessing everything downstream
and delaying the eradication of the buggy behavior even longer.

-- 
Eric Blake   eblake redhat com+1-919-301-3266
Libvirt virtualization library http://libvirt.org



signature.asc
Description: OpenPGP digital signature

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 03:39:17PM +0100, Paolo Bonzini wrote:
 -BEGIN PGP SIGNED MESSAGE-
 Hash: SHA1
 
 Il 31/10/2013 15:32, Eric Blake ha scritto:
  On 10/31/2013 08:30 AM, Michael S. Tsirkin wrote:
  On Thu, Oct 24, 2013 at 10:39:03AM +0800, Hu Tao wrote:
  Hi All,
  
  I know it's been a long time since this thread. But qemu 1.7 is
  releasing, do you have any consensus on this?
  
  I think the biggest issue is the new PANICKED state. Guests 
  already have simple ways to halt the CPU, and actually do.  I 
  think a new state was a mistake. So how about the following?
  Does it break anything? (Untested).
  
  Don't you still need to halt the guest on a panic event, for 
  management to have a chance to choose what to do about the panic? 
  I'm suspecting this patch does break things.
 
 Yes, it does.

What does it break exactly?

 But I think that, once we make the pvpanic device is
 optional, to a large extent there is no bug.  Adding the pvpanic
 device to the VM will make libvirt obey oncrash instead of the
 in-guest setting, and that's it.
 
 Two months have passed and no casualties have been reported due to
 pvpanic.  Let's just remove the auto-pvpanic from all machine types in
 1.7 (yes, that's backwards incompatible in a strict sense), document
 it in the release notes, and hope that the old QEMU versions with
 mandatory pvpanic die of old age.

Nod. I'm fine with that.

 All the advantages/disadvantages from my original messages still
 apply.  Let's ignore the disadvantages and just KISS.
 
 Paolo

I think we still need to do get rid of the PANICKED state somehow.
If we can't replace it with RUNNING state, let's replace it with PAUSED.

For example, you can't continue from panicked for some reason.
You can't do a reset.
But you can pause and then continue.


 -BEGIN PGP SIGNATURE-
 Version: GnuPG v2.0.22 (GNU/Linux)
 Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/
 
 iQIcBAEBAgAGBQJScmuVAAoJEBvWZb6bTYbyS6MP/Rb9japkUmjKy+bUW2Bf5vHD
 hOrCL/20LC17OdIdzOngjfcY6OyPrtZ6dtjzvSjsWoxjW+QNuTwLM5h+0kOmvlM/
 UCqX9MZiKFysVphnwIFy2fzTKmAVw5WqUlS17XMsiKxpvCquy5Fss5PWB3k3orMD
 kRCoTTmQDID95T38OqezvzDaRjNvoHXOTuWFOhbzWt2OFUwg1WDy1GLKV/pSx0/o
 GS2/RYo5iQkEtZk0muj0fmWEZX1K2nfaYmVQh39LJhFrWQLabHskYz1+n/OTmwQT
 3DiUkfeqrxLahlyHbIOguLjLY8gH2fZa0gzHEB7D0b1aiuqQmsxM3ELdl8pJJzex
 1ZGKt9X1u58v/SvfDHW14uCMAiv8jgQDnDOa6pgi7DezBQym+90+RnzC2Z5BcHIp
 hsPEB0Bc47REPu69GOSj7XQ1uan5yQS38jSv8D11nJEW8VfXiV4smOZhivrEOicR
 mdYYV6BNc985vcVOmvmkTJ5VkUOKeMzMDAJkSDqN6P0fQKTOJpCtJag3TcjHVRB4
 ORXUrdO9NuICGjwB75T86INkxEsXFaw1aHIKpcxMk2PN6u/Zc+n7GXf65ReGorbL
 2QUrepzUUt+mTYXJha9h7gEudRnixe5zK8AgqtSHAJPKP++LrFz4lrmzCHC6e3V1
 6K2AhZl4EjBJaf6AMy70
 =GBBt
 -END PGP SIGNATURE-

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
  Yes, it does.
 What does it break exactly?

The point of a panicked event is to examine the guest at a particular
moment in time (e.g. host-initiated crash dump).  If you let the guest
run, it may reboot and prevent you from getting a meaningful dump.

  But I think that, once we make the pvpanic device is
  optional, to a large extent there is no bug.  Adding the pvpanic
  device to the VM will make libvirt obey oncrash instead of the
  in-guest setting, and that's it.
  
  Two months have passed and no casualties have been reported due to
  pvpanic.  Let's just remove the auto-pvpanic from all machine types in
  1.7 (yes, that's backwards incompatible in a strict sense), document
  it in the release notes, and hope that the old QEMU versions with
  mandatory pvpanic die of old age.
 
 Nod. I'm fine with that.
 
 I think we still need to do get rid of the PANICKED state somehow.
 If we can't replace it with RUNNING state, let's replace it with PAUSED.
 
 For example, you can't continue from panicked for some reason.
 You can't do a reset.  But you can pause and then continue.

We need to keep the PANICKED state, but we can make it a normal
resumable state.

Basically it's patches 1 and 2 at
http://permalink.gmane.org/gmane.comp.emulators.qemu/229131.  Rebasing
will fix the problem highlighted in the commit message of patch 2.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
   Yes, it does.
  What does it break exactly?
 
 The point of a panicked event is to examine the guest at a particular
 moment in time (e.g. host-initiated crash dump).  If you let the guest
 run, it may reboot and prevent you from getting a meaningful dump.

Well we trust guest anyway, so I think we can trust it to call halt.


   But I think that, once we make the pvpanic device is
   optional, to a large extent there is no bug.  Adding the pvpanic
   device to the VM will make libvirt obey oncrash instead of the
   in-guest setting, and that's it.
   
   Two months have passed and no casualties have been reported due to
   pvpanic.  Let's just remove the auto-pvpanic from all machine types in
   1.7 (yes, that's backwards incompatible in a strict sense), document
   it in the release notes, and hope that the old QEMU versions with
   mandatory pvpanic die of old age.
  
  Nod. I'm fine with that.
  
  I think we still need to do get rid of the PANICKED state somehow.
  If we can't replace it with RUNNING state, let's replace it with PAUSED.
  
  For example, you can't continue from panicked for some reason.
  You can't do a reset.  But you can pause and then continue.
 
 We need to keep the PANICKED state, but we can make it a normal
 resumable state.

If it's resumable how is it different from PAUSED?

 Basically it's patches 1 and 2 at
 http://permalink.gmane.org/gmane.comp.emulators.qemu/229131.  Rebasing
 will fix the problem highlighted in the commit message of patch 2.
 
 Paolo

Looks like all transitions from paused state should be allowed from panicked
state. So why keep it separate?

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 08:49:08AM -0600, Eric Blake wrote:
 On 10/31/2013 08:47 AM, Michael S. Tsirkin wrote:
 
   if (event  PVPANIC_PANICKED) {
   panicked_mon_event(pause);
  -vm_stop(RUN_STATE_GUEST_PANICKED);
 
  Don't you still need to halt the guest on a panic event, for management
  to have a chance to choose what to do about the panic?
  
  Guest can just call hlt to do this. Most guests do this on a panic
  already.
 
 On the one hand, the fact that the guest already has to inform the host
 means we are already trusting the guest behavior on a panic.  On the
 other hand, assuming that the guest will ALWAYS halt after triggering a
 panic is putting a lot more trust in the guest, compared to qemu
 explicitly halting the guest so that management has a chance to choose
 to dump the guest's state at the moment the panic was flagged.

I wouldn't call it *a lot* more trust. And again, this is guest policy:
if you want to do hlt from driver because you think it's safer, go for it.

 The biggest argument for either removing all auto-pvpanic, or reverting
 pvpanic altogether, is that no one seems to be actively using pvpanic in
 the field yet.  I wish we could get more feedback from Fujitsu as the
 original patch authors on what they are looking for in a working
 solution, rather than repeatedly second-guessing everything downstream
 and delaying the eradication of the buggy behavior even longer.


With my patch we have a benign device that merely reports io writes
on the monitor. No code - no bugs.


 Eric Blake   eblake redhat com+1-919-301-3266
 Libvirt virtualization library http://libvirt.org
 

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
 On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
 Yes, it does.
 What does it break exactly?

 The point of a panicked event is to examine the guest at a particular
 moment in time (e.g. host-initiated crash dump).  If you let the guest
 run, it may reboot and prevent you from getting a meaningful dump.
 
 Well we trust guest anyway, so I think we can trust it to call halt.

No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
configuration.

 But I think that, once we make the pvpanic device is
 optional, to a large extent there is no bug.  Adding the pvpanic
 device to the VM will make libvirt obey oncrash instead of the
 in-guest setting, and that's it.

 Two months have passed and no casualties have been reported due to
 pvpanic.  Let's just remove the auto-pvpanic from all machine types in
 1.7 (yes, that's backwards incompatible in a strict sense), document
 it in the release notes, and hope that the old QEMU versions with
 mandatory pvpanic die of old age.

 Nod. I'm fine with that.

 I think we still need to do get rid of the PANICKED state somehow.
 If we can't replace it with RUNNING state, let's replace it with PAUSED.

 For example, you can't continue from panicked for some reason.
 You can't do a reset.  But you can pause and then continue.

 We need to keep the PANICKED state, but we can make it a normal
 resumable state.
 
 If it's resumable how is it different from PAUSED?

If the guest panics while for some reason libvirtd went down, libvirt
can see what happened.  It is similar to the I/O error state in this
respect.

 Looks like all transitions from paused state should be allowed from panicked
 state. So why keep it separate?

Because you can poll for the state instead of watching an event.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 04:26:13PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
  On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
  Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
  Yes, it does.
  What does it break exactly?
 
  The point of a panicked event is to examine the guest at a particular
  moment in time (e.g. host-initiated crash dump).  If you let the guest
  run, it may reboot and prevent you from getting a meaningful dump.
  
  Well we trust guest anyway, so I think we can trust it to call halt.
 
 No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
 configuration.

  But I think that, once we make the pvpanic device is
  optional, to a large extent there is no bug.  Adding the pvpanic
  device to the VM will make libvirt obey oncrash instead of the
  in-guest setting, and that's it.
 
  Two months have passed and no casualties have been reported due to
  pvpanic.  Let's just remove the auto-pvpanic from all machine types in
  1.7 (yes, that's backwards incompatible in a strict sense), document
  it in the release notes, and hope that the old QEMU versions with
  mandatory pvpanic die of old age.
 
  Nod. I'm fine with that.
 
  I think we still need to do get rid of the PANICKED state somehow.
  If we can't replace it with RUNNING state, let's replace it with PAUSED.
 
  For example, you can't continue from panicked for some reason.
  You can't do a reset.  But you can pause and then continue.
 
  We need to keep the PANICKED state, but we can make it a normal
  resumable state.
  
  If it's resumable how is it different from PAUSED?
 
 If the guest panics while for some reason libvirtd went down, libvirt
 can see what happened.  It is similar to the I/O error state in this
 respect.
 
  Looks like all transitions from paused state should be allowed from panicked
  state. So why keep it separate?
 
 Because you can poll for the state instead of watching an event.
 
 Paolo

I see. Maybe it was a mistake to use a separate runtime state for
this, but oh well.

So it should be exactly like paused, we can just find all transitions
from PAUSED and it should be same for PANICKED?
Why isn't DEBUG allowed from PAUSED but allowed from PANICKED then?
Maybe it should be allowed for PAUSED?

-- 
MST

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 04:56:07PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 16:45, Michael S. Tsirkin ha scritto:
  On Thu, Oct 31, 2013 at 04:26:13PM +0100, Paolo Bonzini wrote:
  Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
  On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
  Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
  Yes, it does.
  What does it break exactly?
 
  The point of a panicked event is to examine the guest at a particular
  moment in time (e.g. host-initiated crash dump).  If you let the guest
  run, it may reboot and prevent you from getting a meaningful dump.
 
  Well we trust guest anyway, so I think we can trust it to call halt.
 
  No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
  configuration.
 
  But I think that, once we make the pvpanic device is
  optional, to a large extent there is no bug.  Adding the pvpanic
  device to the VM will make libvirt obey oncrash instead of the
  in-guest setting, and that's it.
 
  Two months have passed and no casualties have been reported due to
  pvpanic.  Let's just remove the auto-pvpanic from all machine types in
  1.7 (yes, that's backwards incompatible in a strict sense), document
  it in the release notes, and hope that the old QEMU versions with
  mandatory pvpanic die of old age.
 
  Nod. I'm fine with that.
 
  I think we still need to do get rid of the PANICKED state somehow.
  If we can't replace it with RUNNING state, let's replace it with PAUSED.
 
  For example, you can't continue from panicked for some reason.
  You can't do a reset.  But you can pause and then continue.
 
  We need to keep the PANICKED state, but we can make it a normal
  resumable state.
 
  If it's resumable how is it different from PAUSED?
 
  If the guest panics while for some reason libvirtd went down, libvirt
  can see what happened.  It is similar to the I/O error state in this
  respect.
 
  Looks like all transitions from paused state should be allowed from 
  panicked
  state. So why keep it separate?
 
  Because you can poll for the state instead of watching an event.
  
  I see. Maybe it was a mistake to use a separate runtime state for
  this, but oh well.
 
 Yes, we should have had a list of reasons why a guest is stopped (I/O
 error, panic, gdb, ...) and a command to clear one or more of them;
 there can be paused/running/waiting-for-migration/... states, but many
 of the states we have are not necessarily in mutual exclusion.
 
 But we cannot really choose now.
 
  So it should be exactly like paused, we can just find all transitions
  from PAUSED and it should be same for PANICKED?
  Why isn't DEBUG allowed from PAUSED but allowed from PANICKED then?
  Maybe it should be allowed for PAUSED?
 
 PANICKED-DEBUG was added by commit bc7d0e667.  That commit can be
 reverted if the panicked state is removed from runstate_needs_reset.
 
 Paolo

Okay so let's drop the code duplication and explicitly make
them the same?

Signed-off-by: Michael S. Tsirkin m...@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;
+}
 }
 }
 
@@ -686,8 +688,7 @@ int runstate_is_running(void)
 bool runstate_needs_reset(void)
 {
 return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
-runstate_check(RUN_STATE_SHUTDOWN) ||
-runstate_check(RUN_STATE_GUEST_PANICKED);
+runstate_check(RUN_STATE_SHUTDOWN);
 }
 
 StatusInfo *qmp_query_status(Error **errp)

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

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 m...@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

 +}
  }
  }
  
 @@ -686,8 +688,7 @@ int runstate_is_running(void)
  bool runstate_needs_reset(void)
  {
  return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
 -runstate_check(RUN_STATE_SHUTDOWN) ||
 -runstate_check(RUN_STATE_GUEST_PANICKED);
 +runstate_check(RUN_STATE_SHUTDOWN);
  }
  
  StatusInfo *qmp_query_status(Error **errp)
 

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

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 m...@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.

  +}
   }
   }
   
  @@ -686,8 +688,7 @@ int runstate_is_running(void)
   bool runstate_needs_reset(void)
   {
   return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
  -runstate_check(RUN_STATE_SHUTDOWN) ||
  -runstate_check(RUN_STATE_GUEST_PANICKED);
  +runstate_check(RUN_STATE_SHUTDOWN);
   }
   
   StatusInfo *qmp_query_status(Error **errp)
  

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

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 m...@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,

Yea, let's do that.

 and perhaps there are others I'm missing.
  Just add a comment
 before runstate_transitions_def's entries for PANICKED, IO_ERROR and
 WATCHDOG.

comments don't compile :)

 But again, it is somewhat separate from the issue at hand, which is to
 finally make pvpanic usable and hopefully before 1.7.
 
 Paolo
 
  +}
   }
   }
   
  @@ -686,8 +688,7 @@ int runstate_is_running(void)
   bool runstate_needs_reset(void)
   {
   return runstate_check(RUN_STATE_INTERNAL_ERROR) ||
  -runstate_check(RUN_STATE_SHUTDOWN) ||
  -runstate_check(RUN_STATE_GUEST_PANICKED);
  +runstate_check(RUN_STATE_SHUTDOWN);
   }
   
   StatusInfo *qmp_query_status(Error **errp)
  

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

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 m...@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

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

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 m...@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?

diff --git a/vl.c b/vl.c
index 46c29c4..4388c95 100644
--- a/vl.c
+++ b/vl.c
@@ -593,12 +593,6 @@ static const RunStateTransition runstate_transitions_def[] 
= {
 { RUN_STATE_INMIGRATE, RUN_STATE_RUNNING },
 { RUN_STATE_INMIGRATE, RUN_STATE_PAUSED },
 
-{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_PAUSED },
-{ RUN_STATE_INTERNAL_ERROR, RUN_STATE_FINISH_MIGRATE },
-
-{ RUN_STATE_IO_ERROR, RUN_STATE_RUNNING },
-{ RUN_STATE_IO_ERROR, RUN_STATE_FINISH_MIGRATE },
-
 { RUN_STATE_PAUSED, RUN_STATE_RUNNING },
 { RUN_STATE_PAUSED, RUN_STATE_FINISH_MIGRATE },
 
@@ -635,16 +629,17 @@ static const RunStateTransition 
runstate_transitions_def[] = {
 { RUN_STATE_SUSPENDED, RUN_STATE_RUNNING },
 { RUN_STATE_SUSPENDED, RUN_STATE_FINISH_MIGRATE },
 
-{ 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 },
 };
 
+static const RunState runstate_paused[] = {
+{ RUN_STATE_GUEST_PANICKED},
+{ RUN_STATE_IO_ERROR},
+{ RUN_STATE_INTERNAL_ERROR},
+{ RUN_STATE_WATCHDOG},
+{ RUN_STATE_MAX },
+};
+
 static bool runstate_valid_transitions[RUN_STATE_MAX][RUN_STATE_MAX];
 
 bool runstate_check(RunState state)
@@ -655,12 +650,21 @@ bool runstate_check(RunState state)
 static void runstate_init(void)
 {
 const RunStateTransition *p;
+const RunState *i;
 
 memset(runstate_valid_transitions, 0, sizeof(runstate_valid_transitions));
 
 for (p = runstate_transitions_def[0]; p-from != RUN_STATE_MAX; p++) {
 runstate_valid_transitions[p-from][p-to] = true;
 }
+/* Allow two-way transitions between identical states */
+for (i = runstate_paused[0]; *p != RUN_STATE_MAX; p++) {
+runstate_valid_transitions[*i][RUN_STATE_PAUSED] = true;
+runstate_valid_transitions[RUN_STATE_PAUSED][*i] = true;
+memcpy(runstate_valid_transitions[*i],
+   runstate_valid_transitions[RUN_STATE_PAUSED],
+

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

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

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

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 m...@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 },
 

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

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 16:45, Michael S. Tsirkin ha scritto:
 On Thu, Oct 31, 2013 at 04:26:13PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 16:09, Michael S. Tsirkin ha scritto:
 On Thu, Oct 31, 2013 at 03:56:42PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 15:52, Michael S. Tsirkin ha scritto:
 Yes, it does.
 What does it break exactly?

 The point of a panicked event is to examine the guest at a particular
 moment in time (e.g. host-initiated crash dump).  If you let the guest
 run, it may reboot and prevent you from getting a meaningful dump.

 Well we trust guest anyway, so I think we can trust it to call halt.

 No, we cannot.  Reboot-in-guest-after-dump-on-host is a perfectly fine
 configuration.

 But I think that, once we make the pvpanic device is
 optional, to a large extent there is no bug.  Adding the pvpanic
 device to the VM will make libvirt obey oncrash instead of the
 in-guest setting, and that's it.

 Two months have passed and no casualties have been reported due to
 pvpanic.  Let's just remove the auto-pvpanic from all machine types in
 1.7 (yes, that's backwards incompatible in a strict sense), document
 it in the release notes, and hope that the old QEMU versions with
 mandatory pvpanic die of old age.

 Nod. I'm fine with that.

 I think we still need to do get rid of the PANICKED state somehow.
 If we can't replace it with RUNNING state, let's replace it with PAUSED.

 For example, you can't continue from panicked for some reason.
 You can't do a reset.  But you can pause and then continue.

 We need to keep the PANICKED state, but we can make it a normal
 resumable state.

 If it's resumable how is it different from PAUSED?

 If the guest panics while for some reason libvirtd went down, libvirt
 can see what happened.  It is similar to the I/O error state in this
 respect.

 Looks like all transitions from paused state should be allowed from panicked
 state. So why keep it separate?

 Because you can poll for the state instead of watching an event.
 
 I see. Maybe it was a mistake to use a separate runtime state for
 this, but oh well.

Yes, we should have had a list of reasons why a guest is stopped (I/O
error, panic, gdb, ...) and a command to clear one or more of them;
there can be paused/running/waiting-for-migration/... states, but many
of the states we have are not necessarily in mutual exclusion.

But we cannot really choose now.

 So it should be exactly like paused, we can just find all transitions
 from PAUSED and it should be same for PANICKED?
 Why isn't DEBUG allowed from PAUSED but allowed from PANICKED then?
 Maybe it should be allowed for PAUSED?

PANICKED-DEBUG was added by commit bc7d0e667.  That commit can be
reverted if the panicked state is removed from runstate_needs_reset.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

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=0xfff0.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto:
  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?

Yes, and also modify gdbstub.c.  It's all in the URL I posted a few
hours ago.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Michael S. Tsirkin]

On Thu, Oct 31, 2013 at 06:10:36PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto:
   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?
 
 Yes, and also modify gdbstub.c.  It's all in the URL I posted a few
 hours ago.
 
 Paolo

OK, so can you pls post patches 1 and 2? I'll review and ack.

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 31/10/2013 18:18, Michael S. Tsirkin ha scritto:
 On Thu, Oct 31, 2013 at 06:10:36PM +0100, Paolo Bonzini wrote:
 Il 31/10/2013 18:01, Michael S. Tsirkin ha scritto:
 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?

 Yes, and also modify gdbstub.c.  It's all in the URL I posted a few
 hours ago.

 Paolo
 
 OK, so can you pls post patches 1 and 2? I'll review and ack.

Next Monday I will.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Markus Armbruster]

Ping!

Hu Tao hu...@cn.fujitsu.com writes:

 Hi All,

 I know it's been a long time since this thread. But qemu 1.7 is
 releasing, do you have any consensus on this?

 Thanks.

Re: [Qemu-devel] pvpanic plans?

[Hu Tao]

Hi All,

I know it's been a long time since this thread. But qemu 1.7 is
releasing, do you have any consensus on this?

Thanks.

Re: [Qemu-devel] pvpanic plans?

[Richard W.M. Jones]

On Thu, Aug 22, 2013 at 03:09:06PM -0500, Anthony Liguori wrote:
 Paolo Bonzini pbonz...@redhat.com writes:
  Also, a virtio watchdog device makes little sense, IMHO.  PV makes sense
  if emulation has insufficient performance, excessive CPU usage, or
  excessive complexity.  We already have both an ISA and a PCI watchdog,
  and they serve their purpose wonderfully.
 
 Neither of which actually work with modern versions of Windows FWIW.

Correct, although someone could write a driver!

 Plus emulated watchdogs do not take into account steal time or
 overcommit in general.  I've seen multiple cases where a naive watchdog
 has a problem in the field when the system is under heavy load.

The watchdog devices in qemu run on guest time.  However the watchdog
*daemon* inside the guest probably does behave badly as you describe.
Changing the device model isn't going to help this, but it would
definitely make sense to fix the daemon (although I don't know how --
is steal time exposed to guests?)

I don't necessarily think a virtio-watchdog is a bad idea.  For one
thing it'd mean we would have a watchdog device that works on ARM.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] pvpanic plans?

[Richard W.M. Jones]

On Thu, Aug 22, 2013 at 01:25:32PM -0500, Anthony Liguori wrote:
 I believe that the watchdogs we emulate today are not supported by a
 majority of guests.

BTW this is not true.  The two watchdog devices are supported
by all Linux guests.

Windows guests do not support them, but Windows lacks[1] any sort of
watchdog framework so lack of device support is the least of your
problems.  There would be nothing for the device to plug into (unlike
the /dev/watchdog API on Linux), nor is there any daemon to support it
(unlike the 15 year old watchdog daemon on Linux).

Rich.

[1] Yes, this exists:
http://msdn.microsoft.com/en-us/library/ms856963.aspx
but it requires a special version of Windows.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
virt-top is 'top' for virtual machines.  Tiny program with many
powerful monitoring features, net stats, disk stats, logging, etc.
http://people.redhat.com/~rjones/virt-top

Re: [Qemu-devel] pvpanic plans?

[Ronen Hod]

On 08/27/2013 11:06 AM, Richard W.M. Jones wrote:

On Thu, Aug 22, 2013 at 03:09:06PM -0500, Anthony Liguori wrote:

Paolo Bonzini pbonz...@redhat.com writes:

Also, a virtio watchdog device makes little sense, IMHO.  PV makes sense
if emulation has insufficient performance, excessive CPU usage, or
excessive complexity.  We already have both an ISA and a PCI watchdog,
and they serve their purpose wonderfully.

Neither of which actually work with modern versions of Windows FWIW.

Correct, although someone could write a driver!


Plus emulated watchdogs do not take into account steal time or
overcommit in general.  I've seen multiple cases where a naive watchdog
has a problem in the field when the system is under heavy load.

The watchdog devices in qemu run on guest time.  However the watchdog
*daemon* inside the guest probably does behave badly as you describe.
Changing the device model isn't going to help this, but it would
definitely make sense to fix the daemon (although I don't know how --
is steal time exposed to guests?)

I don't necessarily think a virtio-watchdog is a bad idea.  For one
thing it'd mean we would have a watchdog device that works on ARM.

Rich.


I believe that a watchdog is not the way to go. You need host-side decision 
making.
Say that the guest did not receive CPU/Disk/network resources for a lengthy 
period
of time, but the host knows that this is due to host resources availability. In 
such cases,
you certainly do not want to reboot all the guests, especially since rebooting 
50
Windows VMs could be a nightmare.
BTW, Windows guest disable some of their watchdogs when they detect the presence
of Hyper-V, we use it to overcome BSODs!
So the right solution is to send a heart-beat to a management application 
(using qemu-ga
or whatever), and let it decide how to handle it.

Ronen.

Re: [Qemu-devel] pvpanic plans?

[Richard W.M. Jones]

On Tue, Aug 27, 2013 at 04:08:12PM +0300, Ronen Hod wrote:
 So the right solution is to send a heart-beat to a management
 application (using qemu-ga or whatever), and let it decide how to
 handle it.

Agreed.  The qemu watchdog lets you do this already.  You can (using
the qemu monitor, or libvirt) capture watchdog events and put them
into your management application.  Watchdog firing does *not*
necessarily mean a guest reboot.

[Note what I say applies to the qemu watchdog device.  The Linux
watchdog daemon may independently initiate a guest reboot, but you can
configure it to perform other actions instead.]

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

On Tue, Aug 27, 2013 at 8:20 AM, Richard W.M. Jones rjo...@redhat.com wrote:
 On Tue, Aug 27, 2013 at 04:08:12PM +0300, Ronen Hod wrote:
 So the right solution is to send a heart-beat to a management
 application (using qemu-ga or whatever), and let it decide how to
 handle it.

This is host-centric solution and assumes that a management tool is
making all of the decisions.  This doesn't work in an IaaS environment
where these sort of policy decisions need to be driven from the guest.

Furthermore, you really want the watchdog daemon to run with real time
priority which implies a heightened privilege level.  This rules out
using qemu-ga for that purpose.

 Agreed.  The qemu watchdog lets you do this already.  You can (using
 the qemu monitor, or libvirt) capture watchdog events and put them
 into your management application.  Watchdog firing does *not*
 necessarily mean a guest reboot.

Ack, but the current watchdog does not work for Windows guests and is
not aware of guest time.

That's why I think having a virtio-ilo makes sense.  This is not a
solved problem today.

Regards,

Anthony Liguori

 [Note what I say applies to the qemu watchdog device.  The Linux
 watchdog daemon may independently initiate a guest reboot, but you can
 configure it to perform other actions instead.]

 Rich.

 --
 Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
 Fedora Windows cross-compiler. Compile Windows programs, test, and
 build Windows installers. Over 100 libraries supported.
 http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] pvpanic plans?

[Richard W.M. Jones]

On Tue, Aug 27, 2013 at 08:26:53AM -0500, Anthony Liguori wrote:
 That's why I think having a virtio-ilo makes sense.  This is not a
 solved problem today.

What's the scope of virtio-ilo?  If it's anything like a real ILO it's
going to do a lot of not-very-related things, such as:

 - pvpanic-type function
 - watchdog-type function
 - remote console / serial ports
 - remote CD-ROM
 - remote power switch

qemu already does nearly all of this ...

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Fedora Windows cross-compiler. Compile Windows programs, test, and
build Windows installers. Over 100 libraries supported.
http://fedoraproject.org/wiki/MinGW

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 22/08/2013 22:39, Anthony Liguori ha scritto:
 On Thu, Aug 22, 2013 at 3:36 PM, Laszlo Ersek ler...@redhat.com wrote:
 On 08/22/13 22:09, Anthony Liguori wrote:

 The difference is that ACPI or platform devices in general are
 unexpected to be added.  By definition it means that the motherboard has
 most likely been changed.

 You could encounter a new ACPI artifact after simply re-flashing your MB
 with an updated BIOS, without opening the chassis. If windows can't
 deal with that, their loss! :)
 
 I'm pretty sure does Windows boot up okay is on every major vendor's
 firmware test plan for shipping new updates...

For a firmware vendor it is perfectly okay to ship and require new
drivers for functionality introduced by a firmware update...

Paolo

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

Paolo Bonzini pbonz...@redhat.com writes:

 The thread from yesterday has died off (perhaps also because of
 my inappropriate answer to Michael, for which I apologize to him
 and everyone).  I took some time to discuss the libvirt requirements
 further with Daniel Berrange and Eric Blake on IRC.  If anyone is
 interested, I can give logs.  This is a suggestion for how to
 proceed in both QEMU and libvirt.


 == Builtin pvpanic ==

 QEMU will remove pvpanic from pc-1.5 in 1.6.1 and 1.5.4.  This does not
 break migration.

pvpanic has been a failure.  It's a poorly designed device with even
worse semantics.  I applied it and I'll take the fault for merging it in
the first place.

We should simply scrap it and start over.  It has so few users at this
point that this is still a realistic option.  Using something based on
ISA that requires specific ACPI entries was a mistake.

We should just introduce a simple watchdog device based on virtio and
call it a day.  Then it's cross platform, solves the guest enumeration
problem, and libvirt can detect the presence of the new device.

None of the plans outlined below give us a proper solution.  I think
removing is our best option at this point.

Regards,

Anthony Liguori



 == Support in libvirt for current functionality ==

 libvirt will add a panic-notifier/ element, and possibly a capability
 for it accessible via virsh capabilities.  There are two possibilities:

 1) On QEMU 1.5.4/1.6.1 and newer (and on QEMU 1.6.0 with a machine type
other than pc-1.5), on_crash will only work if the element is there.
On QEMU 1.5.0-1.5.3, and on QEMU 1.6.0 with the pc-1.5 machine type,
on_crash will be obeyed always, and may override e.g. reboot-on-panic
if a guest driver exist.

 2) On all versions, on_crash will only work if the element is there.


 In turn, there are two ways to implement (2):

 2a) libvirt will always add -global pvpanic.iobase=0 to neutralize
 the builtin pvpanic device if present.  panic-notifier/
 will create the device with -device pvpanic,iobase=0x505

 Advantage: no changes to QEMU

 Disadvantage 1: writes to port 0 with QEMU 1.{5.0,5.1,5.2,5.3,6.0}
   and pc-1.5 machine type will write to a pvpanic device instead of
   the DMA controller.  Probably harmless, and limited to some QEMU
   versions.

 Disadvantage 2: libvirt has knowledge of the pvpanic port number

 2b) QEMU will provide a way for libvirt to detect that no machine type
 has the builtin pvpanic.  If some machine type may have the builtin
 pvpanic, and panic-notifier/ is absent, libvirt will add
 -global pvpanic.iobase=0 to neutralize it.  Otherwise, libvirt
 will create the device normally.

 A possible way for libvirt to detect good machine types is a
 dummy property.  This is a bit ugly in that the property would not
 affect the behavior of the device.  The property would remain in
 the long term.

 Another possibility is for QEMU to rename the device, e.g. to
 isa-pvpanic.  This is also somewhat gross, but not visible in the
 long term when the pvpanic name will be lost in history.

 Advantage 1: libvirt has no knowledge of the pvpanic port number

 Disadvantage 1: same as above

 Disadvantage 2: need a somewhat gross change in QEMU


 This method also provides an (also somewhat gross on the QEMU side)
 way to detect other changes in the pvpanic semantics.  One example
 mentioned below, is making the panicked state temporary.

 == Possible improvements to pvpanic ==

 The current implementation of pvpanic supports three modes: reset system
 on panic, destroy domain on panic, preserve domain with no possibility
 to resume it.  (Optionally a domain can be dumped too).

 Long term, the choice to include pvpanic should not be on the guest
 admin's shoulders, but rather in libosinfo.  Thus, it would be nice to
 have a fourth mode where the panic is logged but the guest otherwise
 keeps running.  This mode would let libosinfo add pvpanic by default
 without affecting the guest's behavior on panic.

 With this change, on_crashignore/on_crash will behave as follows
 for the three possibilities above:

 (1)  With QEMU 1.5.0 to 1.6.1, on_crash will _not_ obey the setting,
  never (even if no panic-notifier/ is specified).

  libvirt will have to pick a fallback action.

advantage of destroy as fallback: it is the default (but
   note that restart is the default for virt-install)

advantage of preserve as fallback: lets the admin examine
   the panic

advantage of restart as fallback: maximum availability of
   the VM, it is the default for virt-install

 (2a) With QEMU 1.5.0 to 1.6.1, on_crash will _not_ obey the setting
  if panic-notifier/ is specified.  libvirt has _no way_ to learn
  about this, so the capability would always be present with these
  QEMU versions and libosinfo would always add panic-notifier/ 

Re: [Qemu-devel] pvpanic plans?

[Laszlo Ersek]

On 08/22/13 18:10, Paolo Bonzini wrote:
 The thread from yesterday has died off (perhaps also because of
 my inappropriate answer to Michael, for which I apologize to him
 and everyone).  I took some time to discuss the libvirt requirements
 further with Daniel Berrange and Eric Blake on IRC.  If anyone is
 interested, I can give logs.  This is a suggestion for how to
 proceed in both QEMU and libvirt.

The analysis is pretty overwhelming :)

I have read Anthony's response and I'm not trying to argue -- I'm just
spending a few thoughts on this and I'm willing to let them go to waste.

In general I think we should minimize the quirks the user (who edits the
libvirt XML) has to know about. Interactions between some XML elements,
without explicit inter-references (formulated as attributes, like
controller/index) are Bad (TM). So,

 == Builtin pvpanic ==
 
 QEMU will remove pvpanic from pc-1.5 in 1.6.1 and 1.5.4.  This does not
 break migration.
 
 
 == Support in libvirt for current functionality ==
 
 libvirt will add a panic-notifier/ element, and possibly a capability
 for it accessible via virsh capabilities.  There are two possibilities:
 
 1) On QEMU 1.5.4/1.6.1 and newer (and on QEMU 1.6.0 with a machine type
other than pc-1.5), on_crash will only work if the element is there.
On QEMU 1.5.0-1.5.3, and on QEMU 1.6.0 with the pc-1.5 machine type,
on_crash will be obeyed always, and may override e.g. reboot-on-panic
if a guest driver exist.

I don't like this because there's some interplay between on_crash and
panic_notifier, which even depends on the qemu version.


 
 2) On all versions, on_crash will only work if the element is there.

I like this, because, if on_crash doesn't work without panic_notifier
*at all*, then we can just drop panic_notifier, and make on_crash mean
(on_crash  panic_notifier) in the original sense.

IOW, drop panic_notifier, and make on_crash work *always*.

 
 
 In turn, there are two ways to implement (2):
 
 2a) libvirt will always add -global pvpanic.iobase=0 to neutralize
 the builtin pvpanic device if present.  panic-notifier/
 will create the device with -device pvpanic,iobase=0x505
 
 Advantage: no changes to QEMU
 
 Disadvantage 1: writes to port 0 with QEMU 1.{5.0,5.1,5.2,5.3,6.0}
   and pc-1.5 machine type will write to a pvpanic device instead of
   the DMA controller.  Probably harmless, and limited to some QEMU
   versions.
 
 Disadvantage 2: libvirt has knowledge of the pvpanic port number

Updating this paragraph with my above suggestion:

- (s/pvpanic.iobase/pvpanic.ioport/g)

- if on_crash is absent:
  - for 1.{5.0,5.1,5.2,5.3,6.0}, add -global pvpanic.ioport=0
  - for other versions, do nothing

- if on_crash is present:
  - for 1.{5.0,5.1,5.2,5.3,6.0}, do nothing,
  - for other versions, pass -device pvpanic
(knowledge of 0x505 is unneeded)

advantage and disadvantage 1 remain, disadvantage 2 is gone.


 2b) QEMU will provide a way for libvirt to detect that no machine type
 has the builtin pvpanic.  If some machine type may have the builtin
 pvpanic, and panic-notifier/ is absent, libvirt will add
 -global pvpanic.iobase=0 to neutralize it.  Otherwise, libvirt
 will create the device normally.
 
 A possible way for libvirt to detect good machine types is a
 dummy property.  This is a bit ugly in that the property would not
 affect the behavior of the device.  The property would remain in
 the long term.
 
 Another possibility is for QEMU to rename the device, e.g. to
 isa-pvpanic.  This is also somewhat gross, but not visible in the
 long term when the pvpanic name will be lost in history.
 
 Advantage 1: libvirt has no knowledge of the pvpanic port number
 
 Disadvantage 1: same as above
 
 Disadvantage 2: need a somewhat gross change in QEMU
 
 
 This method also provides an (also somewhat gross on the QEMU side)
 way to detect other changes in the pvpanic semantics.  One example
 mentioned below, is making the panicked state temporary.

Too much work in qemu, in order to introduce ugliness, to hide older
ugliness.

 == Possible improvements to pvpanic ==

That's too complex / far out for me now, sorry :)

Thanks,
Laszlo

Re: [Qemu-devel] pvpanic plans?

[Laszlo Ersek]

On 08/22/13 18:44, Anthony Liguori wrote:

 pvpanic has been a failure.  It's a poorly designed device with even
 worse semantics.

I disagree somewhat.

Requiring a separate ioport is not ideal, I admit. Configuration over
ACPI is good OTOH (it seems to put standards to good use anyway).

Noone realized pvpanic had poor technical design until the Windows new
device wizard popped up -- is that correct? Most of us are probably not
habitual Windows users, which is probably why we haven't thought of it
earlier.

Maybe we shouldn't promise there won't be guest-visible changes in ACPI
contents. If we do promise, maybe we should then make the SeaBIOS
binary that we're loading dependent on -M too too.

After all, had we managed to completely hide the \_SB.PCI0.ISA.PEVT
device programmatically, as opposed to only disabling it, we might have
never realized pvpanic had poor design. Which (almost) means it wouldn't
have had one.

If we selected a SeaBIOS binary based on -M, then we could hide this
stuff from Windows.


 I applied it and I'll take the fault for merging it in
 the first place.
 
 We should simply scrap it and start over.

That will kinda Eff some downstreams in the A...


 It has so few users at this
 point that this is still a realistic option.  Using something based on
 ISA that requires specific ACPI entries was a mistake.
 
 We should just introduce a simple watchdog device based on virtio and
 call it a day.  Then it's cross platform, solves the guest enumeration
 problem, and libvirt can detect the presence of the new device.

If the guest doesn't initialize the proposed virtio-panic device, then
it will lie dormant too, just like the current pvpanic device. That's good.

However a new (standalone) virtio device will take up yet another PCI
function (a full device if you want it to be hotpluggable). PCI
functions are scarcer than ioports.

It will need documentation in the virtio-spec as well.

We'd need an arbitrarily heavily multiplexed paravirt channel between
guest and qemu. Maybe a dedicated virtio-serial port that's not exposed
to other host processes; one that qemu would consume itself.

If you want to be able to panic in boot firmware, writing to an ioport
is easier than adding a new virtio driver (virtio-serial, or a
completely new device).

 None of the plans outlined below give us a proper solution.  I think
 removing is our best option at this point.

I'm just trolling ^W playing the devil's advocate here, giving you more
opportunity to argue your point :)

Thanks,
Laszlo

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

Laszlo Ersek ler...@redhat.com writes:

 On 08/22/13 18:44, Anthony Liguori wrote:

 pvpanic has been a failure.  It's a poorly designed device with even
 worse semantics.

 I disagree somewhat.

 Requiring a separate ioport is not ideal, I admit. Configuration over
 ACPI is good OTOH (it seems to put standards to good use anyway).

 Noone realized pvpanic had poor technical design until the Windows new
 device wizard popped up -- is that correct? Most of us are probably not
 habitual Windows users, which is probably why we haven't thought of it
 earlier.

Generating ACPI tables dynamically is painful and worse yet, it's 100%
ACPI specific.  Had we used virtio from the start, we would have had a
cross-architecture mechanism instead of a one-off x86-ism.

Yes, hind sight is 20/20 but that shouldn't stop us from doing things
right when presented the opportunity.

 Maybe we shouldn't promise there won't be guest-visible changes in ACPI
 contents. If we do promise, maybe we should then make the SeaBIOS
 binary that we're loading dependent on -M too too.

 After all, had we managed to completely hide the \_SB.PCI0.ISA.PEVT
 device programmatically, as opposed to only disabling it, we might have
 never realized pvpanic had poor design. Which (almost) means it wouldn't
 have had one.

 If we selected a SeaBIOS binary based on -M, then we could hide this
 stuff from Windows.

Yes, at some point I'm sure we'll hit the need for maintaining multiple
copies of SeaBIOS but that's going to make testing all that much
harder.  The longer we can avoid it the better IMHO.

 I applied it and I'll take the fault for merging it in
 the first place.
 
 We should simply scrap it and start over.

 That will kinda Eff some downstreams in the A...

If it's too late then we're stuck with it, but perhaps some of the
downstreams can skip up about what level of support they need for the
existing device in a bit more detail...

AFAICT, we've got something that's fundamentally broken right now so
downstreams are already in a bind if they're planning on supporting this
device.

 It has so few users at this
 point that this is still a realistic option.  Using something based on
 ISA that requires specific ACPI entries was a mistake.
 
 We should just introduce a simple watchdog device based on virtio and
 call it a day.  Then it's cross platform, solves the guest enumeration
 problem, and libvirt can detect the presence of the new device.

 If the guest doesn't initialize the proposed virtio-panic device, then
 it will lie dormant too, just like the current pvpanic device. That's
 good.

Ack.

 However a new (standalone) virtio device will take up yet another PCI
 function (a full device if you want it to be hotpluggable). PCI
 functions are scarcer than ioports.

We can always use bridges to expand the number of slots available.  If
we truly run into a situation where slots become too scarce, then we can
look at introducing a PCI-to-N-virtio-devices bridge.

 It will need documentation in the virtio-spec as well.

Ack.

 We'd need an arbitrarily heavily multiplexed paravirt channel between
 guest and qemu. Maybe a dedicated virtio-serial port that's not exposed
 to other host processes; one that qemu would consume itself.

I don't think using virtio-serial would be a good approach.

If we make the panic flag a config space variable, it makes it pretty
easy for firmware to use since it's still just an ioport write.

 If you want to be able to panic in boot firmware, writing to an ioport
 is easier than adding a new virtio driver (virtio-serial, or a
 completely new device).

See above.

 None of the plans outlined below give us a proper solution.  I think
 removing is our best option at this point.

 I'm just trolling ^W playing the devil's advocate here, giving you more
 opportunity to argue your point :)

It's really a tremendously simple virtio device to start with.  No
queues, just a configuration space with a single flag.

Down the road, I can imagine lots of useful extensions like the ability
to send a stack trace to the host as part of the panic.  That would be
mighty handy.  That's easy to add with virtio but pretty much impossible
with the current device.

Plus adding watchdog functionality would be a logical extension too.  I
believe that the watchdogs we emulate today are not supported by a
majority of guests.  A virtio-ras device with Windows and Linux drivers
would give us a universally supported watchdog device.

Regards,

Anthony Liguori


 Thanks,
 Laszlo

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

Laszlo Ersek ler...@redhat.com writes:

 On 08/22/13 18:10, Paolo Bonzini wrote:
 The thread from yesterday has died off (perhaps also because of
 my inappropriate answer to Michael, for which I apologize to him
 and everyone).  I took some time to discuss the libvirt requirements
 further with Daniel Berrange and Eric Blake on IRC.  If anyone is
 interested, I can give logs.  This is a suggestion for how to
 proceed in both QEMU and libvirt.

 The analysis is pretty overwhelming :)

 I have read Anthony's response and I'm not trying to argue -- I'm just
 spending a few thoughts on this and I'm willing to let them go to waste.

 In general I think we should minimize the quirks the user (who edits the
 libvirt XML) has to know about. Interactions between some XML elements,
 without explicit inter-references (formulated as attributes, like
 controller/index) are Bad (TM). So,

 == Builtin pvpanic ==
 
 QEMU will remove pvpanic from pc-1.5 in 1.6.1 and 1.5.4.  This does not
 break migration.
 
 
 == Support in libvirt for current functionality ==
 
 libvirt will add a panic-notifier/ element, and possibly a capability
 for it accessible via virsh capabilities.  There are two possibilities:
 
 1) On QEMU 1.5.4/1.6.1 and newer (and on QEMU 1.6.0 with a machine type
other than pc-1.5), on_crash will only work if the element is there.
On QEMU 1.5.0-1.5.3, and on QEMU 1.6.0 with the pc-1.5 machine type,
on_crash will be obeyed always, and may override e.g. reboot-on-panic
if a guest driver exist.

 I don't like this because there's some interplay between on_crash and
 panic_notifier, which even depends on the qemu version.


 
 2) On all versions, on_crash will only work if the element is there.

 I like this, because, if on_crash doesn't work without panic_notifier
 *at all*, then we can just drop panic_notifier, and make on_crash mean
 (on_crash  panic_notifier) in the original sense.

 IOW, drop panic_notifier, and make on_crash work *always*.

 
 
 In turn, there are two ways to implement (2):
 
 2a) libvirt will always add -global pvpanic.iobase=0 to neutralize
 the builtin pvpanic device if present.  panic-notifier/
 will create the device with -device pvpanic,iobase=0x505
 
 Advantage: no changes to QEMU
 
 Disadvantage 1: writes to port 0 with QEMU 1.{5.0,5.1,5.2,5.3,6.0}
   and pc-1.5 machine type will write to a pvpanic device instead of
   the DMA controller.  Probably harmless, and limited to some QEMU
   versions.
 
 Disadvantage 2: libvirt has knowledge of the pvpanic port number

 Updating this paragraph with my above suggestion:

 - (s/pvpanic.iobase/pvpanic.ioport/g)

 - if on_crash is absent:
   - for 1.{5.0,5.1,5.2,5.3,6.0}, add -global pvpanic.ioport=0
   - for other versions, do nothing

 - if on_crash is present:
   - for 1.{5.0,5.1,5.2,5.3,6.0}, do nothing,
   - for other versions, pass -device pvpanic
 (knowledge of 0x505 is unneeded)

Just to further complicate things...

pvpanic is not going to be present on S390, PPC, ARM, or MIPS because of
the fact that it's x86 specific.

That means at some point there's going to have to be another device to
support these platforms and libvirt will need to deal with that too.

Regards,

Anthony Liguori

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 22/08/2013 19:53, Laszlo Ersek ha scritto:
  We should just introduce a simple watchdog device based on virtio and
  call it a day.  Then it's cross platform, solves the guest enumeration
  problem, and libvirt can detect the presence of the new device.
 If the guest doesn't initialize the proposed virtio-panic device, then
 it will lie dormant too, just like the current pvpanic device. That's good.
 
 However a new (standalone) virtio device will take up yet another PCI
 function (a full device if you want it to be hotpluggable). PCI
 functions are scarcer than ioports.

Not just that.  Panic notifiers are called in a substantially unknown
environment, with locks taken or interrupts already set up.

This is why we went for a simple ISA device.  Configuration via ACPI
follows naturally from there, and anyway any other standard of the day
would have had the same problem with Windows.  At some point we had ACPI
methods instead of a simple ioport write, but we had to remove that
because the ACPI subsystem might have had its lock taken.

Also, a virtio watchdog device makes little sense, IMHO.  PV makes sense
if emulation has insufficient performance, excessive CPU usage, or
excessive complexity.  We already have both an ISA and a PCI watchdog,
and they serve their purpose wonderfully.

Paolo

Re: [Qemu-devel] pvpanic plans?

[Paolo Bonzini]

Il 22/08/2013 19:15, Laszlo Ersek ha scritto:
 2) On all versions, on_crash will only work if the element is there.
 
 I like this, because, if on_crash doesn't work without panic_notifier
 *at all*, then we can just drop panic_notifier, and make on_crash mean
 (on_crash  panic_notifier) in the original sense.
 
 IOW, drop panic_notifier, and make on_crash work *always*.

No, we cannot because of backwards compatibility.  VMs could have no
on_crash element (which means on_crashdestroy/on_crash) and yet the
guest admin could expect them to reboot on panic.


 2b) QEMU will provide a way for libvirt to detect that no machine type
 has the builtin pvpanic.  If some machine type may have the builtin
 pvpanic, and panic-notifier/ is absent, libvirt will add
 -global pvpanic.iobase=0 to neutralize it.  Otherwise, libvirt
 will create the device normally.

 A possible way for libvirt to detect good machine types is a
 dummy property.  This is a bit ugly in that the property would not
 affect the behavior of the device.  The property would remain in
 the long term.

 Another possibility is for QEMU to rename the device, e.g. to
 isa-pvpanic.  This is also somewhat gross, but not visible in the
 long term when the pvpanic name will be lost in history.

 Advantage 1: libvirt has no knowledge of the pvpanic port number

 Disadvantage 1: same as above

 Disadvantage 2: need a somewhat gross change in QEMU


 This method also provides an (also somewhat gross on the QEMU side)
 way to detect other changes in the pvpanic semantics.  One example
 mentioned below, is making the panicked state temporary.
 
 Too much work in qemu, in order to introduce ugliness, to hide older
 ugliness.

Is it too much work?  s/pvpanic/isa-pvpanic?

Paolo

Re: [Qemu-devel] pvpanic plans?

[Laszlo Ersek]

On 08/22/13 21:19, Paolo Bonzini wrote:
 Il 22/08/2013 19:15, Laszlo Ersek ha scritto:
 2) On all versions, on_crash will only work if the element is there.

 I like this, because, if on_crash doesn't work without panic_notifier
 *at all*, then we can just drop panic_notifier, and make on_crash mean
 (on_crash  panic_notifier) in the original sense.

 IOW, drop panic_notifier, and make on_crash work *always*.
 
 No, we cannot because of backwards compatibility.  VMs could have no
 on_crash element (which means on_crashdestroy/on_crash) and yet the
 guest admin could expect them to reboot on panic.

Ah. I thought no on_crash meant on_crashignore/on_crash, or
something like that -- if on_crash was absent, the guest wouldn't see a
working pvpanic device in ACPI, and wouldn't trigger the event in qemu.

 2b) QEMU will provide a way for libvirt to detect that no machine type
 has the builtin pvpanic.  If some machine type may have the builtin
 pvpanic, and panic-notifier/ is absent, libvirt will add
 -global pvpanic.iobase=0 to neutralize it.  Otherwise, libvirt
 will create the device normally.

 A possible way for libvirt to detect good machine types is a
 dummy property.  This is a bit ugly in that the property would not
 affect the behavior of the device.  The property would remain in
 the long term.

 Another possibility is for QEMU to rename the device, e.g. to
 isa-pvpanic.  This is also somewhat gross, but not visible in the
 long term when the pvpanic name will be lost in history.

 Advantage 1: libvirt has no knowledge of the pvpanic port number

 Disadvantage 1: same as above

 Disadvantage 2: need a somewhat gross change in QEMU


 This method also provides an (also somewhat gross on the QEMU side)
 way to detect other changes in the pvpanic semantics.  One example
 mentioned below, is making the panicked state temporary.

 Too much work in qemu, in order to introduce ugliness, to hide older
 ugliness.
 
 Is it too much work?  s/pvpanic/isa-pvpanic?

... I probably skipped the rename option because you called it gross
(and maybe because I (erroneously?) recall Michael's opposition). I
think I meant the dummy property under too much work (it may not be,
in retrospect, but properties always imply compat stuff for me, and
*that* is scary).

Laszlo

Re: [Qemu-devel] pvpanic plans?

[Christian Borntraeger]

On 22/08/13 20:33, Anthony Liguori wrote:
 Laszlo Ersek ler...@redhat.com writes:
 
 On 08/22/13 18:10, Paolo Bonzini wrote:
 The thread from yesterday has died off (perhaps also because of
 my inappropriate answer to Michael, for which I apologize to him
 and everyone).  I took some time to discuss the libvirt requirements
 further with Daniel Berrange and Eric Blake on IRC.  If anyone is
 interested, I can give logs.  This is a suggestion for how to
 proceed in both QEMU and libvirt.

 The analysis is pretty overwhelming :)

 I have read Anthony's response and I'm not trying to argue -- I'm just
 spending a few thoughts on this and I'm willing to let them go to waste.

 In general I think we should minimize the quirks the user (who edits the
 libvirt XML) has to know about. Interactions between some XML elements,
 without explicit inter-references (formulated as attributes, like
 controller/index) are Bad (TM). So,

 == Builtin pvpanic ==

 QEMU will remove pvpanic from pc-1.5 in 1.6.1 and 1.5.4.  This does not
 break migration.


 == Support in libvirt for current functionality ==

 libvirt will add a panic-notifier/ element, and possibly a capability
 for it accessible via virsh capabilities.  There are two possibilities:

 1) On QEMU 1.5.4/1.6.1 and newer (and on QEMU 1.6.0 with a machine type
other than pc-1.5), on_crash will only work if the element is there.
On QEMU 1.5.0-1.5.3, and on QEMU 1.6.0 with the pc-1.5 machine type,
on_crash will be obeyed always, and may override e.g. reboot-on-panic
if a guest driver exist.

 I don't like this because there's some interplay between on_crash and
 panic_notifier, which even depends on the qemu version.



 2) On all versions, on_crash will only work if the element is there.

 I like this, because, if on_crash doesn't work without panic_notifier
 *at all*, then we can just drop panic_notifier, and make on_crash mean
 (on_crash  panic_notifier) in the original sense.

 IOW, drop panic_notifier, and make on_crash work *always*.



 In turn, there are two ways to implement (2):

 2a) libvirt will always add -global pvpanic.iobase=0 to neutralize
 the builtin pvpanic device if present.  panic-notifier/
 will create the device with -device pvpanic,iobase=0x505

 Advantage: no changes to QEMU

 Disadvantage 1: writes to port 0 with QEMU 1.{5.0,5.1,5.2,5.3,6.0}
   and pc-1.5 machine type will write to a pvpanic device instead of
   the DMA controller.  Probably harmless, and limited to some QEMU
   versions.

 Disadvantage 2: libvirt has knowledge of the pvpanic port number

 Updating this paragraph with my above suggestion:

 - (s/pvpanic.iobase/pvpanic.ioport/g)

 - if on_crash is absent:
   - for 1.{5.0,5.1,5.2,5.3,6.0}, add -global pvpanic.ioport=0
   - for other versions, do nothing

 - if on_crash is present:
   - for 1.{5.0,5.1,5.2,5.3,6.0}, do nothing,
   - for other versions, pass -device pvpanic
 (knowledge of 0x505 is unneeded)
 
 Just to further complicate things...
 
 pvpanic is not going to be present on S390, PPC, ARM, or MIPS because of
 the fact that it's x86 specific.

What about crashed state? I have implemented this state after the guest entered
disabled wait (by convention used by all s390 OSes for crashes)

See commit 08eb8c85e3967b97865d46acadf26dc908fbb094
Author: Christian Borntraeger borntrae...@de.ibm.com
Date:   Fri Apr 26 11:24:47 2013 +0800

Wire up disabled wait a panicked event on s390



Should we remove that as well? From s390 point of view, after allowing
the crashed-running transition the feature would probably work as expected.


Christian

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

Paolo Bonzini pbonz...@redhat.com writes:

 Il 22/08/2013 19:53, Laszlo Ersek ha scritto:
  We should just introduce a simple watchdog device based on virtio and
  call it a day.  Then it's cross platform, solves the guest enumeration
  problem, and libvirt can detect the presence of the new device.
 If the guest doesn't initialize the proposed virtio-panic device, then
 it will lie dormant too, just like the current pvpanic device. That's good.
 
 However a new (standalone) virtio device will take up yet another PCI
 function (a full device if you want it to be hotpluggable). PCI
 functions are scarcer than ioports.

 Not just that.  Panic notifiers are called in a substantially unknown
 environment, with locks taken or interrupts already set up.

If you make the panic notify a config space write, then on virtio-pci,
it's an outb to a fixed offset within a io address that after boot is
static.

So the address could be stored in a global and accessed without a lock.

 This is why we went for a simple ISA device.

Simple ISA device doesn't exist outside of x86 and as we are learning,
it's not all that simple.

 Configuration via ACPI
 follows naturally from there, and anyway any other standard of the day
 would have had the same problem with Windows.  At some point we had ACPI
 methods instead of a simple ioport write, but we had to remove that
 because the ACPI subsystem might have had its lock taken.

The difference is that ACPI or platform devices in general are
unexpected to be added.  By definition it means that the motherboard has
most likely been changed.

OTOH, a new PCI device is expected and most OSes will deal gracefully
with it.


 Also, a virtio watchdog device makes little sense, IMHO.  PV makes sense
 if emulation has insufficient performance, excessive CPU usage, or
 excessive complexity.  We already have both an ISA and a PCI watchdog,
 and they serve their purpose wonderfully.

Neither of which actually work with modern versions of Windows FWIW.

Plus emulated watchdogs do not take into account steal time or
overcommit in general.  I've seen multiple cases where a naive watchdog
has a problem in the field when the system is under heavy load.

A PV watchdog actually makes sense because it can be defined based on
guest run time instead of wall clock time.

Regards,

Anthony Liguori


 Paolo

Re: [Qemu-devel] pvpanic plans?

[Laszlo Ersek]

On 08/22/13 22:09, Anthony Liguori wrote:

 The difference is that ACPI or platform devices in general are
 unexpected to be added.  By definition it means that the motherboard has
 most likely been changed.

You could encounter a new ACPI artifact after simply re-flashing your MB
with an updated BIOS, without opening the chassis. If windows can't
deal with that, their loss! :)

Laszlo
/hides

Re: [Qemu-devel] pvpanic plans?

[Anthony Liguori]

On Thu, Aug 22, 2013 at 3:36 PM, Laszlo Ersek ler...@redhat.com wrote:
 On 08/22/13 22:09, Anthony Liguori wrote:

 The difference is that ACPI or platform devices in general are
 unexpected to be added.  By definition it means that the motherboard has
 most likely been changed.

 You could encounter a new ACPI artifact after simply re-flashing your MB
 with an updated BIOS, without opening the chassis. If windows can't
 deal with that, their loss! :)

I'm pretty sure does Windows boot up okay is on every major vendor's
firmware test plan for shipping new updates...

Regards,

Anthony Liguori

 Laszlo
 /hides

Re: [Qemu-devel] pvpanic plans?

[Peter Maydell]

On 22 August 2013 21:09, Anthony Liguori anth...@codemonkey.ws wrote:
 Paolo Bonzini pbonz...@redhat.com writes:
 Not just that.  Panic notifiers are called in a substantially unknown
 environment, with locks taken or interrupts already set up.

 If you make the panic notify a config space write, then on virtio-pci,
 it's an outb to a fixed offset within a io address that after boot is
 static.

 So the address could be stored in a global and accessed without a lock.

Fine for virtio-mmio too, obviously. I have a vague recollection that
config space writes on virtio-s390 are weird though. (would also
be an issue if we wanted to implement the virtio-console emergency
write functionality.)

-- PMM