From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=51701 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Ofz2b-00029Z-Up for qemu-devel@nongnu.org; Mon, 02 Aug 2010 13:43:11 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69) (envelope-from ) id 1Ofz2Z-0004PO-2W for qemu-devel@nongnu.org; Mon, 02 Aug 2010 13:43:09 -0400 Received: from mail-yw0-f45.google.com ([209.85.213.45]:65512) by eggs.gnu.org with esmtp (Exim 4.69) (envelope-from ) id 1Ofz2Y-0004P7-T4 for qemu-devel@nongnu.org; Mon, 02 Aug 2010 13:43:07 -0400 Received: by ywt2 with SMTP id 2so1485493ywt.4 for ; Mon, 02 Aug 2010 10:43:06 -0700 (PDT) Message-ID: <4C5703A1.2040500@codemonkey.ws> Date: Mon, 02 Aug 2010 12:42:57 -0500 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] RFC adding ioctl's to virtserial/virtconsole References: <1998763220.603051280770128553.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com> In-Reply-To: <1998763220.603051280770128553.JavaMail.root@zmail06.collab.prod.int.phx2.redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alon Levy Cc: Amit Shah , Anthony Liguori , qemu-devel On 08/02/2010 12:28 PM, Alon Levy wrote: > ----- "Anthony Liguori" 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 >>> 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 >>> >>>