| Message ID | 1998763220.603051280770128553.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 08/02/2010 12:28 PM, Alon Levy wrote: > ----- "Anthony Liguori"<anthony@codemonkey.ws> wrote: > > >> On 08/02/2010 03:33 AM, Alon Levy wrote: >> >>> Hi, >>> >>> This patch adds three CHR_IOCTLs and uses them in virtserial >>> >> devices, to be used >> >>> by a chardev backend, such as a spice vm channel (spice is a vdi >>> >> solution). >> >>> Basically virtio-serial provides three driver initiated events for >>> >> guest open of >> >>> a device, guest close, and guest ready (driver port init complete) >>> >> that before this >> >>> patch are not exposed to the chardev backend. >>> >>> With the spicevmc backend this is used like this: >>> qemu -chardev spicevmc,id=vdiport,name=vdiport -device >>> >> virtserialport,chardev=vdiport,name=com.redhat.spice.0 >> >>> I'd appreciate any feedback if this seems the right way to >>> >> accomplish this, and >> >>> for the numbers I grabbed. >>> >>> >> I really hate to add connection semantics via IOCTLs. I would rather >> we >> add them as first class semantics to the char device layer. This >> would >> allow us to use char devices for VNC, for instance. >> >> > Ok, that's actually what I wanted to do at first, how about: > My main objection to ioctls is that you change states based on event delivery. This results in weird things like what happens when you do a chr_write while not ready or not connected. So what I'd rather see is a move to an API that was connection oriented. For instance, we could treat CharDriverState as an established connection. So something like: typedef struct CharServerState { int backlog; /* max simultaneous connections; -1 for unlimited */ void (*connect)(CharServerState *s, CharDriverState *session); void (*disconnect)(CharServerState *s, CharDriverState *session); } CharDriverState; Obviously, more thought is needed but I hope the point comes across. We should be able to reflect the connect/disconnect semantics with an object that has a life cycle that matches the session instead of forcing each user to keep track of the session's life cycle. Regards, Anthony Liguori > diff --git a/qemu-char.h b/qemu-char.h > index 1df53ae..22973cd 100644 > --- a/qemu-char.h > +++ b/qemu-char.h > @@ -16,6 +16,8 @@ > #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */ > #define CHR_EVENT_CLOSED 5 /* connection closed */ > > +#define CHR_DEVICE_EVENT_OPENED 0 > +#define CHR_DEVICE_EVENT_CLOSED 1 > > #define CHR_IOCTL_SERIAL_SET_PARAMS 1 > typedef struct { > @@ -59,6 +61,7 @@ struct CharDriverState { > int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len); > void (*chr_update_read_handler)(struct CharDriverState *s); > int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg); > + int (*chr_device_event)(struct CharDriverState *s, int cmd, void *arg); > int (*get_msgfd)(struct CharDriverState *s); > IOEventHandler *chr_event; > IOCanReadHandler *chr_can_read; > @@ -83,6 +86,7 @@ void qemu_chr_close(CharDriverState *chr); > void qemu_chr_printf(CharDriverState *s, const char *fmt, ...); > int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len); > void qemu_chr_send_event(CharDriverState *s, int event); > +void qemu_chr_send_device_event(CharDriverState *s, int event, void *arg); > void qemu_chr_add_handlers(CharDriverState *s, > IOCanReadHandler *fd_can_read, > IOReadHandler *fd_read, > > > >> Regards, >> >> Anthony Liguori >> >> >>> Alon >>> >>> -------------- commit message -------------------------------- >>> From a90d4e26df727ed0d2b64b705e955f695289fa61 Mon Sep 17 00:00:00 >>> >> 2001 >> >>> From: Alon Levy<alevy@redhat.com> >>> Date: Mon, 2 Aug 2010 11:22:58 +0300 >>> Subject: [PATCH] virtio-console: add IOCTL's for >>> >> guest_{ready,open,close} >> >>> Add three IOCTL corresponding to the three control events of: >>> guest_ready -> CHR_IOCTL_VIRT_SERIAL_READY >>> guest_open -> CHR_IOCTL_VIRT_SERIAL_OPEN >>> guest_close -> CHR_IOCTL_VIRT_SERIAL_CLOSE >>> >>> Can be used by a matching backend. >>> --- >>> hw/virtio-console.c | 33 +++++++++++++++++++++++++++++++++ >>> qemu-char.h | 4 ++++ >>> 2 files changed, 37 insertions(+), 0 deletions(-) >>> >>> diff --git a/hw/virtio-console.c b/hw/virtio-console.c >>> index caea11f..4c3686d 100644 >>> --- a/hw/virtio-console.c >>> +++ b/hw/virtio-console.c >>> @@ -58,6 +58,33 @@ static void chr_event(void *opaque, int event) >>> } >>> } >>> >>> +static void virtconsole_guest_open(VirtIOSerialPort *port) >>> +{ >>> + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); >>> + >>> + if (vcon->chr) { >>> + qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_OPEN, >>> >> NULL); >> >>> + } >>> +} >>> + >>> +static void virtconsole_guest_close(VirtIOSerialPort *port) >>> +{ >>> + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); >>> + >>> + if (vcon->chr) { >>> + qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_CLOSE, >>> >> NULL); >> >>> + } >>> +} >>> + >>> +static void virtconsole_guest_ready(VirtIOSerialPort *port) >>> +{ >>> + VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); >>> + >>> + if (vcon->chr) { >>> + qemu_chr_ioctl(vcon->chr, CHR_IOCTL_VIRT_SERIAL_READY, >>> >> NULL); >> >>> + } >>> +} >>> + >>> /* Virtio Console Ports */ >>> static int virtconsole_initfn(VirtIOSerialDevice *dev) >>> { >>> @@ -94,6 +121,9 @@ static VirtIOSerialPortInfo virtconsole_info = { >>> .qdev.size = sizeof(VirtConsole), >>> .init = virtconsole_initfn, >>> .exit = virtconsole_exitfn, >>> + .guest_open = virtconsole_guest_open, >>> + .guest_close = virtconsole_guest_close, >>> + .guest_ready = virtconsole_guest_ready, >>> .qdev.props = (Property[]) { >>> DEFINE_PROP_UINT8("is_console", VirtConsole, >>> >> port.is_console, 1), >> >>> DEFINE_PROP_UINT32("nr", VirtConsole, port.id, >>> >> VIRTIO_CONSOLE_BAD_ID), >> >>> @@ -130,6 +160,9 @@ static VirtIOSerialPortInfo virtserialport_info >>> >> = { >> >>> .qdev.size = sizeof(VirtConsole), >>> .init = virtserialport_initfn, >>> .exit = virtconsole_exitfn, >>> + .guest_open = virtconsole_guest_open, >>> + .guest_close = virtconsole_guest_close, >>> + .guest_ready = virtconsole_guest_ready, >>> .qdev.props = (Property[]) { >>> DEFINE_PROP_UINT32("nr", VirtConsole, port.id, >>> >> VIRTIO_CONSOLE_BAD_ID), >> >>> DEFINE_PROP_CHR("chardev", VirtConsole, chr), >>> diff --git a/qemu-char.h b/qemu-char.h >>> index e3a0783..1df53ae 100644 >>> --- a/qemu-char.h >>> +++ b/qemu-char.h >>> @@ -41,6 +41,10 @@ typedef struct { >>> #define CHR_IOCTL_SERIAL_SET_TIOCM 13 >>> #define CHR_IOCTL_SERIAL_GET_TIOCM 14 >>> >>> +#define CHR_IOCTL_VIRT_SERIAL_OPEN 15 >>> +#define CHR_IOCTL_VIRT_SERIAL_CLOSE 16 >>> +#define CHR_IOCTL_VIRT_SERIAL_READY 17 >>> + >>> #define CHR_TIOCM_CTS 0x020 >>> #define CHR_TIOCM_CAR 0x040 >>> #define CHR_TIOCM_DSR 0x100 >>> >>>
Hi, > My main objection to ioctls is that you change states based on event > delivery. This results in weird things like what happens when you do a > chr_write while not ready or not connected. > > So what I'd rather see is a move to an API that was connection oriented. > For instance, we could treat CharDriverState as an established > connection. So something like: > > typedef struct CharServerState > { > int backlog; /* max simultaneous connections; -1 for unlimited */ > void (*connect)(CharServerState *s, CharDriverState *session); > void (*disconnect)(CharServerState *s, CharDriverState *session); > } CharDriverState; Oh, that is a similar but unrelated issue. We have open/close events on the *guest* side (i.e. some process inside the guests opens/closes /dev/vmchannel/org.qemu.foo.42). This is what Alon wants to propagate from the device backend to the chardev. We also have open/close (or connect/disconnect) events on the *host* side for the devices (or sockets) the chardevs are bound to. This is what you are talking about. Note that we already have events (CHR_EVENT_OPENED,CLOSED) for the host side. Adding events for the guest side open/close events makes sense to me (and is certainly better than the ioctl patch). cheers, Gerd
On 08/03/2010 03:46 AM, Gerd Hoffmann wrote: > Hi, > >> My main objection to ioctls is that you change states based on event >> delivery. This results in weird things like what happens when you do a >> chr_write while not ready or not connected. >> >> So what I'd rather see is a move to an API that was connection oriented. >> For instance, we could treat CharDriverState as an established >> connection. So something like: >> >> typedef struct CharServerState >> { >> int backlog; /* max simultaneous connections; -1 for unlimited */ >> void (*connect)(CharServerState *s, CharDriverState *session); >> void (*disconnect)(CharServerState *s, CharDriverState *session); >> } CharDriverState; > > Oh, that is a similar but unrelated issue. > > We have open/close events on the *guest* side (i.e. some process > inside the guests opens/closes /dev/vmchannel/org.qemu.foo.42). This > is what Alon wants to propagate from the device backend to the chardev. > > We also have open/close (or connect/disconnect) events on the *host* > side for the devices (or sockets) the chardevs are bound to. This is > what you are talking about. No, I'm not. You have a front-end device that's connected to virtio-serial. You're implementing the backend in spice. The front-end needs to communicate to the backend events like connect, ready, disconnect. I don't see what the difference between connect and ready is so I'll ignore it for now. The proposal is to implement this via events. My concern is that this interface is brittle because it leaves a lot of behavior undefined. There are three distinct states in the life cycle, DISCONNECTED, CONNECTED_BUT_NOT_READY, and CONNECTED_AND_READY. The entire CharDriverState interface is only useful in the CONNECTED_AND_READY state so what's the behavior of every function in any of the other states? My suggestion is to implement a simple CharServerState driver. This interface is connection oriented. You can have a dummy CharServerState that returns a single CharDriverState on connect() and does nothing on disconnect(). That's how you bridge virtio-serial to what we have today. But the idea is that virtio-serial no longer takes a CharDriverState but a CharServerState. Spice would then implement it's own CharServerState and would use it to understand what state the session is in. It's a really simple interface yet it makes the code much more robust because it eliminates the entire class of errors associated with undefined behavior when state != CONNECTED_AND_READY. The problem we've had with host side state is poorly defined semantics. For instance, I still think we generate multiple OPENED events as opposed to strictly generating CLOSED, followed by OPENED, followed by CLOSED. > Note that we already have events (CHR_EVENT_OPENED,CLOSED) for the > host side. Adding events for the guest side open/close events makes > sense to me (and is certainly better than the ioctl patch). We have the same problem with host side events today but it's even worse because the semantics are very subtle. Ultimately we need something like CharServerState and we could probably even use it but that's a larger scope than just this patch. The reason I think it's worth doing it this way is that I anticipate future virtio-serial backends in QEMU. It's a very simple difference too. Regards, Anthony Liguori > > cheers, > Gerd >
On 08/03/10 15:12, Anthony Liguori wrote: > On 08/03/2010 03:46 AM, Gerd Hoffmann wrote: >> Hi, >> >>> My main objection to ioctls is that you change states based on event >>> delivery. This results in weird things like what happens when you do a >>> chr_write while not ready or not connected. >>> >>> So what I'd rather see is a move to an API that was connection oriented. >>> For instance, we could treat CharDriverState as an established >>> connection. So something like: >>> >>> typedef struct CharServerState >>> { >>> int backlog; /* max simultaneous connections; -1 for unlimited */ >>> void (*connect)(CharServerState *s, CharDriverState *session); >>> void (*disconnect)(CharServerState *s, CharDriverState *session); >>> } CharDriverState; >> >> Oh, that is a similar but unrelated issue. >> >> We have open/close events on the *guest* side (i.e. some process >> inside the guests opens/closes /dev/vmchannel/org.qemu.foo.42). This >> is what Alon wants to propagate from the device backend to the chardev. >> >> We also have open/close (or connect/disconnect) events on the *host* >> side for the devices (or sockets) the chardevs are bound to. This is >> what you are talking about. > > No, I'm not. /me wonders what the point of the 'backlog' struct element is then. That made me think this is intended for the host side as it would be useful there. Monitor sockets could allow more than one connect then. > You have a front-end device that's connected to > virtio-serial. You're implementing the backend in spice. The front-end > needs to communicate to the backend events like connect, ready, > disconnect. That happens already. Guest opens device, virtio-serial receives a control message and calls port->info->guest_open(). Likewise on close. > The proposal is to implement this via events. Basically forwarding the events virtio-serial provides to the linked chardev, yes. > My concern is that this > interface is brittle because it leaves a lot of behavior undefined. > There are three distinct states in the life cycle, DISCONNECTED, > CONNECTED_BUT_NOT_READY, and CONNECTED_AND_READY. The entire > CharDriverState interface is only useful in the CONNECTED_AND_READY > state so what's the behavior of every function in any of the other states? Most chardev backends don't care anyway. In case they do it is up to them to define behavior when closed IMHO. > My suggestion is to implement a simple CharServerState driver. This > interface is connection oriented. You can have a dummy CharServerState > that returns a single CharDriverState on connect() and does nothing on > disconnect(). That's how you bridge virtio-serial to what we have today. > But the idea is that virtio-serial no longer takes a CharDriverState but > a CharServerState. Yes, we can do that. I don't think it is useful. Oh, and it also changes the command line interface. Todays ... qemu -chardev soemthing,id=foo \ -device virtserport,chardev=foo ... would turn into something like ... qemu -chardev something,if=foo \ -charsrv simple,chardev=foo,id=bar \ -device virtserport,charsrv=bar > Spice would then implement it's own CharServerState and would use it to > understand what state the session is in. Spice would basically (ab-)use it as event delivery mechanism. > It's a really simple interface > yet it makes the code much more robust because it eliminates the entire > class of errors associated with undefined behavior when state != > CONNECTED_AND_READY. Well. I disagree. Checking the state is needed nevertheless. The places where virtio-serial checks port->state today it would have to check whenever port->chardev is non-NULL then. The only difference is that failures to do so might become a bit more obvious as qemu will segfault due to the NULL pointer dereferences then. I still think this isn't worth the effort though. > The problem we've had with host side state is poorly defined semantics. > For instance, I still think we generate multiple OPENED events as > opposed to strictly generating CLOSED, followed by OPENED, followed by > CLOSED. Lets add assert()s (after 0.13-release) to catch those cases. cheers, Gerd
On 08/03/2010 10:28 AM, Gerd Hoffmann wrote: > On 08/03/10 15:12, Anthony Liguori wrote: >> On 08/03/2010 03:46 AM, Gerd Hoffmann wrote: >>> Hi, >>> >>>> My main objection to ioctls is that you change states based on event >>>> delivery. This results in weird things like what happens when you do a >>>> chr_write while not ready or not connected. >>>> >>>> So what I'd rather see is a move to an API that was connection >>>> oriented. >>>> For instance, we could treat CharDriverState as an established >>>> connection. So something like: >>>> >>>> typedef struct CharServerState >>>> { >>>> int backlog; /* max simultaneous connections; -1 for unlimited */ >>>> void (*connect)(CharServerState *s, CharDriverState *session); >>>> void (*disconnect)(CharServerState *s, CharDriverState *session); >>>> } CharDriverState; >>> >>> Oh, that is a similar but unrelated issue. >>> >>> We have open/close events on the *guest* side (i.e. some process >>> inside the guests opens/closes /dev/vmchannel/org.qemu.foo.42). This >>> is what Alon wants to propagate from the device backend to the chardev. >>> >>> We also have open/close (or connect/disconnect) events on the *host* >>> side for the devices (or sockets) the chardevs are bound to. This is >>> what you are talking about. >> >> No, I'm not. > > /me wonders what the point of the 'backlog' struct element is then. Because it could be used for host event but let's ignore that for now. >> You have a front-end device that's connected to >> virtio-serial. You're implementing the backend in spice. The front-end >> needs to communicate to the backend events like connect, ready, >> disconnect. > > That happens already. Guest opens device, virtio-serial receives a > control message and calls port->info->guest_open(). Likewise on close. > >> The proposal is to implement this via events. > > Basically forwarding the events virtio-serial provides to the linked > chardev, yes. > >> My concern is that this >> interface is brittle because it leaves a lot of behavior undefined. >> There are three distinct states in the life cycle, DISCONNECTED, >> CONNECTED_BUT_NOT_READY, and CONNECTED_AND_READY. The entire >> CharDriverState interface is only useful in the CONNECTED_AND_READY >> state so what's the behavior of every function in any of the other >> states? > > Most chardev backends don't care anyway. > In case they do it is up to them to define behavior when closed IMHO. But is there really any reasonable way to define it? Wouldn't it be better to just prevent the operations in the first place and basically push back to the front-ends? >> My suggestion is to implement a simple CharServerState driver. This >> interface is connection oriented. You can have a dummy CharServerState >> that returns a single CharDriverState on connect() and does nothing on >> disconnect(). That's how you bridge virtio-serial to what we have today. >> But the idea is that virtio-serial no longer takes a CharDriverState but >> a CharServerState. > > Yes, we can do that. I don't think it is useful. Oh, and it also > changes the command line interface. Todays ... > > qemu -chardev soemthing,id=foo \ > -device virtserport,chardev=foo > > ... would turn into something like ... > > qemu -chardev something,if=foo \ > -charsrv simple,chardev=foo,id=bar \ > -device virtserport,charsrv=bar If we think this is useful, then we can find a way to make the command line syntax work. If we don't think it's useful, then there's no point in doing it. >> Spice would then implement it's own CharServerState and would use it to >> understand what state the session is in. > > Spice would basically (ab-)use it as event delivery mechanism. Can you explain what spice uses these events for? >> It's a really simple interface >> yet it makes the code much more robust because it eliminates the entire >> class of errors associated with undefined behavior when state != >> CONNECTED_AND_READY. > > Well. I disagree. Checking the state is needed nevertheless. The > places where virtio-serial checks port->state today it would have to > check whenever port->chardev is non-NULL then. The only difference is > that failures to do so might become a bit more obvious as qemu will > segfault due to the NULL pointer dereferences then. I still think > this isn't worth the effort though. But I think we ultimately need to switch to having the front-ends having a NULL check. Even beyond front-end initiated connect/disconnect, front-end's need to learn to deal with back-end initiated disconnect/connect. If you look at something like the serial device's chardev usage, right now, it writes to the stream regardless of whether the back-end is connected and we have different semantics in each backend about what we do. It would be far better to just expose the fact that the backend isn't connected to the device such that it can either present that to the guest or make it's own decision about what to do. I think the model where we always write to a chardev is fundamentally broken. Sending life cycle events over an always open stream is even more broken and I think it's a good opportunity to introduce life cycle awareness into the API (especially since it can be done as a pretty small incremental change). Regards, Anthony Liguori >> The problem we've had with host side state is poorly defined semantics. >> For instance, I still think we generate multiple OPENED events as >> opposed to strictly generating CLOSED, followed by OPENED, followed by >> CLOSED. > > Lets add assert()s (after 0.13-release) to catch those cases. > > cheers, > Gerd >
Hi, >> /me wonders what the point of the 'backlog' struct element is then. > > Because it could be used for host event but let's ignore that for now. I doubt using the same beast for both host and guest is going to fly ... >> Yes, we can do that. I don't think it is useful. Oh, and it also >> changes the command line interface. Todays ... >> >> qemu -chardev soemthing,id=foo \ >> -device virtserport,chardev=foo >> >> ... would turn into something like ... >> >> qemu -chardev something,if=foo \ >> -charsrv simple,chardev=foo,id=bar \ >> -device virtserport,charsrv=bar > > If we think this is useful, then we can find a way to make the command > line syntax work. If we don't think it's useful, then there's no point > in doing it. Well, when using the new model everythere this is a moot point as -chardev would just define a CharServerState instead of a CharDriverState then. >>> Spice would then implement it's own CharServerState and would use it to >>> understand what state the session is in. >> >> Spice would basically (ab-)use it as event delivery mechanism. > > Can you explain what spice uses these events for? spice-vmc code registers/unregisters the interface within the spice server. So the interface is only activated in case the guest uses it. Spice client sees the interface being active or not and can act accordingly. http://cgit.freedesktop.org/spice/qemu/tree/hw/spice-vmc.c?h=spice.v13#n128 >> Well. I disagree. Checking the state is needed nevertheless. The >> places where virtio-serial checks port->state today it would have to >> check whenever port->chardev is non-NULL then. The only difference is >> that failures to do so might become a bit more obvious as qemu will >> segfault due to the NULL pointer dereferences then. I still think this >> isn't worth the effort though. > > But I think we ultimately need to switch to having the front-ends having > a NULL check. Even beyond front-end initiated connect/disconnect, > front-end's need to learn to deal with back-end initiated > disconnect/connect. I don't think we have to go with a NULL check. Providing chr_is_*() functions to query state and adding asserts() to the chr_*() function should provide the same level of robustness IMHO. Also having CharDriverStates come and go brings its own share of problems. > If you look at something like the serial device's chardev usage, right > now, it writes to the stream regardless of whether the back-end is > connected and we have different semantics in each backend about what we > do. It would be far better to just expose the fact that the backend > isn't connected to the device such that it can either present that to > the guest or make it's own decision about what to do. Indeed. > I think the model where we always write to a chardev is fundamentally > broken. Sending life cycle events over an always open stream is even > more broken and I think it's a good opportunity to introduce life cycle > awareness into the API (especially since it can be done as a pretty > small incremental change). The chardevs API can use a major overhaul, no question. I doubt that creating CharServerState helps here much though. Especially I don't see how that could be used for *both* Host and Guest side. cheers, Gerd
On 08/03/2010 11:42 AM, Gerd Hoffmann wrote: >>>> understand what state the session is in. >>> >>> Spice would basically (ab-)use it as event delivery mechanism. >> >> Can you explain what spice uses these events for? > Spice would then implement it's own CharServerState and would use it to > > spice-vmc code registers/unregisters the interface within the spice > server. So the interface is only activated in case the guest uses it. > Spice client sees the interface being active or not and can act > accordingly. So we have to migrate connected state? >>> Well. I disagree. Checking the state is needed nevertheless. The >>> places where virtio-serial checks port->state today it would have to >>> check whenever port->chardev is non-NULL then. The only difference is >>> that failures to do so might become a bit more obvious as qemu will >>> segfault due to the NULL pointer dereferences then. I still think this >>> isn't worth the effort though. >> >> But I think we ultimately need to switch to having the front-ends having >> a NULL check. Even beyond front-end initiated connect/disconnect, >> front-end's need to learn to deal with back-end initiated >> disconnect/connect. > > I don't think we have to go with a NULL check. Providing chr_is_*() > functions to query state and adding asserts() to the chr_*() function > should provide the same level of robustness IMHO. Also having > CharDriverStates come and go brings its own share of problems. I think this would be a reasonable solution too. Regards, Anthony Liguori
On 08/03/10 18:45, Anthony Liguori wrote: > On 08/03/2010 11:42 AM, Gerd Hoffmann wrote: >> spice-vmc code registers/unregisters the interface within the spice >> server. So the interface is only activated in case the guest uses it. >> Spice client sees the interface being active or not and can act >> accordingly. > > So we have to migrate connected state? virtio-serial handles that already. cheers, Gerd
On 08/03/2010 12:02 PM, Gerd Hoffmann wrote: > On 08/03/10 18:45, Anthony Liguori wrote: >> On 08/03/2010 11:42 AM, Gerd Hoffmann wrote: >>> spice-vmc code registers/unregisters the interface within the spice >>> server. So the interface is only activated in case the guest uses it. >>> Spice client sees the interface being active or not and can act >>> accordingly. >> >> So we have to migrate connected state? > > virtio-serial handles that already. And we have to propagate this upon load to the char device backend. Do we assume that the chardev is in the CONNECTED state initially? If we do a loadvm at run time while we're in the CONNECTED state, do we generate a DISCONNECTED followed by CONNECTED state transition? If we loadvm to a state where we were in the DISCONNECTED state, does that generate DISCONNECTED followed by CONNECTED followed by DISCONNECTED or just DISCONNECTED? This is exactly the type of problem that we've had in the past by not having an API that clearly forces management of life cycle. Regards, Anthony Liguori > > cheers, > Gerd >
On 08/03/10 19:53, Anthony Liguori wrote: > On 08/03/2010 12:02 PM, Gerd Hoffmann wrote: >> On 08/03/10 18:45, Anthony Liguori wrote: >>> On 08/03/2010 11:42 AM, Gerd Hoffmann wrote: >>>> spice-vmc code registers/unregisters the interface within the spice >>>> server. So the interface is only activated in case the guest uses it. >>>> Spice client sees the interface being active or not and can act >>>> accordingly. >>> >>> So we have to migrate connected state? >> >> virtio-serial handles that already. > > And we have to propagate this upon load to the char device backend. Happens too, by calling port->guest_open() if needed in loadvm_post(). > Do we assume that the chardev is in the CONNECTED state initially? If we > do a loadvm at run time while we're in the CONNECTED state, do we > generate a DISCONNECTED followed by CONNECTED state transition? If we > loadvm to a state where we were in the DISCONNECTED state, does that > generate DISCONNECTED followed by CONNECTED followed by DISCONNECTED or > just DISCONNECTED? > > This is exactly the type of problem that we've had in the past by not > having an API that clearly forces management of life cycle. That indeed must be cleanly defined and for best results also enforced in some way, say using asserts(). That needs some bigger planning and not some ad-hoc "Oh lets us quickly add this" patching though. When re-designing chardevs I wanna do it once and wanna do it right. Are there BoF slots @ kvm forum? I can try to put together something (current pain points + feature wishlist + better chardev data structures + better chardev api) to kick a discussion either in Boston or at qemu-devel. Will take some time though. cheers, Gerd
On 08/03/2010 03:41 PM, Gerd Hoffmann wrote: > On 08/03/10 19:53, Anthony Liguori wrote: >> On 08/03/2010 12:02 PM, Gerd Hoffmann wrote: >>> On 08/03/10 18:45, Anthony Liguori wrote: >>>> On 08/03/2010 11:42 AM, Gerd Hoffmann wrote: >>>>> spice-vmc code registers/unregisters the interface within the spice >>>>> server. So the interface is only activated in case the guest uses it. >>>>> Spice client sees the interface being active or not and can act >>>>> accordingly. >>>> >>>> So we have to migrate connected state? >>> >>> virtio-serial handles that already. >> >> And we have to propagate this upon load to the char device backend. > > Happens too, by calling port->guest_open() if needed in loadvm_post(). > >> Do we assume that the chardev is in the CONNECTED state initially? If we >> do a loadvm at run time while we're in the CONNECTED state, do we >> generate a DISCONNECTED followed by CONNECTED state transition? If we >> loadvm to a state where we were in the DISCONNECTED state, does that >> generate DISCONNECTED followed by CONNECTED followed by DISCONNECTED or >> just DISCONNECTED? >> >> This is exactly the type of problem that we've had in the past by not >> having an API that clearly forces management of life cycle. > > That indeed must be cleanly defined and for best results also enforced > in some way, say using asserts(). That needs some bigger planning and > not some ad-hoc "Oh lets us quickly add this" patching though. When > re-designing chardevs I wanna do it once and wanna do it right. > > Are there BoF slots @ kvm forum? Yes. > I can try to put together something (current pain points + feature > wishlist + better chardev data structures + better chardev api) to > kick a discussion either in Boston or at qemu-devel. Will take some > time though. That would be really helpful! Regards, Anthony Liguori > cheers, > Gerd >
diff --git a/qemu-char.h b/qemu-char.h index 1df53ae..22973cd 100644 --- a/qemu-char.h +++ b/qemu-char.h @@ -16,6 +16,8 @@ #define CHR_EVENT_MUX_OUT 4 /* mux-focus will move on */ #define CHR_EVENT_CLOSED 5 /* connection closed */ +#define CHR_DEVICE_EVENT_OPENED 0 +#define CHR_DEVICE_EVENT_CLOSED 1 #define CHR_IOCTL_SERIAL_SET_PARAMS 1 typedef struct { @@ -59,6 +61,7 @@ struct CharDriverState { int (*chr_write)(struct CharDriverState *s, const uint8_t *buf, int len); void (*chr_update_read_handler)(struct CharDriverState *s); int (*chr_ioctl)(struct CharDriverState *s, int cmd, void *arg); + int (*chr_device_event)(struct CharDriverState *s, int cmd, void *arg); int (*get_msgfd)(struct CharDriverState *s); IOEventHandler *chr_event; IOCanReadHandler *chr_can_read; @@ -83,6 +86,7 @@ void qemu_chr_close(CharDriverState *chr); void qemu_chr_printf(CharDriverState *s, const char *fmt, ...); int qemu_chr_write(CharDriverState *s, const uint8_t *buf, int len); void qemu_chr_send_event(CharDriverState *s, int event); +void qemu_chr_send_device_event(CharDriverState *s, int event, void *arg); void qemu_chr_add_handlers(CharDriverState *s, IOCanReadHandler *fd_can_read, IOReadHandler *fd_read,