Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Alon Levy]

----- "Anthony Liguori" <anthony@codemonkey.ws> 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.  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.
> 
Ok, I don't see how it's that totally simple right now, but I'm going to
send a patch along those lines. Basically just adding a qemu_chr_server_write
to be used instead of the qemu_chr_write.

I think it would actually be simpler to add connect and disconnect to ChrDriverState
and have a guest_connect host_connected booleans (basically same as virtio-serial).
For the normal case (all existing backends) a connected bool would be initialized to 1,
and write would have an if (!connected) return, and connect/disconnect would be empty.
For spicevmc backend I would implement those as I would have the ioctl's / events.

> Regards,
> 
> Anthony Liguori
> 
> 
> >
> > cheers,
> >   Gerd
> >

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Alon Levy]

----- "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:

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
> >

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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
>>>
>>>        

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Gerd Hoffmann]

   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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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
>

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Gerd Hoffmann]

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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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
>

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Gerd Hoffmann]

   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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Gerd Hoffmann]

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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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
>

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Gerd Hoffmann]

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

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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
>

[Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Alon Levy]

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.

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
-- 
1.7.2

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Amit Shah]

On (Mon) Aug 02 2010 [04:33:49], 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.

Looks fine to me, maybe you don't need the notifications for
virtio-console ports, only the virtio-serial ports could be of interest,
but I'm fine with both notifying as well.

Also, this description could be made part of the patch description
itself.

		Amit

Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole

[Anthony Liguori]

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.

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
>