| Message ID | 1269442173-18421-11-git-send-email-amit.shah@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Wed, 24 Mar 2010 20:19:28 +0530 Amit Shah <amit.shah@redhat.com> wrote: > When adding a port or a device to the guest fails, management software > might be interested in knowing and then cleaning up the host-side of the > port. Introduce QMP events to signal such errors. I'm completely unfamiliar with virtio-serial, so let me ask: how are ports added? I'd expect the command performing this operation to fail in this case.
On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > On Wed, 24 Mar 2010 20:19:28 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > When adding a port or a device to the guest fails, management software > > might be interested in knowing and then cleaning up the host-side of the > > port. Introduce QMP events to signal such errors. > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > added? I'd expect the command performing this operation to fail in this case. If adding the port fails in qemu, then the command will fail. However if adding the port in the guest fails, we raise this QMP event. Adding in the guest could fail because of several reasons, like ENOMEM. In this case, the mgmt might want to hot-unplug the port from qemu so that resources are freed and also apps are notified of guest side not ready. Amit
On Thu, 25 Mar 2010 09:17:17 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > On Wed, 24 Mar 2010 20:19:28 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > When adding a port or a device to the guest fails, management software > > > might be interested in knowing and then cleaning up the host-side of the > > > port. Introduce QMP events to signal such errors. > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > added? I'd expect the command performing this operation to fail in this case. > > If adding the port fails in qemu, then the command will fail. However if > adding the port in the guest fails, we raise this QMP event. Adding in > the guest could fail because of several reasons, like ENOMEM. In this > case, the mgmt might want to hot-unplug the port from qemu so that > resources are freed and also apps are notified of guest side not ready. Ok, what about a disconnect? Does virtio-serial have this kind of action?
On Wed, 24 Mar 2010 20:19:28 +0530 Amit Shah <amit.shah@redhat.com> wrote: > When adding a port or a device to the guest fails, management software > might be interested in knowing and then cleaning up the host-side of the > port. Introduce QMP events to signal such errors. > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > CC: Luiz Capitulino <lcapitulino@redhat.com> > --- > QMP/qmp-events.txt | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > hw/virtio-serial-bus.c | 15 +++++++++++++++ > monitor.c | 3 +++ > monitor.h | 1 + > 4 files changed, 67 insertions(+), 0 deletions(-) > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > index a94e9b4..f13cf45 100644 > --- a/QMP/qmp-events.txt > +++ b/QMP/qmp-events.txt > @@ -188,3 +188,51 @@ Example: > > Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is > followed respectively by the RESET, SHUTDOWN, or STOP events. > + > +VIRTIO_SERIAL > +------------- It should be VIRTIO_SERIAL_ADD. > + > +Emitted when errors occur in guest port add or guest device add. > + > +Data: > + > +- "device": The virtio-serial device that triggered the event {json-string} > + This is the name given to the bus on the command line: > + -device virtio-serial,id="foo" > + here, the device name is "foo" > + > +- "port": The port number that triggered the event {json-number} > + This is the number given to the port on the command line: > + -device virtserialport,nr=2 > + here, the port number is 2. If not mentioned on the command > + line, the number is auto-assigned. We use (json-number) instead of {json-number}. > + > +- "result": The result of the operation {json-string} > + This is one of the following: > + "pass", "fail" "result" could be a boolean "success". > + > +- "operation": The operation that triggered the event {json-sring} > + This is one of the following: > + "port_add", "device_add" You can drop the '_add', as this information is in the event name. > + > +Example: > + > +Port 0 add failure in the guest: > + > +{ "timestamp": {"seconds": 1269438649, "microseconds": 851170}, > + "event": "VIRTIO_SERIAL", > + "data": { > + "device": "virtio-serial-bus.0", > + "port": 0, > + "result": "fail", > + "operation": "port_add" } } If you look at the other events you will see that I put the event first and the timestamp later, I know this is not how the event is going to be on the wire but improves the readability of this file (and the spec says that clients should not assume the ordering of dicts or lists). Implementation looks ok.
Luiz Capitulino wrote: > On Thu, 25 Mar 2010 09:17:17 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > > On Wed, 24 Mar 2010 20:19:28 +0530 > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > When adding a port or a device to the guest fails, management software > > > > might be interested in knowing and then cleaning up the host-side of the > > > > port. Introduce QMP events to signal such errors. > > > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > > added? I'd expect the command performing this operation to fail in this case. > > > > If adding the port fails in qemu, then the command will fail. However if > > adding the port in the guest fails, we raise this QMP event. Adding in > > the guest could fail because of several reasons, like ENOMEM. In this > > case, the mgmt might want to hot-unplug the port from qemu so that > > resources are freed and also apps are notified of guest side not ready. > > Ok, what about a disconnect? Does virtio-serial have this kind of action? Disconnect would be valuable. E.g. if the guest app dies (but not the guest kernel), it won't get a chance to send an "I'm going away" message. Machine reboot, PCI reset and so on, should probably trigger a disconnect event too (perhaps annotated with the reason), so that the host app's byte stream parser can reliably start parsing anew. Somehow that reset event needs to be synchronised with data transport, so the host app works when the guest boots, reconnects and sends more data before the host app has had enough time to process the earlier reset event. Connect event is a nice idea for symmetry, so the host knows when the guest app has started up, but it's not essential as the guest app can just send a message. -- Jamie
On (Thu) Mar 25 2010 [15:34:06], Luiz Capitulino wrote: > On Thu, 25 Mar 2010 09:17:17 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > > On Wed, 24 Mar 2010 20:19:28 +0530 > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > When adding a port or a device to the guest fails, management software > > > > might be interested in knowing and then cleaning up the host-side of the > > > > port. Introduce QMP events to signal such errors. > > > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > > added? I'd expect the command performing this operation to fail in this case. > > > > If adding the port fails in qemu, then the command will fail. However if > > adding the port in the guest fails, we raise this QMP event. Adding in > > the guest could fail because of several reasons, like ENOMEM. In this > > case, the mgmt might want to hot-unplug the port from qemu so that > > resources are freed and also apps are notified of guest side not ready. > > Ok, what about a disconnect? Does virtio-serial have this kind of action? In case of guest disconnect, an event is sent by the guest to the host and the host sends it to the app. A qmp event is not triggered, one can be, if desired. Amit
On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote: > Luiz Capitulino wrote: > > On Thu, 25 Mar 2010 09:17:17 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > > > On Wed, 24 Mar 2010 20:19:28 +0530 > > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > > > When adding a port or a device to the guest fails, management software > > > > > might be interested in knowing and then cleaning up the host-side of the > > > > > port. Introduce QMP events to signal such errors. > > > > > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > > > added? I'd expect the command performing this operation to fail in this case. > > > > > > If adding the port fails in qemu, then the command will fail. However if > > > adding the port in the guest fails, we raise this QMP event. Adding in > > > the guest could fail because of several reasons, like ENOMEM. In this > > > case, the mgmt might want to hot-unplug the port from qemu so that > > > resources are freed and also apps are notified of guest side not ready. > > > > Ok, what about a disconnect? Does virtio-serial have this kind of action? > > Disconnect would be valuable. E.g. if the guest app dies (but not the > guest kernel), it won't get a chance to send an "I'm going away" > message. That's something applications should be able to handle: If an app on the guest dies, the app on the host should be able to discover this. In any case, we have 'open' and 'close' notifications where we trigger callbacks for the applications if they're interested in such events. This only works for in-qemu apps, though, so I'm OK with adding a QMP event for this as well. > Machine reboot, PCI reset and so on, should probably trigger a All these messages belong to other subsystems, not virtio-serial. Eg, libvirt or other mgmt app should know that a reset event, when received, affects virtio ports as well. Similar for pci events. Amit
On (Thu) Mar 25 2010 [15:55:41], Luiz Capitulino wrote: > On Wed, 24 Mar 2010 20:19:28 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > When adding a port or a device to the guest fails, management software > > might be interested in knowing and then cleaning up the host-side of the > > port. Introduce QMP events to signal such errors. > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > > CC: Luiz Capitulino <lcapitulino@redhat.com> > > --- > > QMP/qmp-events.txt | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > hw/virtio-serial-bus.c | 15 +++++++++++++++ > > monitor.c | 3 +++ > > monitor.h | 1 + > > 4 files changed, 67 insertions(+), 0 deletions(-) > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > index a94e9b4..f13cf45 100644 > > --- a/QMP/qmp-events.txt > > +++ b/QMP/qmp-events.txt > > @@ -188,3 +188,51 @@ Example: > > > > Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is > > followed respectively by the RESET, SHUTDOWN, or STOP events. > > + > > +VIRTIO_SERIAL > > +------------- > > It should be VIRTIO_SERIAL_ADD. What about other events that VIRTIO_SERIAL generates? Should they have a different event by themselves? Or should they ride on top of VIRTIO_SERIAL and mention different 'operations' that caused the event? > > +Emitted when errors occur in guest port add or guest device add. > > + > > +Data: > > + > > +- "device": The virtio-serial device that triggered the event {json-string} > > + This is the name given to the bus on the command line: > > + -device virtio-serial,id="foo" > > + here, the device name is "foo" > > + > > +- "port": The port number that triggered the event {json-number} > > + This is the number given to the port on the command line: > > + -device virtserialport,nr=2 > > + here, the port number is 2. If not mentioned on the command > > + line, the number is auto-assigned. > > We use (json-number) instead of {json-number}. Fixed. > > + > > +- "result": The result of the operation {json-string} > > + This is one of the following: > > + "pass", "fail" > > "result" could be a boolean "success". OK; success/fail? Also, by boolean, do you mean the data type? How is that represented? (Note: I put the port number as '%ld' instead of '%u' since %u isn't parsed..) > > +- "operation": The operation that triggered the event {json-sring} > > + This is one of the following: > > + "port_add", "device_add" > > You can drop the '_add', as this information is in the event name. The answer to the first question above will answer this too. > > +Example: > > + > > +Port 0 add failure in the guest: > > + > > +{ "timestamp": {"seconds": 1269438649, "microseconds": 851170}, > > + "event": "VIRTIO_SERIAL", > > + "data": { > > + "device": "virtio-serial-bus.0", > > + "port": 0, > > + "result": "fail", > > + "operation": "port_add" } } > > If you look at the other events you will see that I put the event > first and the timestamp later, I know this is not how the event is > going to be on the wire but improves the readability of this file > (and the spec says that clients should not assume the ordering of > dicts or lists). OK; I'll do that. > Implementation looks ok. OK, thanks. Amit
Amit Shah wrote: > On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote: > > Luiz Capitulino wrote: > > > On Thu, 25 Mar 2010 09:17:17 +0530 > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > > > > On Wed, 24 Mar 2010 20:19:28 +0530 > > > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > > > > > When adding a port or a device to the guest fails, management software > > > > > > might be interested in knowing and then cleaning up the host-side of the > > > > > > port. Introduce QMP events to signal such errors. > > > > > > > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > > > > added? I'd expect the command performing this operation to fail in this case. > > > > > > > > If adding the port fails in qemu, then the command will fail. However if > > > > adding the port in the guest fails, we raise this QMP event. Adding in > > > > the guest could fail because of several reasons, like ENOMEM. In this > > > > case, the mgmt might want to hot-unplug the port from qemu so that > > > > resources are freed and also apps are notified of guest side not ready. > > > > > > Ok, what about a disconnect? Does virtio-serial have this kind of action? > > > > Disconnect would be valuable. E.g. if the guest app dies (but not the > > guest kernel), it won't get a chance to send an "I'm going away" > > message. > > That's something applications should be able to handle: If an app on the > guest dies, the app on the host should be able to discover this. Sure. Does the host app see an EOF on its input when that happens? (I.e. *not* like a real serial port). If so, there's no need for a disconnect event, otherwise, if it's like a serial port, there is. > > Machine reboot, PCI reset and so on, should probably trigger a > > All these messages belong to other subsystems, not virtio-serial. Eg, b> libvirt or other mgmt app should know that a reset event, when received, > affects virtio ports as well. Similar for pci events. Fine (although I don't like that a non-mgmt host app only interested talking to a guest with an architecture-neutral protocol might need to know about PCI, or even need to know anything about how the VM was launched). But what I really meant was, if the guest resets, and then it boots up before the host apps manage to process their events (e.g. due to timing, remoteness, swapping or whatever), it's important that the virtio-serial using host app knows where the discontinuity in the byte stream is. Otherwise the app needs to use a silly overcomplicated protocol just to provide a reliable layer over the byte stream. -- Jamie
On (Fri) Mar 26 2010 [04:07:54], Jamie Lokier wrote: > Amit Shah wrote: > > On (Fri) Mar 26 2010 [01:17:49], Jamie Lokier wrote: > > > Luiz Capitulino wrote: > > > > On Thu, 25 Mar 2010 09:17:17 +0530 > > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > > > On (Wed) Mar 24 2010 [17:34:15], Luiz Capitulino wrote: > > > > > > On Wed, 24 Mar 2010 20:19:28 +0530 > > > > > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > > > > > > > > When adding a port or a device to the guest fails, management software > > > > > > > might be interested in knowing and then cleaning up the host-side of the > > > > > > > port. Introduce QMP events to signal such errors. > > > > > > > > > > > > I'm completely unfamiliar with virtio-serial, so let me ask: how are ports > > > > > > added? I'd expect the command performing this operation to fail in this case. > > > > > > > > > > If adding the port fails in qemu, then the command will fail. However if > > > > > adding the port in the guest fails, we raise this QMP event. Adding in > > > > > the guest could fail because of several reasons, like ENOMEM. In this > > > > > case, the mgmt might want to hot-unplug the port from qemu so that > > > > > resources are freed and also apps are notified of guest side not ready. > > > > > > > > Ok, what about a disconnect? Does virtio-serial have this kind of action? > > > > > > Disconnect would be valuable. E.g. if the guest app dies (but not the > > > guest kernel), it won't get a chance to send an "I'm going away" > > > message. > > > > That's something applications should be able to handle: If an app on the > > guest dies, the app on the host should be able to discover this. > > Sure. Does the host app see an EOF on its input when that happens? > (I.e. *not* like a real serial port). If it's an in-qemu app, it gets the guest_closed() callback. So I guess qmp events for non-qemu apps is what you're looking for? > If so, there's no need for a disconnect event, otherwise, if it's like > a serial port, there is. > > > > Machine reboot, PCI reset and so on, should probably trigger a > > > > All these messages belong to other subsystems, not virtio-serial. Eg, > b> libvirt or other mgmt app should know that a reset event, when received, > > affects virtio ports as well. Similar for pci events. > > Fine (although I don't like that a non-mgmt host app only interested > talking to a guest with an architecture-neutral protocol might need to > know about PCI, or even need to know anything about how the VM was > launched). > > But what I really meant was, if the guest resets, and then it boots up > before the host apps manage to process their events (e.g. due to > timing, remoteness, swapping or whatever), it's important that the > virtio-serial using host app knows where the discontinuity in the byte > stream is. Otherwise the app needs to use a silly overcomplicated > protocol just to provide a reliable layer over the byte stream. I'd rather that the apps used the existing QMP notifications for guest reset so that we don't have to do anything special for virtio-serial (and for other devices as well). Also, if it might help, the 'guest device ready' and 'guest port ready' QMP events can be sent on success as well (right now they're only sent on failure). Amit
Amit Shah wrote: > > Sure. Does the host app see an EOF on its input when that happens? > > (I.e. *not* like a real serial port). > > If it's an in-qemu app, it gets the guest_closed() callback. So I guess > qmp events for non-qemu apps is what you're looking for? See below. > > But what I really meant was, if the guest resets, and then it boots up > > before the host apps manage to process their events (e.g. due to > > timing, remoteness, swapping or whatever), it's important that the > > virtio-serial using host app knows where the discontinuity in the byte > > stream is. Otherwise the app needs to use a silly overcomplicated > > protocol just to provide a reliable layer over the byte stream. > > I'd rather that the apps used the existing QMP notifications for guest > reset so that we don't have to do anything special for virtio-serial > (and for other devices as well). I'm trying to understand how to avoid the race condition with that. 1. guest sends a big blob of data to virtio-serial 2. qemu writes to socket for host app -> wakes up host app (outside qemu) listening for virtio-serial data 3. guest resets or its kernel crashes -> the big blob is only partially sent 4. qemu sends QMP notification 5. -> wakes up host app listening for QMP events 6. guest boots up. 7. guest opens virtio-serial, and starts by sending a message. 8. -> host app gets to run, sees the event sent in step 2. 9. -> host app reads available data from virtio-serial data includes bytes sent in step 1 and step 7 Can the host app tell which bytes to discard because they were a truncated message sent prior to the reset, so that it can find the start of the new message sent in step 7? A QMP event has that race condition. If communication with the external host app is over a local socket (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and reconnect whenever the guest does, which is perfectly logical and solves this. If the external host app is fork+exec'd from qemu with a pair of pipes (",exec="), closing the writing pipe, waiting for EOF from the reading pipe, and then re-exec-ing the external app would do as well. If neither of those are used, then a bit more context from QMP is needed, such as the exact number of bytes transmitted prior to the reset, presumably in a virtio-serial close event. -- Jamie
On Fri, 26 Mar 2010 10:26:07 +0530 Amit Shah <amit.shah@redhat.com> wrote: > Also, if it might help, the 'guest device ready' and 'guest port ready' > QMP events can be sent on success as well (right now they're only sent > on failure). Although this seems to be make, would be good to have clients' writers feedback on this or wait until this is really needed. I'd like to avoid over designing.
On Fri, 26 Mar 2010 07:46:28 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Thu) Mar 25 2010 [15:55:41], Luiz Capitulino wrote: > > On Wed, 24 Mar 2010 20:19:28 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > When adding a port or a device to the guest fails, management software > > > might be interested in knowing and then cleaning up the host-side of the > > > port. Introduce QMP events to signal such errors. > > > > > > Signed-off-by: Amit Shah <amit.shah@redhat.com> > > > CC: Luiz Capitulino <lcapitulino@redhat.com> > > > --- > > > QMP/qmp-events.txt | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > hw/virtio-serial-bus.c | 15 +++++++++++++++ > > > monitor.c | 3 +++ > > > monitor.h | 1 + > > > 4 files changed, 67 insertions(+), 0 deletions(-) > > > > > > diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt > > > index a94e9b4..f13cf45 100644 > > > --- a/QMP/qmp-events.txt > > > +++ b/QMP/qmp-events.txt > > > @@ -188,3 +188,51 @@ Example: > > > > > > Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is > > > followed respectively by the RESET, SHUTDOWN, or STOP events. > > > + > > > +VIRTIO_SERIAL > > > +------------- > > > > It should be VIRTIO_SERIAL_ADD. > > What about other events that VIRTIO_SERIAL generates? We don't address this problem currently, maybe an integration with qdev will do, but I have to think more about it. > Should they have a different event by themselves? With the current code, yes. But would be good to avoid it until we have a proper solution. > Or should they ride on top of VIRTIO_SERIAL and mention different > 'operations' that caused the event? I'd prefer having a different name if it's a different event, at least this is how we've done it so far. > > > + > > > +- "result": The result of the operation {json-string} > > > + This is one of the following: > > > + "pass", "fail" > > > > "result" could be a boolean "success". > > OK; success/fail? Also, by boolean, do you mean the data type? How is > that represented? In JSON it's true/false. In our parser you can use '%i' with integers, undocumented, yes, sorry for that.
On (Fri) Mar 26 2010 [10:05:07], Luiz Capitulino wrote: > On Fri, 26 Mar 2010 10:26:07 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > Also, if it might help, the 'guest device ready' and 'guest port ready' > > QMP events can be sent on success as well (right now they're only sent > > on failure). > > Although this seems to be make, would be good to have clients' writers > feedback on this or wait until this is really needed. Right. Amit
On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote: > > > > + > > > > +VIRTIO_SERIAL > > > > +------------- > > > > > > It should be VIRTIO_SERIAL_ADD. > > > > What about other events that VIRTIO_SERIAL generates? > > We don't address this problem currently, maybe an integration with qdev > will do, but I have to think more about it. So should I just keep it as VIRTIO_SERIAL for now? With new events also riding on this one? > > Should they have a different event by themselves? > > With the current code, yes. But would be good to avoid it until we have > a proper solution. > > > Or should they ride on top of VIRTIO_SERIAL and mention different > > 'operations' that caused the event? > > I'd prefer having a different name if it's a different event, at least > this is how we've done it so far. Erm, now I'm confused. > > > > + > > > > +- "result": The result of the operation {json-string} > > > > + This is one of the following: > > > > + "pass", "fail" > > > > > > "result" could be a boolean "success". > > > > OK; success/fail? Also, by boolean, do you mean the data type? How is > > that represented? > > In JSON it's true/false. In our parser you can use '%i' with integers, > undocumented, yes, sorry for that. Oh ok; no problem. Amit
On (Fri) Mar 26 2010 [05:23:25], Jamie Lokier wrote: > Amit Shah wrote: > > > Sure. Does the host app see an EOF on its input when that happens? > > > (I.e. *not* like a real serial port). > > > > If it's an in-qemu app, it gets the guest_closed() callback. So I guess > > qmp events for non-qemu apps is what you're looking for? > > See below. > > > > But what I really meant was, if the guest resets, and then it boots up > > > before the host apps manage to process their events (e.g. due to > > > timing, remoteness, swapping or whatever), it's important that the > > > virtio-serial using host app knows where the discontinuity in the byte > > > stream is. Otherwise the app needs to use a silly overcomplicated > > > protocol just to provide a reliable layer over the byte stream. > > > > I'd rather that the apps used the existing QMP notifications for guest > > reset so that we don't have to do anything special for virtio-serial > > (and for other devices as well). > > I'm trying to understand how to avoid the race condition with that. > > 1. guest sends a big blob of data to virtio-serial > 2. qemu writes to socket for host app > -> wakes up host app (outside qemu) listening for virtio-serial data > 3. guest resets or its kernel crashes > -> the big blob is only partially sent > 4. qemu sends QMP notification > 5. -> wakes up host app listening for QMP events > 6. guest boots up. > 7. guest opens virtio-serial, and starts by sending a message. > 8. -> host app gets to run, sees the event sent in step 2. > 9. -> host app reads available data from virtio-serial > data includes bytes sent in step 1 and step 7 > > Can the host app tell which bytes to discard because they were a > truncated message sent prior to the reset, so that it can find the > start of the new message sent in step 7? > > A QMP event has that race condition. Problem is we're going to have to maintain a lot of state if we're going to provide guarantees. One solution is to always have an in-qemu user of the serial functionality that sits between the app and the guest and the in-qemu user can signal to the app about such things. > If communication with the external host app is over a local socket > (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and > reconnect whenever the guest does, which is perfectly logical and > solves this. The virtio-serial code cannot know what kind of a connection it has with the host app. > If the external host app is fork+exec'd from qemu with a pair of pipes > (",exec="), closing the writing pipe, waiting for EOF from the > reading pipe, and then re-exec-ing the external app would do as well. > > If neither of those are used, then a bit more context from QMP is > needed, such as the exact number of bytes transmitted prior to the > reset, presumably in a virtio-serial close event. Yeah; that's some state to maintain -- and I'm not sure we should do that. If we start adding some state now, we could end up adding more later, and it could soon become a mess. Amit
On Fri, 26 Mar 2010 18:56:20 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote: > > > > > + > > > > > +VIRTIO_SERIAL > > > > > +------------- > > > > > > > > It should be VIRTIO_SERIAL_ADD. > > > > > > What about other events that VIRTIO_SERIAL generates? > > > > We don't address this problem currently, maybe an integration with qdev > > will do, but I have to think more about it. > > So should I just keep it as VIRTIO_SERIAL for now? With new events also > riding on this one? I don't like this because with the current events code this will lead to confusion, as you're using a single event to notify different things. My suggestion for the immediate term is to do what we have been doing so far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way to group events which requires a new VIRTIO_SERIAL event, in this case we could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The latter would be deprecated too. Or, if you can wait I can _try_ to solve this problem next week, although I have no idea how hard this is going to be. Any comments, Anthony?
On (Fri) Mar 26 2010 [11:29:03], Luiz Capitulino wrote: > On Fri, 26 Mar 2010 18:56:20 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote: > > > > > > + > > > > > > +VIRTIO_SERIAL > > > > > > +------------- > > > > > > > > > > It should be VIRTIO_SERIAL_ADD. > > > > > > > > What about other events that VIRTIO_SERIAL generates? > > > > > > We don't address this problem currently, maybe an integration with qdev > > > will do, but I have to think more about it. > > > > So should I just keep it as VIRTIO_SERIAL for now? With new events also > > riding on this one? > > I don't like this because with the current events code this will lead > to confusion, as you're using a single event to notify different things. > > My suggestion for the immediate term is to do what we have been doing so > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way > to group events which requires a new VIRTIO_SERIAL event, in this case we > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The > latter would be deprecated too. I've no problem doing it either way - whatever you prefer is fine. BTW these are two distinct events already - failure in initialising a device and failure in initialising a port. Do you think these should be separate as well? > Or, if you can wait I can _try_ to solve this problem next week, although > I have no idea how hard this is going to be. I think it's cleaner to club everything; but basically I'll go with whatever you say. I've no problem waiting. > Any comments, Anthony? Amit
Amit Shah wrote: > Problem is we're going to have to maintain a lot of state if we're going > to provide guarantees. > > One solution is to always have an in-qemu user of the serial > functionality that sits between the app and the guest and the in-qemu > user can signal to the app about such things. Isn't that what I suggested? I don't mind how the app is signalled. I'm just thinking of the simplest way to do it. It doesn't have to live in the virtio-serial driver, just be signalled somehow out of it. > > If communication with the external host app is over a local socket > > (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and > > reconnect whenever the guest does, which is perfectly logical and > > solves this. > > The virtio-serial code cannot know what kind of a connection it has with > the host app. True, but the qemu internal plumbing could pass an open/close event to that connection handler, for it to do whatever's appropriate. I don't think that 1 bit state ("open" or "closed") is too much to carry around in migration etc. > > If neither of those are used, then a bit more context from QMP is > > needed, such as the exact number of bytes transmitted prior to the > > reset, presumably in a virtio-serial close event. > > Yeah; that's some state to maintain -- and I'm not sure we should do > that. I'd rather not count bytes anyway. That smacks too much of maintaining long term history. I'd rather it was 1 bit of state, and the host app connection handler able to use it. > If we start adding some state now, we could end up adding more > later, and it could soon become a mess. Each thing ought to be weighed on its merits. Without this specific thing, which is an indicator that guest has lost state outside its control, the guest<->host communication is unreliable (even for things like "cut and paste"), so every app that cares has to implement a packet framing protocol with no binary data (to reserve an escaping byte), or with CRCs like PPP-over-virtio-serial, which is complicated and silly imho. If it were a real serial port, not emulated, that's the sort of thing apps would actually do (or use timeouts, which are more dubious in emulator-land). But I hope we're not that sadistic :-) *Inband* open/close indication aren't 100% guarantees of reliability, but I think they raise it to the point where an app can usefully count on it. -- Jamie
On (Fri) Mar 26 2010 [14:44:23], Jamie Lokier wrote: > Amit Shah wrote: > > Problem is we're going to have to maintain a lot of state if we're going > > to provide guarantees. > > > > One solution is to always have an in-qemu user of the serial > > functionality that sits between the app and the guest and the in-qemu > > user can signal to the app about such things. > > Isn't that what I suggested? I don't mind how the app is signalled. > I'm just thinking of the simplest way to do it. It doesn't have to > live in the virtio-serial driver, just be signalled somehow out of it. You did? Well, then we agree on a solution :-) > > > If communication with the external host app is over a local socket > > > (AF_UNIX or AF_INET or mkpipe), qemu could just disconnect and > > > reconnect whenever the guest does, which is perfectly logical and > > > solves this. > > > > The virtio-serial code cannot know what kind of a connection it has with > > the host app. > > True, but the qemu internal plumbing could pass an open/close event to > that connection handler, for it to do whatever's appropriate. Which means the qemu chardev. Is it smart enough? Dunno. > I don't think that 1 bit state ("open" or "closed") is too much to > carry around in migration etc. If the app sits inside qemu then maintaining open/close isn't a problem at all. In fact, guest and host open/closed state is already maintained by virtio-serial. > > > If neither of those are used, then a bit more context from QMP is > > > needed, such as the exact number of bytes transmitted prior to the > > > reset, presumably in a virtio-serial close event. > > > > Yeah; that's some state to maintain -- and I'm not sure we should do > > that. > > I'd rather not count bytes anyway. That smacks too much of > maintaining long term history. I'd rather it was 1 bit of state, and > the host app connection handler able to use it. > > > If we start adding some state now, we could end up adding more > > later, and it could soon become a mess. > > Each thing ought to be weighed on its merits. > > Without this specific thing, which is an indicator that guest has lost > state outside its control, the guest<->host communication is > unreliable (even for things like "cut and paste"), so every app that > cares has to implement a packet framing protocol with no binary data > (to reserve an escaping byte), or with CRCs like > PPP-over-virtio-serial, which is complicated and silly imho. If it > were a real serial port, not emulated, that's the sort of thing apps > would actually do (or use timeouts, which are more dubious in > emulator-land). But I hope we're not that sadistic :-) I agree. So: ports have in-qemu users, they get guest_open / guest_close callbacks and get data which they can pass on to external apps. Looks like we're fine there? > *Inband* open/close indication aren't 100% guarantees of reliability, > but I think they raise it to the point where an app can usefully count > on it. Amit
On 03/26/2010 09:29 AM, Luiz Capitulino wrote: > On Fri, 26 Mar 2010 18:56:20 +0530 > Amit Shah<amit.shah@redhat.com> wrote: > > >> On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote: >> >>>>>> + >>>>>> +VIRTIO_SERIAL >>>>>> +------------- >>>>>> >>>>> It should be VIRTIO_SERIAL_ADD. >>>>> >>>> What about other events that VIRTIO_SERIAL generates? >>>> >>> We don't address this problem currently, maybe an integration with qdev >>> will do, but I have to think more about it. >>> >> So should I just keep it as VIRTIO_SERIAL for now? With new events also >> riding on this one? >> > I don't like this because with the current events code this will lead > to confusion, as you're using a single event to notify different things. > > My suggestion for the immediate term is to do what we have been doing so > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way > to group events which requires a new VIRTIO_SERIAL event, in this case we > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The > latter would be deprecated too. > > Or, if you can wait I can _try_ to solve this problem next week, although > I have no idea how hard this is going to be. > > Any comments, Anthony? > The virtio serial events bother me in a number of ways. Ports being added should just be more generic (we should generate an event any time a device is added to a bus). Port disconnected/reconnect ought to be something that we propagate via a char device but I understand the limitations of the current code. I'd like to see us try to at least address the add/remove case better and we may just have to live with the connect/disconnect stuff. Regards, Anthony Liguori
On Fri, 26 Mar 2010 20:13:09 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) Mar 26 2010 [11:29:03], Luiz Capitulino wrote: > > On Fri, 26 Mar 2010 18:56:20 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > On (Fri) Mar 26 2010 [10:14:02], Luiz Capitulino wrote: > > > > > > > + > > > > > > > +VIRTIO_SERIAL > > > > > > > +------------- > > > > > > > > > > > > It should be VIRTIO_SERIAL_ADD. > > > > > > > > > > What about other events that VIRTIO_SERIAL generates? > > > > > > > > We don't address this problem currently, maybe an integration with qdev > > > > will do, but I have to think more about it. > > > > > > So should I just keep it as VIRTIO_SERIAL for now? With new events also > > > riding on this one? > > > > I don't like this because with the current events code this will lead > > to confusion, as you're using a single event to notify different things. > > > > My suggestion for the immediate term is to do what we have been doing so > > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way > > to group events which requires a new VIRTIO_SERIAL event, in this case we > > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The > > latter would be deprecated too. > > I've no problem doing it either way - whatever you prefer is fine. > > BTW these are two distinct events already - failure in initialising a > device and failure in initialising a port. Do you think these should be > separate as well? That depends on what you want clients to know/do about it. Can ports and devices be added and work independently of each other? Why is it relevant for a client to know that a _device_ has failed to initialize? If clients connect to a port and all they need to know is "Sorry, but that port won't be available", then you don't even need to have a port/device distinction in the event. Also note that events can be improved to include more information later, if needed. So, the best approach is to include as less information as possible (given that it satisfies current client needs, of course). > > Or, if you can wait I can _try_ to solve this problem next week, although > > I have no idea how hard this is going to be. > > I think it's cleaner to club everything; but basically I'll go with > whatever you say. I've no problem waiting. It's definitely needed to group events some way, we just have to find a good way to do it. Having each subsystem doing it its own way is not what we want because of protocol consistency and related problems.
On (Fri) Mar 26 2010 [14:52:36], Luiz Capitulino wrote: > > > My suggestion for the immediate term is to do what we have been doing so > > > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way > > > to group events which requires a new VIRTIO_SERIAL event, in this case we > > > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The > > > latter would be deprecated too. > > > > I've no problem doing it either way - whatever you prefer is fine. > > > > BTW these are two distinct events already - failure in initialising a > > device and failure in initialising a port. Do you think these should be > > separate as well? > > That depends on what you want clients to know/do about it. > > Can ports and devices be added and work independently of each other? > Why is it relevant for a client to know that a _device_ has failed to > initialize? I'm not sure what you mean by a client, but let's say libvirt handles the qemu session. A single device can have multiple ports. If a device fails to register *in the guest*, all the ports associated with that device could be hot-unplugged on the host to reduce host resource usage. If just a single port fails to register *in the guest*, libvirt may just want to hot-unplug it to free up resources. So yes, I think both are necessary. Anyway, I guess the answer is to split both these events. > If clients connect to a port and all they need to know is "Sorry, but > that port won't be available", then you don't even need to have a port/device > distinction in the event. > > Also note that events can be improved to include more information later, > if needed. So, the best approach is to include as less information as > possible (given that it satisfies current client needs, of course). Right; that's the reason only the failing port number is given right now. > > > Or, if you can wait I can _try_ to solve this problem next week, although > > > I have no idea how hard this is going to be. > > > > I think it's cleaner to club everything; but basically I'll go with > > whatever you say. I've no problem waiting. > > It's definitely needed to group events some way, we just have to > find a good way to do it. Having each subsystem doing it its own way > is not what we want because of protocol consistency and related > problems. Yes, that's exactly why I think waiting till you iron it out would help. Amit
Amit Shah wrote: > > Without this specific thing, which is an indicator that guest has lost > > state outside its control, the guest<->host communication is > > unreliable (even for things like "cut and paste"), so every app that > > cares has to implement a packet framing protocol with no binary data > > (to reserve an escaping byte), or with CRCs like > > PPP-over-virtio-serial, which is complicated and silly imho. If it > > were a real serial port, not emulated, that's the sort of thing apps > > would actually do (or use timeouts, which are more dubious in > > emulator-land). But I hope we're not that sadistic :-) > > I agree. So: ports have in-qemu users, they get guest_open / > guest_close callbacks and get data which they can pass on to external > apps. Looks like we're fine there? Provided the guest_open / guest_close callbacks are synchronous with the data - so that data sent/received following guest_open exactly matches what the guest sends/receives from its beginning, that internal interface looks fine to me. We can tidy up the chardev later as needed :-) -- Jamie > > *Inband* open/close indication aren't 100% guarantees of reliability, > > but I think they raise it to the point where an app can usefully count > > on it.
On Sat, 27 Mar 2010 13:33:22 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Fri) Mar 26 2010 [14:52:36], Luiz Capitulino wrote: > > > > My suggestion for the immediate term is to do what we have been doing so > > > > far, ie. call it VIRTIO_SERIAL_ADD. Worst case here is: we add a new way > > > > to group events which requires a new VIRTIO_SERIAL event, in this case we > > > > could emit both, the new VIRTIO_SERIAL and the old VIRTIO_SERIAL_ADD. The > > > > latter would be deprecated too. > > > > > > I've no problem doing it either way - whatever you prefer is fine. > > > > > > BTW these are two distinct events already - failure in initialising a > > > device and failure in initialising a port. Do you think these should be > > > separate as well? > > > > That depends on what you want clients to know/do about it. > > > > Can ports and devices be added and work independently of each other? > > Why is it relevant for a client to know that a _device_ has failed to > > initialize? > > I'm not sure what you mean by a client, but let's say libvirt handles > the qemu session. Client is any application talking to QEMU through QMP. > A single device can have multiple ports. If a device > fails to register *in the guest*, all the ports associated with that > device could be hot-unplugged on the host to reduce host resource usage. > > If just a single port fails to register *in the guest*, libvirt may just > want to hot-unplug it to free up resources. > > So yes, I think both are necessary. > > Anyway, I guess the answer is to split both these events. Yes, that makes sense. [...] > > > > Or, if you can wait I can _try_ to solve this problem next week, although > > > > I have no idea how hard this is going to be. > > > > > > I think it's cleaner to club everything; but basically I'll go with > > > whatever you say. I've no problem waiting. > > > > It's definitely needed to group events some way, we just have to > > find a good way to do it. Having each subsystem doing it its own way > > is not what we want because of protocol consistency and related > > problems. > > Yes, that's exactly why I think waiting till you iron it out would help. Ok.
diff --git a/QMP/qmp-events.txt b/QMP/qmp-events.txt index a94e9b4..f13cf45 100644 --- a/QMP/qmp-events.txt +++ b/QMP/qmp-events.txt @@ -188,3 +188,51 @@ Example: Note: If action is "reset", "shutdown", or "pause" the WATCHDOG event is followed respectively by the RESET, SHUTDOWN, or STOP events. + +VIRTIO_SERIAL +------------- + +Emitted when errors occur in guest port add or guest device add. + +Data: + +- "device": The virtio-serial device that triggered the event {json-string} + This is the name given to the bus on the command line: + -device virtio-serial,id="foo" + here, the device name is "foo" + +- "port": The port number that triggered the event {json-number} + This is the number given to the port on the command line: + -device virtserialport,nr=2 + here, the port number is 2. If not mentioned on the command + line, the number is auto-assigned. + +- "result": The result of the operation {json-string} + This is one of the following: + "pass", "fail" + +- "operation": The operation that triggered the event {json-sring} + This is one of the following: + "port_add", "device_add" + +Example: + +Port 0 add failure in the guest: + +{ "timestamp": {"seconds": 1269438649, "microseconds": 851170}, + "event": "VIRTIO_SERIAL", + "data": { + "device": "virtio-serial-bus.0", + "port": 0, + "result": "fail", + "operation": "port_add" } } + +Device add failure in the guest: + +{ "timestamp": {"seconds": 1269438702, "microseconds": 78114}, + "event": "VIRTIO_SERIAL", + "data": { + "device": "virtio-serial-bus.0", + "port": 4294967295, + "result": "fail", + "operation": "device_add" } } diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c index 33083af..efcc66c 100644 --- a/hw/virtio-serial-bus.c +++ b/hw/virtio-serial-bus.c @@ -16,6 +16,7 @@ */ #include "monitor.h" +#include "qemu-objects.h" #include "qemu-queue.h" #include "sysbus.h" #include "virtio-serial.h" @@ -204,6 +205,17 @@ size_t virtio_serial_guest_ready(VirtIOSerialPort *port) return 0; } +static void mon_event(const char *device, const uint32_t port_id, + const char *operation, const char *result) +{ + QObject *data; + + data = qobject_from_jsonf("{ 'device': %s, 'port': %ld, 'operation': %s, 'result': %s }", + device, (int64_t)port_id, operation, result); + monitor_protocol_event(QEVENT_VIRTIO_SERIAL, data); + qobject_decref(data); +} + /* Guest wants to notify us of some event */ static void handle_control_message(VirtIOSerial *vser, void *buf) { @@ -226,6 +238,8 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) if (!cpkt.value) { error_report("virtio-serial-bus: Guest failure in adding device %s\n", vser->bus->qbus.name); + mon_event(vser->bus->qbus.name, VIRTIO_CONSOLE_BAD_ID, + "device_add", "fail"); break; } /* @@ -241,6 +255,7 @@ static void handle_control_message(VirtIOSerial *vser, void *buf) if (!cpkt.value) { error_report("virtio-serial-bus: Guest failure in adding port %u for device %s\n", port->id, vser->bus->qbus.name); + mon_event(vser->bus->qbus.name, port->id, "port_add", "fail"); break; } /* diff --git a/monitor.c b/monitor.c index 0448a70..9e5bfe7 100644 --- a/monitor.c +++ b/monitor.c @@ -441,6 +441,9 @@ void monitor_protocol_event(MonitorEvent event, QObject *data) case QEVENT_WATCHDOG: event_name = "WATCHDOG"; break; + case QEVENT_VIRTIO_SERIAL: + event_name = "VIRTIO_SERIAL"; + break; default: abort(); break; diff --git a/monitor.h b/monitor.h index bd4ae34..ea4df8a 100644 --- a/monitor.h +++ b/monitor.h @@ -28,6 +28,7 @@ typedef enum MonitorEvent { QEVENT_BLOCK_IO_ERROR, QEVENT_RTC_CHANGE, QEVENT_WATCHDOG, + QEVENT_VIRTIO_SERIAL, QEVENT_MAX, } MonitorEvent;
When adding a port or a device to the guest fails, management software might be interested in knowing and then cleaning up the host-side of the port. Introduce QMP events to signal such errors. Signed-off-by: Amit Shah <amit.shah@redhat.com> CC: Luiz Capitulino <lcapitulino@redhat.com> --- QMP/qmp-events.txt | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ hw/virtio-serial-bus.c | 15 +++++++++++++++ monitor.c | 3 +++ monitor.h | 1 + 4 files changed, 67 insertions(+), 0 deletions(-)