diff mbox

[v9,3/3] virtio-console: Add a new virtserialport device for generic serial port support

Message ID 1256022825-16180-4-git-send-email-amit.shah@redhat.com
State New
Headers show

Commit Message

Amit Shah Oct. 20, 2009, 7:13 a.m. UTC
This patch adds generic serial ports over the virtio serial bus.
These ports have a few more options that are not relevant for
virtio console ports: the ability to cache buffers that are
received for a port while it's disconnected, setting of limits
to the bytes that are cached so as to prevent OOM conditions,
etc.

Sample uses for such a device can be obtaining info from the
guest like the file systems used, apps installed, etc. for
offline usage and logged-in users, clipboard copy-paste, etc.
for online usage.

For requirements, use-cases, test cases and some history see

    http://www.linux-kvm.org/page/VMchannel_Requirements

Signed-off-by: Amit Shah <amit.shah@redhat.com>
---
 hw/virtio-console.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 38 insertions(+), 0 deletions(-)

Comments

Gerd Hoffmann Oct. 20, 2009, 8:51 a.m. UTC | #1
> @@ -107,3 +107,41 @@ static void virtcon_register(void)
>       virtio_serial_port_qdev_register(&virtcon_info);
>   }
>   device_init(virtcon_register)

> +static VirtIOSerialPortInfo virtserial_port_info = {
> +    .qdev.name     = "virtserialport",
> +    .qdev.size     = sizeof(VirtConsole),
> +    .init          = virtserial_port_initfn,
> +    .have_data     = flush_buf,
> +    .qdev.props = (Property[]) {
> +        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),

likewise: DEFINE_PROP_STRING("name", VirtConsole, port.name),

> +static void virtserial_port_register(void)
> +{
> +    virtio_serial_port_qdev_register(&virtserial_port_info);
> +}
> +device_init(virtserial_port_register)

You can simply stick two register calls into the existing init function ...

cheers,
   Gerd
Amit Shah Oct. 20, 2009, 9:48 a.m. UTC | #2
On (Tue) Oct 20 2009 [10:51:30], Gerd Hoffmann wrote:
>> @@ -107,3 +107,41 @@ static void virtcon_register(void)
>>       virtio_serial_port_qdev_register(&virtcon_info);
>>   }
>>   device_init(virtcon_register)
>
>> +static VirtIOSerialPortInfo virtserial_port_info = {
>> +    .qdev.name     = "virtserialport",
>> +    .qdev.size     = sizeof(VirtConsole),
>> +    .init          = virtserial_port_initfn,
>> +    .have_data     = flush_buf,
>> +    .qdev.props = (Property[]) {
>> +        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
>> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
>
> likewise: DEFINE_PROP_STRING("name", VirtConsole, port.name),

The 'name' field is common to all ports, so it makes sense to put it in
the VirtIOSerialPort struct. (Internal users can pre-define it to their
liking, eg, org.qemu.vnc)

>> +static void virtserial_port_register(void)
>> +{
>> +    virtio_serial_port_qdev_register(&virtserial_port_info);
>> +}
>> +device_init(virtserial_port_register)
>
> You can simply stick two register calls into the existing init function ...

The console init function has extra init'ing to do, like disable buffer
caching, set is_console to true, etc., so there are two init fns.

		Amit
Gerd Hoffmann Oct. 20, 2009, 2:02 p.m. UTC | #3
On 10/20/09 11:48, Amit Shah wrote:
> On (Tue) Oct 20 2009 [10:51:30], Gerd Hoffmann wrote:
>>> @@ -107,3 +107,41 @@ static void virtcon_register(void)
>>>        virtio_serial_port_qdev_register(&virtcon_info);
>>>    }
>>>    device_init(virtcon_register)
>>
>>> +static VirtIOSerialPortInfo virtserial_port_info = {
>>> +    .qdev.name     = "virtserialport",
>>> +    .qdev.size     = sizeof(VirtConsole),
>>> +    .init          = virtserial_port_initfn,
>>> +    .have_data     = flush_buf,
>>> +    .qdev.props = (Property[]) {
>>> +        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
>>> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
>>
>> likewise: DEFINE_PROP_STRING("name", VirtConsole, port.name),
>
> The 'name' field is common to all ports, so it makes sense to put it in
> the VirtIOSerialPort struct. (Internal users can pre-define it to their
> liking, eg, org.qemu.vnc)

Sure.  I don't want to move it out there.  But the driver state struct 
is 'VirtConsole' so all properties should use that.  Note that the field 
changed too: from "name" to "port.name", so everything keeps working ;)

cheers,
   Gerd
Amit Shah Oct. 21, 2009, 3:44 a.m. UTC | #4
On (Tue) Oct 20 2009 [16:02:16], Gerd Hoffmann wrote:
> On 10/20/09 11:48, Amit Shah wrote:
>> On (Tue) Oct 20 2009 [10:51:30], Gerd Hoffmann wrote:
>>>> @@ -107,3 +107,41 @@ static void virtcon_register(void)
>>>>        virtio_serial_port_qdev_register(&virtcon_info);
>>>>    }
>>>>    device_init(virtcon_register)
>>>
>>>> +static VirtIOSerialPortInfo virtserial_port_info = {
>>>> +    .qdev.name     = "virtserialport",
>>>> +    .qdev.size     = sizeof(VirtConsole),
>>>> +    .init          = virtserial_port_initfn,
>>>> +    .have_data     = flush_buf,
>>>> +    .qdev.props = (Property[]) {
>>>> +        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
>>>> +        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
>>>
>>> likewise: DEFINE_PROP_STRING("name", VirtConsole, port.name),
>>
>> The 'name' field is common to all ports, so it makes sense to put it in
>> the VirtIOSerialPort struct. (Internal users can pre-define it to their
>> liking, eg, org.qemu.vnc)
>
> Sure.  I don't want to move it out there.  But the driver state struct  
> is 'VirtConsole' so all properties should use that.  Note that the field  
> changed too: from "name" to "port.name", so everything keeps working ;)

Ah; I understand it now. Sorry for the confusion.

Thanks Gerd for the reviews.

		Amit
diff mbox

Patch

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index ef32820..fec346c 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -107,3 +107,41 @@  static void virtcon_register(void)
     virtio_serial_port_qdev_register(&virtcon_info);
 }
 device_init(virtcon_register)
+
+
+/* Generic Virtio Serial Ports */
+static int virtserial_port_initfn(VirtIOSerialDevice *dev)
+{
+    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, &dev->qdev);
+    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+
+    port->info = dev->info;
+
+    if (vcon->chr) {
+        qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
+                              vcon);
+    }
+    return 0;
+}
+
+static VirtIOSerialPortInfo virtserial_port_info = {
+    .qdev.name     = "virtserialport",
+    .qdev.size     = sizeof(VirtConsole),
+    .init          = virtserial_port_initfn,
+    .have_data     = flush_buf,
+    .qdev.props = (Property[]) {
+        DEFINE_PROP_CHR("chardev", VirtConsole, chr),
+        DEFINE_PROP_STRING("name", VirtIOSerialPort, name),
+        DEFINE_PROP_INT32("cache_buffers", VirtIOSerialPort, cache_buffers, 1),
+        DEFINE_PROP_UINT64("byte_limit", VirtIOSerialPort, byte_limit, 0),
+        DEFINE_PROP_UINT64("guest_byte_limit", VirtIOSerialPort,
+                           guest_byte_limit, 0),
+        DEFINE_PROP_END_OF_LIST(),
+    },
+};
+
+static void virtserial_port_register(void)
+{
+    virtio_serial_port_qdev_register(&virtserial_port_info);
+}
+device_init(virtserial_port_register)