| Message ID | 1363883716-30289-2-git-send-email-alevy@redhat.com |
|---|---|
| State | New |
| Headers | show |
Alon Levy <alevy@redhat.com> writes: > Note that the handler is called chr_is_guest_connected and not > chr_is_fe_connected, consistent with other members of CharDriverState. Sorry, I don't get it. There isn't a notion of "connected" for the front-ends in the char layer. The closest thing is whether add_handlers() have been called or not. I really dislike the idea of introduction a new concept to the char layer in a half baked way. Why can't migration notifiers be used for this? I think Gerd objected to using a migration *handler* but not necessarily a state notifier. Regards, Anthony Liguori > > Signed-off-by: Alon Levy <alevy@redhat.com> > --- > hw/virtio-console.c | 9 +++++++++ > include/char/char.h | 11 +++++++++++ > qemu-char.c | 9 +++++++++ > 3 files changed, 29 insertions(+) > > diff --git a/hw/virtio-console.c b/hw/virtio-console.c > index e2d1c58..643e24e 100644 > --- a/hw/virtio-console.c > +++ b/hw/virtio-console.c > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event) > } > } > > +static bool chr_is_guest_connected(void *opaque) > +{ > + VirtConsole *vcon = opaque; > + > + return vcon->port.guest_connected; > +} > + > static int virtconsole_initfn(VirtIOSerialPort *port) > { > VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port) > if (vcon->chr) { > qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, > vcon); > + /* only user of chr_is_guest_connected so leave it as special cased*/ > + vcon->chr->chr_is_guest_connected = chr_is_guest_connected; > } > > return 0; > diff --git a/include/char/char.h b/include/char/char.h > index 0326b2a..b41ddc0 100644 > --- a/include/char/char.h > +++ b/include/char/char.h > @@ -52,6 +52,7 @@ typedef struct { > #define CHR_TIOCM_RTS 0x004 > > typedef void IOEventHandler(void *opaque, int event); > +typedef bool IOIsGuestConnectedHandler(void *opaque); > > struct CharDriverState { > void (*init)(struct CharDriverState *s); > @@ -64,6 +65,7 @@ struct CharDriverState { > IOEventHandler *chr_event; > IOCanReadHandler *chr_can_read; > IOReadHandler *chr_read; > + IOIsGuestConnectedHandler *chr_is_guest_connected; > void *handler_opaque; > void (*chr_close)(struct CharDriverState *chr); > void (*chr_accept_input)(struct CharDriverState *chr); > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len); > */ > void qemu_chr_be_event(CharDriverState *s, int event); > > +/** > + * @qemu_chr_be_is_fe_connected: > + * > + * Back end calls this to check if the front end is connected. > + * > + * Returns: true if the guest (front end) is connected, false otherwise. > + */ > +bool qemu_chr_be_is_fe_connected(CharDriverState *s); > + > void qemu_chr_add_handlers(CharDriverState *s, > IOCanReadHandler *fd_can_read, > IOReadHandler *fd_read, > diff --git a/qemu-char.c b/qemu-char.c > index 4e011df..77a501a 100644 > --- a/qemu-char.c > +++ b/qemu-char.c > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int event) > s->chr_event(s->handler_opaque, event); > } > > +bool qemu_chr_be_is_fe_connected(CharDriverState *s) > +{ > + if (s->chr_is_guest_connected) { > + return s->chr_is_guest_connected(s->handler_opaque); > + } > + /* default to always connected */ > + return true; > +} > + > static gboolean qemu_chr_generic_open_bh(gpointer opaque) > { > CharDriverState *s = opaque; > -- > 1.8.1.4
> Alon Levy <alevy@redhat.com> writes: > > > Note that the handler is called chr_is_guest_connected and not > > chr_is_fe_connected, consistent with other members of > > CharDriverState. > > Sorry, I don't get it. > > There isn't a notion of "connected" for the front-ends in the char > layer. The closest thing is whether add_handlers() have been called > or > not. It makes sense for virtio-console - it matches exactly the internal guest_connected port state. > > I really dislike the idea of introduction a new concept to the char > layer in a half baked way. Is the fact there is only one user, virtio-console, the reason you call this half baked? > > Why can't migration notifiers be used for this? I think Gerd > objected > to using a migration *handler* but not necessarily a state notifier. Because if you have two chardevices, i.e. -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2 Then the two on-the-wire vmstates will be identical. > > Regards, > > Anthony Liguori > > > > > Signed-off-by: Alon Levy <alevy@redhat.com> > > --- > > hw/virtio-console.c | 9 +++++++++ > > include/char/char.h | 11 +++++++++++ > > qemu-char.c | 9 +++++++++ > > 3 files changed, 29 insertions(+) > > > > diff --git a/hw/virtio-console.c b/hw/virtio-console.c > > index e2d1c58..643e24e 100644 > > --- a/hw/virtio-console.c > > +++ b/hw/virtio-console.c > > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event) > > } > > } > > > > +static bool chr_is_guest_connected(void *opaque) > > +{ > > + VirtConsole *vcon = opaque; > > + > > + return vcon->port.guest_connected; > > +} > > + > > static int virtconsole_initfn(VirtIOSerialPort *port) > > { > > VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); > > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort > > *port) > > if (vcon->chr) { > > qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, > > chr_event, > > vcon); > > + /* only user of chr_is_guest_connected so leave it as > > special cased*/ > > + vcon->chr->chr_is_guest_connected = > > chr_is_guest_connected; > > } > > > > return 0; > > diff --git a/include/char/char.h b/include/char/char.h > > index 0326b2a..b41ddc0 100644 > > --- a/include/char/char.h > > +++ b/include/char/char.h > > @@ -52,6 +52,7 @@ typedef struct { > > #define CHR_TIOCM_RTS 0x004 > > > > typedef void IOEventHandler(void *opaque, int event); > > +typedef bool IOIsGuestConnectedHandler(void *opaque); > > > > struct CharDriverState { > > void (*init)(struct CharDriverState *s); > > @@ -64,6 +65,7 @@ struct CharDriverState { > > IOEventHandler *chr_event; > > IOCanReadHandler *chr_can_read; > > IOReadHandler *chr_read; > > + IOIsGuestConnectedHandler *chr_is_guest_connected; > > void *handler_opaque; > > void (*chr_close)(struct CharDriverState *chr); > > void (*chr_accept_input)(struct CharDriverState *chr); > > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, > > uint8_t *buf, int len); > > */ > > void qemu_chr_be_event(CharDriverState *s, int event); > > > > +/** > > + * @qemu_chr_be_is_fe_connected: > > + * > > + * Back end calls this to check if the front end is connected. > > + * > > + * Returns: true if the guest (front end) is connected, false > > otherwise. > > + */ > > +bool qemu_chr_be_is_fe_connected(CharDriverState *s); > > + > > void qemu_chr_add_handlers(CharDriverState *s, > > IOCanReadHandler *fd_can_read, > > IOReadHandler *fd_read, > > diff --git a/qemu-char.c b/qemu-char.c > > index 4e011df..77a501a 100644 > > --- a/qemu-char.c > > +++ b/qemu-char.c > > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int > > event) > > s->chr_event(s->handler_opaque, event); > > } > > > > +bool qemu_chr_be_is_fe_connected(CharDriverState *s) > > +{ > > + if (s->chr_is_guest_connected) { > > + return s->chr_is_guest_connected(s->handler_opaque); > > + } > > + /* default to always connected */ > > + return true; > > +} > > + > > static gboolean qemu_chr_generic_open_bh(gpointer opaque) > > { > > CharDriverState *s = opaque; > > -- > > 1.8.1.4 > > >
Alon Levy <alevy@redhat.com> writes: >> Alon Levy <alevy@redhat.com> writes: >> >> > Note that the handler is called chr_is_guest_connected and not >> > chr_is_fe_connected, consistent with other members of >> > CharDriverState. >> >> Sorry, I don't get it. >> >> There isn't a notion of "connected" for the front-ends in the char >> layer. The closest thing is whether add_handlers() have been called >> or >> not. > > It makes sense for virtio-console - it matches exactly the internal > guest_connected port state. I still don't understand why you need to know that detail in the backend. Hint: you should explain this in future commit messages/cover letters. >> I really dislike the idea of introduction a new concept to the char >> layer in a half baked way. > > Is the fact there is only one user, virtio-console, the reason you > call this half baked? You are introducing a function: qemu_chr_be_is_virtio_console_connnected() And calling pretending like it's a generic character device interface. It's not. If spicevmc only works with virtio-console and has no hope of working with anything else, don't use the character device layer! It's trying to fit a square peg into a round hole. If it's a concept that makes sense for all character devices front ends, then it should be a patch making it be a meaningful to all character device front end implementations. >> Why can't migration notifiers be used for this? I think Gerd >> objected >> to using a migration *handler* but not necessarily a state notifier. > > Because if you have two chardevices, i.e. > -chardev spicevmc,name=vdagent,id=spice1 -chardev spicevmc,name=vdagent,id=spice2 > > Then the two on-the-wire vmstates will be identical. I don't understand why this matters but I don't understand what the problem you're trying to solve is either so that's not surprising. Regards, Anthony Liguori > >> >> Regards, >> >> Anthony Liguori >> >> > >> > Signed-off-by: Alon Levy <alevy@redhat.com> >> > --- >> > hw/virtio-console.c | 9 +++++++++ >> > include/char/char.h | 11 +++++++++++ >> > qemu-char.c | 9 +++++++++ >> > 3 files changed, 29 insertions(+) >> > >> > diff --git a/hw/virtio-console.c b/hw/virtio-console.c >> > index e2d1c58..643e24e 100644 >> > --- a/hw/virtio-console.c >> > +++ b/hw/virtio-console.c >> > @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event) >> > } >> > } >> > >> > +static bool chr_is_guest_connected(void *opaque) >> > +{ >> > + VirtConsole *vcon = opaque; >> > + >> > + return vcon->port.guest_connected; >> > +} >> > + >> > static int virtconsole_initfn(VirtIOSerialPort *port) >> > { >> > VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); >> > @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort >> > *port) >> > if (vcon->chr) { >> > qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, >> > chr_event, >> > vcon); >> > + /* only user of chr_is_guest_connected so leave it as >> > special cased*/ >> > + vcon->chr->chr_is_guest_connected = >> > chr_is_guest_connected; >> > } >> > >> > return 0; >> > diff --git a/include/char/char.h b/include/char/char.h >> > index 0326b2a..b41ddc0 100644 >> > --- a/include/char/char.h >> > +++ b/include/char/char.h >> > @@ -52,6 +52,7 @@ typedef struct { >> > #define CHR_TIOCM_RTS 0x004 >> > >> > typedef void IOEventHandler(void *opaque, int event); >> > +typedef bool IOIsGuestConnectedHandler(void *opaque); >> > >> > struct CharDriverState { >> > void (*init)(struct CharDriverState *s); >> > @@ -64,6 +65,7 @@ struct CharDriverState { >> > IOEventHandler *chr_event; >> > IOCanReadHandler *chr_can_read; >> > IOReadHandler *chr_read; >> > + IOIsGuestConnectedHandler *chr_is_guest_connected; >> > void *handler_opaque; >> > void (*chr_close)(struct CharDriverState *chr); >> > void (*chr_accept_input)(struct CharDriverState *chr); >> > @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, >> > uint8_t *buf, int len); >> > */ >> > void qemu_chr_be_event(CharDriverState *s, int event); >> > >> > +/** >> > + * @qemu_chr_be_is_fe_connected: >> > + * >> > + * Back end calls this to check if the front end is connected. >> > + * >> > + * Returns: true if the guest (front end) is connected, false >> > otherwise. >> > + */ >> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s); >> > + >> > void qemu_chr_add_handlers(CharDriverState *s, >> > IOCanReadHandler *fd_can_read, >> > IOReadHandler *fd_read, >> > diff --git a/qemu-char.c b/qemu-char.c >> > index 4e011df..77a501a 100644 >> > --- a/qemu-char.c >> > +++ b/qemu-char.c >> > @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int >> > event) >> > s->chr_event(s->handler_opaque, event); >> > } >> > >> > +bool qemu_chr_be_is_fe_connected(CharDriverState *s) >> > +{ >> > + if (s->chr_is_guest_connected) { >> > + return s->chr_is_guest_connected(s->handler_opaque); >> > + } >> > + /* default to always connected */ >> > + return true; >> > +} >> > + >> > static gboolean qemu_chr_generic_open_bh(gpointer opaque) >> > { >> > CharDriverState *s = opaque; >> > -- >> > 1.8.1.4 >> >> >>
Hi, On 03/21/2013 07:18 PM, Anthony Liguori wrote: > Alon Levy <alevy@redhat.com> writes: > >> Note that the handler is called chr_is_guest_connected and not >> chr_is_fe_connected, consistent with other members of CharDriverState. > > Sorry, I don't get it. > > There isn't a notion of "connected" for the front-ends in the char > layer. And that is simply completely and utterly wrong. We've tried to explain this to you a number of times already. Yet you claim we've not explained it. Or we've not explained it properly. I must say however that I'm getting the feeling the problem is not us not explaining, but that you are not listening. Still let me try to explain it 1 more time, in 2 different ways: 1) At an abstract level a chardev fe + be is a pipe between somewhere and where-ever. Both ends of the pipe can be opened or closed, not just one. Frontends end inside the guest usually in the form of some piece of virtual hardware inside the guest. Just because the virtual hardware is there does not mean the guest has a driver, if the guest has a driver that creates a device-node for it, that does not mean there is a userspace process inside the guest which actually has the device-node open. So you see the front-end device-node inside the guest can be opened and closed. Most frontends cannot signal this to the backend but virtio-console can, and we want to know about it since we want to know if the user-space agent is there. 2) At the code level it clearly is there too, backend open-close is signalled to the frontend by means of the backend calling qemu_chr_be_event with an event of CHR_EVENT_OPENED and CHR_EVENT_CLOSED. Frontend open-close is signalled by the frontend calling qemu_chr_fe_open and qemu_chr_fe_close. Now the problem we have is that after migration the CHR_EVENT_OPENED gets replayed on the destination side but the qemu_chr_fe_open call does not. The logical place to replay the qemu_chr_fe_open would be in the frontend's migration handling code, as suggested here: http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html But Amit did not like this approach, suggesting that instead we took care of this inside spice-qemu-char.c. Which is what we're trying to do now, but this requires spice-qemu-char.c being able to query the open state of the frontend, which is already being migrate by the virtio-console code in the form of the guest_connected variable, we just need to be able to get to that variable from the backend. Regards, Hans
Hi, > There isn't a notion of "connected" for the front-ends in the char > layer. The closest thing is whether add_handlers() have been called or > not. It isn't new. There are qemu_chr_fe_open + qemu_chr_fe_close doing exactly that signaling (whenever someone has opened the virtio-serial port, i.e. whenever the guest agent is active or not). Problem is the chardev forgets this state over migration. So we need a way to restore the state. virtio-serial knows it of course as this is guest state it has to keep track of it. We just need a way the chardev can figure what the current state is after migration. The options I see: (1) Have chardev store the state itself in a new migration section. This is what I've NAK'ed. First, because live-migration host state is a can of worms I don't want open. Second because it actually isn't host state. (2) virtio-serial could just call qemu_chr_fe_open in post_migration hook. Could have unwanted side effects as the chardev can't figure whenever this is a post-load call or a guest-open call. (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the chardev. This was the next proposal from Alon (4) Do it the other way around: Allow the chardev query what the current state is. This patch series. (5) Cut off the chardev layer altogether and implement spicevmc as virtio-serial port. Your proposal if I understand it correctly. Makes sense, but breaks backward compatibility, so even when doing this we must fix the chardev spicevmc bug somehow. Hope this clarifies, Gerd
Hi, On 03/22/2013 09:25 AM, Gerd Hoffmann wrote: > Hi, > >> There isn't a notion of "connected" for the front-ends in the char >> layer. The closest thing is whether add_handlers() have been called or >> not. > > It isn't new. There are qemu_chr_fe_open + qemu_chr_fe_close doing > exactly that signaling (whenever someone has opened the virtio-serial > port, i.e. whenever the guest agent is active or not). > > Problem is the chardev forgets this state over migration. > > So we need a way to restore the state. virtio-serial knows it of course > as this is guest state it has to keep track of it. We just need a way > the chardev can figure what the current state is after migration. > > The options I see: > > (1) Have chardev store the state itself in a new migration section. > This is what I've NAK'ed. First, because live-migration host > state is a can of worms I don't want open. Second because it > actually isn't host state. > > (2) virtio-serial could just call qemu_chr_fe_open in > post_migration hook. Could have unwanted side effects as > the chardev can't figure whenever this is a post-load call > or a guest-open call. > > (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the > chardev. This was the next proposal from Alon > > (4) Do it the other way around: Allow the chardev query what the > current state is. This patch series. > Thanks for the above overview, very useful! > (5) Cut off the chardev layer altogether and implement spicevmc as > virtio-serial port. Your proposal if I understand it correctly. > Makes sense, but breaks backward compatibility, so even when > doing this we must fix the chardev spicevmc bug somehow. I've to nack this one, spicevmc is a chardev backend which is also used with other frontends then just virtio-console, atm it is also used together with the usb-redir and smartcard frontends and there is no reason why it could not be used with others in the future, so: NACK. Regards, Hans
Gerd Hoffmann <kraxel@redhat.com> writes: > Hi, > >> There isn't a notion of "connected" for the front-ends in the char >> layer. The closest thing is whether add_handlers() have been called or >> not. > > It isn't new. There are qemu_chr_fe_open + qemu_chr_fe_close doing > exactly that signaling (whenever someone has opened the virtio-serial > port, i.e. whenever the guest agent is active or not). anthony@titi:~/git/qemu$ find -name "*.[ch]" -exec grep -l qemu_chr_fe_open {} \; ./qemu-char.c ./hw/virtio-console.c ./hw/usb/redirect.c ./include/char/char.h And: anthony@titi:~/git/qemu$ find . -name "*.[ch]" -exec grep -l chr_guest_open {} \; ./qemu-char.c ./include/char/char.h ./spice-qemu-char.c So this whole interface exists specificially for the spice-qemu-char backend and only for that backend. This isn't a generic char layer concept. It's a thinly veiled call into spice. Now spice appears to have a work around in it where the first write will trigger affectively the same action as doing an open. Presumably this makes it usable with !virtio-serial or !usb-redir. If we want to extend this concept of fe open, let's do it right. Let's make all char frontends explicitly open, close on reset, etc. Let's do something that makes sense across the board. > > Problem is the chardev forgets this state over migration. > > So we need a way to restore the state. virtio-serial knows it of course > as this is guest state it has to keep track of it. We just need a way > the chardev can figure what the current state is after migration. > > The options I see: > > (1) Have chardev store the state itself in a new migration section. > This is what I've NAK'ed. First, because live-migration host > state is a can of worms I don't want open. Second because it > actually isn't host state. > > (2) virtio-serial could just call qemu_chr_fe_open in > post_migration hook. Could have unwanted side effects as > the chardev can't figure whenever this is a post-load call > or a guest-open call. The only implementation of qemu_chr_fe_open() is: static void spice_chr_guest_open(struct CharDriverState *chr) { SpiceCharDriver *s = chr->opaque; vmc_register_interface(s); } Is this what you would do anyway? In a world where qemu_chr_fe_open() was actually used throughout the device model, all devices would need to have a post_load() implementation where they checked their state to determine whether to generate an open() call. Regards, Anthony Liguori > > (3) Add a new qemu_chr_fe_* call for virtio-serial to notify the > chardev. This was the next proposal from Alon > > (4) Do it the other way around: Allow the chardev query what the > current state is. This patch series. > > (5) Cut off the chardev layer altogether and implement spicevmc as > virtio-serial port. Your proposal if I understand it correctly. > Makes sense, but breaks backward compatibility, so even when > doing this we must fix the chardev spicevmc bug somehow. > > Hope this clarifies, > Gerd
Hans de Goede <hdegoede@redhat.com> writes: > Hi, > > On 03/21/2013 07:18 PM, Anthony Liguori wrote: >> Alon Levy <alevy@redhat.com> writes: >> >>> Note that the handler is called chr_is_guest_connected and not >>> chr_is_fe_connected, consistent with other members of CharDriverState. >> >> Sorry, I don't get it. >> >> There isn't a notion of "connected" for the front-ends in the char >> layer. > > And that is simply completely and utterly wrong. We've tried to explain > this to you a number of times already. Yet you claim we've not explained > it. Or we've not explained it properly. I must say however that I'm > getting the feeling the problem is not us not explaining, but that you > are not listening. As usual, you make way too many assumptions without stopping to actually think about what I'm saying. > Still let me try to explain it 1 more time, in 2 different ways: > > 1) At an abstract level a chardev fe + be is a pipe between somewhere > and where-ever. Both ends of the pipe can be opened or closed, not just > one. No, this is not the case in reality. The notion of the front-end being open or closed only exists for virtio-serial, usb-redir, and spice-*. For every other aspect of the character device layer, the concept doesn't exist. We should have never allowed that in the first place and I object strongly to extending the concept without making it make sense for everything else. > Frontends end inside the guest usually in the form of some piece of > virtual hardware inside the guest. Just because the virtual hardware > is there does not mean the guest has a driver, Okay, let's use your example here with a standard UART. In the following sequence, I should receive: 1) Starts guest 2) When guest initializes the UART, qemu_chr_fe_open() 3) Reboot guest 4) Receive qemu_chr_fe_close() 5) Boot new guest without a UART driver 6) Nothing is received But this doesn't happen today because the only backend that cares (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write(). So it will certainly approximate steps 1-3 but will not behave correct with steps 4-6. Likewise, if you are dealing with something like a PCI hotpluggable UART and the card is ejected (but the CDS isn't deleted), you won't receive a close() event either. Even just in the world of spice-* which is all ya'll seem to care about, the behavior is broken today. So before we go adding more hacks just for virtio-serial/spice-*, let's consider how this applies to all character devices and do what makes sense for all of them. And for me, the most logical thing is to call qemu_chr_fe_open() in post_load for the device. Regards, Anthony Liguori > if the guest has > a driver that creates a device-node for it, that does not mean there > is a userspace process inside the guest which actually has the > device-node open. So you see the front-end device-node inside the guest > can be opened and closed. Most frontends cannot signal this to the > backend but virtio-console can, and we want to know about it since > we want to know if the user-space agent is there. > > 2) At the code level it clearly is there too, backend open-close > is signalled to the frontend by means of the backend calling > qemu_chr_be_event with an event of CHR_EVENT_OPENED and > CHR_EVENT_CLOSED. Frontend open-close is signalled by the > frontend calling qemu_chr_fe_open and qemu_chr_fe_close. > > Now the problem we have is that after migration the CHR_EVENT_OPENED > gets replayed on the destination side but the qemu_chr_fe_open call > does not. > > The logical place to replay the qemu_chr_fe_open would be in the > frontend's migration handling code, as suggested here: > http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html > > But Amit did not like this approach, suggesting that instead we > took care of this inside spice-qemu-char.c. Which is what we're > trying to do now, but this requires spice-qemu-char.c being > able to query the open state of the frontend, which is already > being migrate by the virtio-console code in the form of the > guest_connected variable, we just need to be able to get to that > variable from the backend. > > Regards, > > Hans
Hi, > Okay, let's use your example here with a standard UART. In the > following sequence, I should receive: > > 1) Starts guest > 2) When guest initializes the UART, qemu_chr_fe_open() > 3) Reboot guest > 4) Receive qemu_chr_fe_close() > 5) Boot new guest without a UART driver > 6) Nothing is received Well, with virtio-serial the logic is slightly different. qemu_chr_fe_open() is called when a process opens /dev/virtio-ports/$name, not when the virtio-serial driver loads. I'm not sure whenever the qemu uart emulation can reliable do the same. Guest loading the uart driver can probably detected without too much trouble. But detecting a process opening /dev/ttySx might not be possible. Depends on the guest driver implementation. For virtio-serial it is trivial, there is a control message for that ;) > And for me, the most logical thing is to call qemu_chr_fe_open() in > post_load for the device. Ok, just lets do that then. cheers, Gerd
Hi, On 03/22/2013 02:50 PM, Anthony Liguori wrote: > Hans de Goede <hdegoede@redhat.com> writes: > >> Hi, >> >> On 03/21/2013 07:18 PM, Anthony Liguori wrote: >>> Alon Levy <alevy@redhat.com> writes: >>> >>>> Note that the handler is called chr_is_guest_connected and not >>>> chr_is_fe_connected, consistent with other members of CharDriverState. >>> >>> Sorry, I don't get it. >>> >>> There isn't a notion of "connected" for the front-ends in the char >>> layer. >> >> And that is simply completely and utterly wrong. We've tried to explain >> this to you a number of times already. Yet you claim we've not explained >> it. Or we've not explained it properly. I must say however that I'm >> getting the feeling the problem is not us not explaining, but that you >> are not listening. > > As usual, you make way too many assumptions without stopping to actually > think about what I'm saying. > >> Still let me try to explain it 1 more time, in 2 different ways: >> >> 1) At an abstract level a chardev fe + be is a pipe between somewhere >> and where-ever. Both ends of the pipe can be opened or closed, not just >> one. > > No, this is not the case in reality. A pipe does not have 2 ends in reality ? > The notion of the front-end being > open or closed only exists for virtio-serial, usb-redir, and spice-*. > > For every other aspect of the character device layer, the concept > doesn't exist. The notion / concept of both ends of the pipe being opened and closed certainly does exist. See your own example of a plain 16x50 uart and the guest using it or not using it. Just because we cannot reliable / practically detect the open/close does not mean the concept of it is not there. > We should have never allowed that in the first place and > I object strongly to extending the concept without making it make sense > for everything else. > >> Frontends end inside the guest usually in the form of some piece of >> virtual hardware inside the guest. Just because the virtual hardware >> is there does not mean the guest has a driver, > > Okay, let's use your example here with a standard UART. In the > following sequence, I should receive: > > 1) Starts guest > 2) When guest initializes the UART, qemu_chr_fe_open() > 3) Reboot guest > 4) Receive qemu_chr_fe_close() > 5) Boot new guest without a UART driver > 6) Nothing is received > > But this doesn't happen today because the only backend that cares > (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write(). 1) If the guest does not have an uart driver, nothing will be written to the uart, so it wont call qemu_chr_fe_write and we won't assume a qemu_chr_fe_open 2) But I agree the assuming of qemu_chr_fe_open on the first write is a hack, it stems from before we had qemu_chr_fe_open/_close and now that we do have we should remove it. Note btw that many backends also don't handle sending CHR_EVENT_OPENED / _CLOSED themselves, they simply use qemu_chr_generic_open and that generated the opened event once on creation of the device. So the concept is just as broken / not broken on the backend side. We could do the same and have a qemu_fe_generic_open for frontends which does the qemu_chr_fe_open call. <snip> > And for me, the most logical thing is to call qemu_chr_fe_open() in > post_load for the device. With the device I assume you mean the frontend? That is what we originally suggested and submitted a patch for (for virtio-console) but Amit didn't like it. Regards, Hans
Hans de Goede <hdegoede@redhat.com> writes: > Hi, > > On 03/22/2013 02:50 PM, Anthony Liguori wrote: >> Hans de Goede <hdegoede@redhat.com> writes: >> >> We should have never allowed that in the first place and >> I object strongly to extending the concept without making it make sense >> for everything else. >> >>> Frontends end inside the guest usually in the form of some piece of >>> virtual hardware inside the guest. Just because the virtual hardware >>> is there does not mean the guest has a driver, >> >> Okay, let's use your example here with a standard UART. In the >> following sequence, I should receive: >> >> 1) Starts guest >> 2) When guest initializes the UART, qemu_chr_fe_open() >> 3) Reboot guest >> 4) Receive qemu_chr_fe_close() >> 5) Boot new guest without a UART driver >> 6) Nothing is received >> >> But this doesn't happen today because the only backend that cares >> (spice-*) assumes qemu_chr_fe_open() on the first qemu_chr_fe_write(). > > 1) If the guest does not have an uart driver, nothing will be written > to the uart, so it wont call qemu_chr_fe_write and we won't assume > a qemu_chr_fe_open > > 2) But I agree the assuming of qemu_chr_fe_open on the first write is > a hack, it stems from before we had qemu_chr_fe_open/_close and now that > we do have we should remove it. If the qemu-char.c code did: int qemu_chr_fe_write(...) { if (!s->fe_open) { qemu_chr_fe_open(s); } ... } That would be one thing. It's a hack, but a more reasonable hack than doing this in the backend like we do today. And in fact, herein lies exactly what the problem is. There is no "s->fe_open". And if such a thing did exist, we would note that this is in fact guest state and that it needs to be taken care of during migration. > Note btw that many backends also don't handle sending CHR_EVENT_OPENED / > _CLOSED themselves, they simply use qemu_chr_generic_open and that generated > the opened event once on creation of the device. So the concept is just > as broken / not broken on the backend side. I know :-/ > We could do the same and have a qemu_fe_generic_open for frontends which > does the qemu_chr_fe_open call. You mean, in qemu_chr_fe_write()? Yes, I think not doing this bit in the backend and making it part of qemu-char would be a significant improvement and would also guide in solving this problem correctly. Also, you can get reset handling for free by unconditionally clearly s->fe_open with a reset handler. That would also simplify the virtio-serial code since it would no longer need the reset logic. >> And for me, the most logical thing is to call qemu_chr_fe_open() in >> post_load for the device. > > With the device I assume you mean the frontend? That is what we originally > suggested and submitted a patch for (for virtio-console) but Amit didn't > like it. As Avi would say, this is "derived guest state" and derived guest state has to be recomputed in post_load handlers. Regards, Anthony Liguori > > Regards, > > Hans
Hi, On 03/22/2013 06:11 PM, Anthony Liguori wrote: > Hans de Goede <hdegoede@redhat.com> writes: <snip> > If the qemu-char.c code did: > > int qemu_chr_fe_write(...) { > if (!s->fe_open) { > qemu_chr_fe_open(s); > } > ... > } > > That would be one thing. It's a hack, but a more reasonable hack than > doing this in the backend like we do today. Agreed. > And in fact, herein lies exactly what the problem is. There is no > "s->fe_open". And if such a thing did exist, we would note that this is > in fact guest state and that it needs to be taken care of during > migration. Agreed too, I've prepared a patch set adding fe_open and cleaning up the whole existing code around the fe_open stuff. I'll send it directly after this mail. <snip> >> We could do the same and have a qemu_fe_generic_open for frontends which >> does the qemu_chr_fe_open call. > > You mean, in qemu_chr_fe_write()? Yes, I think not doing this bit in > the backend and making it part of qemu-char would be a significant > improvement and would also guide in solving this problem correctly. I believe that qemu_chr_fe_write is too late, this assumes that the frontend is always the first one to do a write (assuming fe_open aware backends won't do anything until the fe_open happens). But what if the protocol going over the chardev expects the backend to do the first write? Then the backend will just sit there waiting for the fe_open, and the frontend will just sit there waiting for the first write before sending this fe_open. I believe that your previous comments on qemu_chr_add_handlers being the closest thing to a fe_open for many frontends is correct, esp. since some frontends and hw/qdev-properties-system.c do a NULL qemu_chr_add_handlers when a frontend is being unregistered / removed / closed. Which gives us a central place to detect frontend closes too. So this is what I've used in my patch-set. > Also, you can get reset handling for free by unconditionally clearly > s->fe_open with a reset handler. That would also simplify the > virtio-serial code since it would no longer need the reset logic. > >>> And for me, the most logical thing is to call qemu_chr_fe_open() in >>> post_load for the device. >> >> With the device I assume you mean the frontend? That is what we originally >> suggested and submitted a patch for (for virtio-console) but Amit didn't >> like it. > > As Avi would say, this is "derived guest state" and derived guest state > has to be recomputed in post_load handlers. Agreed, which brings us back to the original patch posted a long time ago which Amit did not like: http://lists.gnu.org/archive/html/qemu-devel/2012-11/msg02721.html Amit can you take a second look at this? Note that after the cleanup patchset which I'll send right after this mail, it will look slightly different, something like: @@ -653,6 +654,10 @@ static void virtio_serial_post_load_timer_cb(void *opaque) send_control_event(port, VIRTIO_CONSOLE_PORT_OPEN, port->host_connected); } + vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port); + if (vsc->set_guest_connected) { + vsc->set_guest_connected(port, port->guest_connected); + } } g_free(s->post_load.connected); s->post_load.connected = NULL; Which to me looks like a reasonable thing to do on post-load. Regards, Hans
diff --git a/hw/virtio-console.c b/hw/virtio-console.c index e2d1c58..643e24e 100644 --- a/hw/virtio-console.c +++ b/hw/virtio-console.c @@ -120,6 +120,13 @@ static void chr_event(void *opaque, int event) } } +static bool chr_is_guest_connected(void *opaque) +{ + VirtConsole *vcon = opaque; + + return vcon->port.guest_connected; +} + static int virtconsole_initfn(VirtIOSerialPort *port) { VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port); @@ -133,6 +140,8 @@ static int virtconsole_initfn(VirtIOSerialPort *port) if (vcon->chr) { qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event, vcon); + /* only user of chr_is_guest_connected so leave it as special cased*/ + vcon->chr->chr_is_guest_connected = chr_is_guest_connected; } return 0; diff --git a/include/char/char.h b/include/char/char.h index 0326b2a..b41ddc0 100644 --- a/include/char/char.h +++ b/include/char/char.h @@ -52,6 +52,7 @@ typedef struct { #define CHR_TIOCM_RTS 0x004 typedef void IOEventHandler(void *opaque, int event); +typedef bool IOIsGuestConnectedHandler(void *opaque); struct CharDriverState { void (*init)(struct CharDriverState *s); @@ -64,6 +65,7 @@ struct CharDriverState { IOEventHandler *chr_event; IOCanReadHandler *chr_can_read; IOReadHandler *chr_read; + IOIsGuestConnectedHandler *chr_is_guest_connected; void *handler_opaque; void (*chr_close)(struct CharDriverState *chr); void (*chr_accept_input)(struct CharDriverState *chr); @@ -229,6 +231,15 @@ void qemu_chr_be_write(CharDriverState *s, uint8_t *buf, int len); */ void qemu_chr_be_event(CharDriverState *s, int event); +/** + * @qemu_chr_be_is_fe_connected: + * + * Back end calls this to check if the front end is connected. + * + * Returns: true if the guest (front end) is connected, false otherwise. + */ +bool qemu_chr_be_is_fe_connected(CharDriverState *s); + void qemu_chr_add_handlers(CharDriverState *s, IOCanReadHandler *fd_can_read, IOReadHandler *fd_read, diff --git a/qemu-char.c b/qemu-char.c index 4e011df..77a501a 100644 --- a/qemu-char.c +++ b/qemu-char.c @@ -120,6 +120,15 @@ void qemu_chr_be_event(CharDriverState *s, int event) s->chr_event(s->handler_opaque, event); } +bool qemu_chr_be_is_fe_connected(CharDriverState *s) +{ + if (s->chr_is_guest_connected) { + return s->chr_is_guest_connected(s->handler_opaque); + } + /* default to always connected */ + return true; +} + static gboolean qemu_chr_generic_open_bh(gpointer opaque) { CharDriverState *s = opaque;
Note that the handler is called chr_is_guest_connected and not chr_is_fe_connected, consistent with other members of CharDriverState. Signed-off-by: Alon Levy <alevy@redhat.com> --- hw/virtio-console.c | 9 +++++++++ include/char/char.h | 11 +++++++++++ qemu-char.c | 9 +++++++++ 3 files changed, 29 insertions(+)