[Qemu-devel] [PATCH v3 0/2] QOM realize for virtio-console

[Qemu-devel] [PATCH v3 0/2] QOM realize for virtio-console

[Andreas Färber]

Hello,

This series converts VirtIOSerialPort to QOM realize/unrealize.

v3 minimizes changes by adopting Anthony's new scheme of just replacing fields,
as done for VirtioDevices already.

I have simple qtests for virtio-serial-bus and virtio-console on a different branch
that I'll submit shortly, with the intent of queuing first the tests and then
these leftover conversions on qom-next.

Available from:
https://github.com/afaerber/qemu-cpu/commits/realize-virtio-console.v3
git://github.com/afaerber/qemu-cpu.git realize-virtio-console.v3

Regards,
Andreas

v2 -> v3:
* Revert changes to just change init/exit signature to realize/unrealize,
  leave the base class in control over realize/unrealize and call in-order.
* Rebased on Paolo's version of virtio QOM realize conversion - no changes.

v1 -> v2:
* Split off virtserialport fix for 1.6.
* Split off from main virtio realization series.
* Split out cleanups (mst).

Cc: Amit Shah <amit.shah@redhat.com>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Cc: Anthony Liguori <anthony@codemonkey.ws>
Cc: Frederic Konrad <fred.konrad@greensocs.com>

Andreas Färber (2):
  virtio-console: QOM cast cleanup for VirtConsole
  virtio-serial-port: Convert to QOM realize/unrealize

 hw/char/virtio-console.c          | 57 +++++++++++++++++++++------------------
 hw/char/virtio-serial-bus.c       | 52 ++++++++++++++++++-----------------
 include/hw/virtio/virtio-serial.h | 14 +++++-----
 3 files changed, 65 insertions(+), 58 deletions(-)

-- 
1.8.4.5

[Qemu-devel] [PATCH v3 1/2] virtio-console: QOM cast cleanup for VirtConsole

[Andreas Färber]

Introduce type constant, cast macro and rename parent field.

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/char/virtio-console.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 2e00ad2..73e18f2 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -15,8 +15,13 @@
 #include "trace.h"
 #include "hw/virtio/virtio-serial.h"
 
+#define TYPE_VIRTIO_CONSOLE "virtconsole"
+#define VIRTIO_CONSOLE(obj) \
+    OBJECT_CHECK(VirtConsole, (obj), TYPE_VIRTIO_CONSOLE)
+
 typedef struct VirtConsole {
-    VirtIOSerialPort port;
+    VirtIOSerialPort parent_obj;
+
     CharDriverState *chr;
     guint watch;
 } VirtConsole;
@@ -31,7 +36,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
     VirtConsole *vcon = opaque;
 
     vcon->watch = 0;
-    virtio_serial_throttle_port(&vcon->port, false);
+    virtio_serial_throttle_port(VIRTIO_SERIAL_PORT(vcon), false);
     return FALSE;
 }
 
@@ -39,7 +44,7 @@ static gboolean chr_write_unblocked(GIOChannel *chan, GIOCondition cond,
 static ssize_t flush_buf(VirtIOSerialPort *port,
                          const uint8_t *buf, ssize_t len)
 {
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+    VirtConsole *vcon = VIRTIO_CONSOLE(port);
     ssize_t ret;
 
     if (!vcon->chr) {
@@ -75,7 +80,7 @@ static ssize_t flush_buf(VirtIOSerialPort *port,
 /* Callback function that's called when the guest opens/closes the port */
 static void set_guest_connected(VirtIOSerialPort *port, int guest_connected)
 {
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+    VirtConsole *vcon = VIRTIO_CONSOLE(port);
 
     if (!vcon->chr) {
         return;
@@ -88,40 +93,42 @@ static int chr_can_read(void *opaque)
 {
     VirtConsole *vcon = opaque;
 
-    return virtio_serial_guest_ready(&vcon->port);
+    return virtio_serial_guest_ready(VIRTIO_SERIAL_PORT(vcon));
 }
 
 /* Send data from a char device over to the guest */
 static void chr_read(void *opaque, const uint8_t *buf, int size)
 {
     VirtConsole *vcon = opaque;
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
 
-    trace_virtio_console_chr_read(vcon->port.id, size);
-    virtio_serial_write(&vcon->port, buf, size);
+    trace_virtio_console_chr_read(port->id, size);
+    virtio_serial_write(port, buf, size);
 }
 
 static void chr_event(void *opaque, int event)
 {
     VirtConsole *vcon = opaque;
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(vcon);
 
-    trace_virtio_console_chr_event(vcon->port.id, event);
+    trace_virtio_console_chr_event(port->id, event);
     switch (event) {
     case CHR_EVENT_OPENED:
-        virtio_serial_open(&vcon->port);
+        virtio_serial_open(port);
         break;
     case CHR_EVENT_CLOSED:
         if (vcon->watch) {
             g_source_remove(vcon->watch);
             vcon->watch = 0;
         }
-        virtio_serial_close(&vcon->port);
+        virtio_serial_close(port);
         break;
     }
 }
 
 static int virtconsole_initfn(VirtIOSerialPort *port)
 {
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+    VirtConsole *vcon = VIRTIO_CONSOLE(port);
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
 
     if (port->id == 0 && !k->is_console) {
@@ -140,7 +147,7 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
 
 static int virtconsole_exitfn(VirtIOSerialPort *port)
 {
-    VirtConsole *vcon = DO_UPCAST(VirtConsole, port, port);
+    VirtConsole *vcon = VIRTIO_CONSOLE(port);
 
     if (vcon->watch) {
         g_source_remove(vcon->watch);
@@ -168,7 +175,7 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
 }
 
 static const TypeInfo virtconsole_info = {
-    .name          = "virtconsole",
+    .name          = TYPE_VIRTIO_CONSOLE,
     .parent        = TYPE_VIRTIO_SERIAL_PORT,
     .instance_size = sizeof(VirtConsole),
     .class_init    = virtconsole_class_init,
-- 
1.8.4.5

[Qemu-devel] [PATCH v3 2/2] virtio-serial-port: Convert to QOM realize/unrealize

[Andreas Färber]

Signed-off-by: Andreas Färber <afaerber@suse.de>
---
 hw/char/virtio-console.c          | 28 ++++++++++-----------
 hw/char/virtio-serial-bus.c       | 52 ++++++++++++++++++++-------------------
 include/hw/virtio/virtio-serial.h | 14 +++++------
 3 files changed, 47 insertions(+), 47 deletions(-)

diff --git a/hw/char/virtio-console.c b/hw/char/virtio-console.c
index 73e18f2..ffd29a8 100644
--- a/hw/char/virtio-console.c
+++ b/hw/char/virtio-console.c
@@ -126,14 +126,16 @@ static void chr_event(void *opaque, int event)
     }
 }
 
-static int virtconsole_initfn(VirtIOSerialPort *port)
+static void virtconsole_realize(DeviceState *dev, Error **errp)
 {
-    VirtConsole *vcon = VIRTIO_CONSOLE(port);
-    VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
+    VirtConsole *vcon = VIRTIO_CONSOLE(dev);
+    VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_GET_CLASS(dev);
 
     if (port->id == 0 && !k->is_console) {
-        error_report("Port number 0 on virtio-serial devices reserved for virtconsole devices for backward compatibility.");
-        return -1;
+        error_setg(errp, "Port number 0 on virtio-serial devices reserved "
+                   "for virtconsole devices for backward compatibility.");
+        return;
     }
 
     if (vcon->chr) {
@@ -141,19 +143,15 @@ static int virtconsole_initfn(VirtIOSerialPort *port)
         qemu_chr_add_handlers(vcon->chr, chr_can_read, chr_read, chr_event,
                               vcon);
     }
-
-    return 0;
 }
 
-static int virtconsole_exitfn(VirtIOSerialPort *port)
+static void virtconsole_unrealize(DeviceState *dev, Error **errp)
 {
-    VirtConsole *vcon = VIRTIO_CONSOLE(port);
+    VirtConsole *vcon = VIRTIO_CONSOLE(dev);
 
     if (vcon->watch) {
         g_source_remove(vcon->watch);
     }
-
-    return 0;
 }
 
 static Property virtconsole_properties[] = {
@@ -167,8 +165,8 @@ static void virtconsole_class_init(ObjectClass *klass, void *data)
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_CLASS(klass);
 
     k->is_console = true;
-    k->init = virtconsole_initfn;
-    k->exit = virtconsole_exitfn;
+    k->realize = virtconsole_realize;
+    k->unrealize = virtconsole_unrealize;
     k->have_data = flush_buf;
     k->set_guest_connected = set_guest_connected;
     dc->props = virtconsole_properties;
@@ -191,8 +189,8 @@ static void virtserialport_class_init(ObjectClass *klass, void *data)
     DeviceClass *dc = DEVICE_CLASS(klass);
     VirtIOSerialPortClass *k = VIRTIO_SERIAL_PORT_CLASS(klass);
 
-    k->init = virtconsole_initfn;
-    k->exit = virtconsole_exitfn;
+    k->realize = virtconsole_realize;
+    k->unrealize = virtconsole_unrealize;
     k->have_data = flush_buf;
     k->set_guest_connected = set_guest_connected;
     dc->props = virtserialport_properties;
diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index 226e9f9..5a52162 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -808,13 +808,14 @@ static void remove_port(VirtIOSerial *vser, uint32_t port_id)
     send_control_event(vser, port->id, VIRTIO_CONSOLE_PORT_REMOVE, 1);
 }
 
-static int virtser_port_qdev_init(DeviceState *qdev)
+static void virtser_port_device_realize(DeviceState *dev, Error **errp)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
     VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
-    VirtIOSerialBus *bus = DO_UPCAST(VirtIOSerialBus, qbus, qdev->parent_bus);
-    int ret, max_nr_ports;
+    VirtIOSerialBus *bus = VIRTIO_SERIAL_BUS(qdev_get_parent_bus(dev));
+    int max_nr_ports;
     bool plugging_port0;
+    Error *err = NULL;
 
     port->vser = bus->vser;
     port->bh = qemu_bh_new(flush_queued_data_bh, port);
@@ -829,9 +830,9 @@ static int virtser_port_qdev_init(DeviceState *qdev)
     plugging_port0 = vsc->is_console && !find_port_by_id(port->vser, 0);
 
     if (find_port_by_id(port->vser, port->id)) {
-        error_report("virtio-serial-bus: A port already exists at id %u",
-                     port->id);
-        return -1;
+        error_setg(errp, "virtio-serial-bus: A port already exists at id %u",
+                   port->id);
+        return;
     }
 
     if (port->id == VIRTIO_CONSOLE_BAD_ID) {
@@ -840,22 +841,25 @@ static int virtser_port_qdev_init(DeviceState *qdev)
         } else {
             port->id = find_free_port_id(port->vser);
             if (port->id == VIRTIO_CONSOLE_BAD_ID) {
-                error_report("virtio-serial-bus: Maximum port limit for this device reached");
-                return -1;
+                error_setg(errp, "virtio-serial-bus: Maximum port limit for "
+                                 "this device reached");
+                return;
             }
         }
     }
 
     max_nr_ports = tswap32(port->vser->config.max_nr_ports);
     if (port->id >= max_nr_ports) {
-        error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
-                     max_nr_ports - 1);
-        return -1;
+        error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
+                         "max. allowed: %u",
+                   max_nr_ports - 1);
+        return;
     }
 
-    ret = vsc->init(port);
-    if (ret) {
-        return ret;
+    vsc->realize(dev, &err);
+    if (err != NULL) {
+        error_propagate(errp, err);
+        return;
     }
 
     port->elem.out_num = 0;
@@ -868,14 +872,12 @@ static int virtser_port_qdev_init(DeviceState *qdev)
 
     /* Send an update to the guest about this new port added */
     virtio_notify_config(VIRTIO_DEVICE(port->vser));
-
-    return ret;
 }
 
-static int virtser_port_qdev_exit(DeviceState *qdev)
+static void virtser_port_device_unrealize(DeviceState *dev, Error **errp)
 {
-    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev, qdev);
-    VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(port);
+    VirtIOSerialPort *port = VIRTIO_SERIAL_PORT(dev);
+    VirtIOSerialPortClass *vsc = VIRTIO_SERIAL_PORT_GET_CLASS(dev);
     VirtIOSerial *vser = port->vser;
 
     qemu_bh_delete(port->bh);
@@ -883,10 +885,9 @@ static int virtser_port_qdev_exit(DeviceState *qdev)
 
     QTAILQ_REMOVE(&vser->ports, port, next);
 
-    if (vsc->exit) {
-        vsc->exit(port);
+    if (vsc->unrealize) {
+        vsc->unrealize(dev, errp);
     }
-    return 0;
 }
 
 static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
@@ -971,10 +972,11 @@ static void virtio_serial_device_realize(DeviceState *dev, Error **errp)
 static void virtio_serial_port_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *k = DEVICE_CLASS(klass);
-    k->init = virtser_port_qdev_init;
+
     set_bit(DEVICE_CATEGORY_INPUT, k->categories);
     k->bus_type = TYPE_VIRTIO_SERIAL_BUS;
-    k->exit = virtser_port_qdev_exit;
+    k->realize = virtser_port_device_realize;
+    k->unrealize = virtser_port_device_unrealize;
     k->unplug = qdev_simple_unplug_cb;
     k->props = virtser_props;
 }
diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
index 1d2040b..b1bc9e5 100644
--- a/include/hw/virtio/virtio-serial.h
+++ b/include/hw/virtio/virtio-serial.h
@@ -77,19 +77,19 @@ typedef struct VirtIOSerialPort VirtIOSerialPort;
 typedef struct VirtIOSerialPortClass {
     DeviceClass parent_class;
 
-    /* Is this a device that binds with hvc in the guest? */
-    bool is_console;
-
     /*
-     * The per-port (or per-app) init function that's called when a
+     * The per-port (or per-app) realize function that's called when a
      * new device is found on the bus.
      */
-    int (*init)(VirtIOSerialPort *port);
+    DeviceRealize realize;
     /*
-     * Per-port exit function that's called when a port gets
+     * Per-port unrealize function that's called when a port gets
      * hot-unplugged or removed.
      */
-    int (*exit)(VirtIOSerialPort *port);
+    DeviceUnrealize unrealize;
+
+    /* Is this a device that binds with hvc in the guest? */
+    bool is_console;
 
     /* Callbacks for guest events */
         /* Guest opened/closed device. */
-- 
1.8.4.5

Re: [Qemu-devel] [PATCH v3 2/2] virtio-serial-port: Convert to QOM realize/unrealize

[Amit Shah]

Hi,

I haven't really followed the whole discussion, so can't say much
about it -- overall looks alright.  A couple of minor nits:

>      max_nr_ports = tswap32(port->vser->config.max_nr_ports);
>      if (port->id >= max_nr_ports) {
> -        error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
> -                     max_nr_ports - 1);
> -        return -1;
> +        error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
> +                         "max. allowed: %u",
> +                   max_nr_ports - 1);

indentation looks off.  Just put this last line on the line above?

> diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
> index 1d2040b..b1bc9e5 100644
> --- a/include/hw/virtio/virtio-serial.h
> +++ b/include/hw/virtio/virtio-serial.h
> @@ -77,19 +77,19 @@ typedef struct VirtIOSerialPort VirtIOSerialPort;
>  typedef struct VirtIOSerialPortClass {
>      DeviceClass parent_class;
>  
> -    /* Is this a device that binds with hvc in the guest? */
> -    bool is_console;
> -
>      /*
> -     * The per-port (or per-app) init function that's called when a
> +     * The per-port (or per-app) realize function that's called when a
>       * new device is found on the bus.
>       */
> -    int (*init)(VirtIOSerialPort *port);
> +    DeviceRealize realize;
>      /*
> -     * Per-port exit function that's called when a port gets
> +     * Per-port unrealize function that's called when a port gets
>       * hot-unplugged or removed.
>       */
> -    int (*exit)(VirtIOSerialPort *port);
> +    DeviceUnrealize unrealize;
> +
> +    /* Is this a device that binds with hvc in the guest? */
> +    bool is_console;

is_console is moved unnecessarily?


		Amit

Re: [Qemu-devel] [PATCH v3 2/2] virtio-serial-port: Convert to QOM realize/unrealize

[Andreas Färber]

Hi Amit,

Am 12.03.2014 13:41, schrieb Amit Shah:
> Hi,
> 
> I haven't really followed the whole discussion, so can't say much
> about it -- overall looks alright.  A couple of minor nits:
> 
>>      max_nr_ports = tswap32(port->vser->config.max_nr_ports);
>>      if (port->id >= max_nr_ports) {
>> -        error_report("virtio-serial-bus: Out-of-range port id specified, max. allowed: %u",
>> -                     max_nr_ports - 1);
>> -        return -1;
>> +        error_setg(errp, "virtio-serial-bus: Out-of-range port id specified, "
>> +                         "max. allowed: %u",
>> +                   max_nr_ports - 1);
> 
> indentation looks off.  Just put this last line on the line above?

Done. (Was intentional to align error string, but no need to keep on
separate lines after breaking.)

> 
>> diff --git a/include/hw/virtio/virtio-serial.h b/include/hw/virtio/virtio-serial.h
>> index 1d2040b..b1bc9e5 100644
>> --- a/include/hw/virtio/virtio-serial.h
>> +++ b/include/hw/virtio/virtio-serial.h
>> @@ -77,19 +77,19 @@ typedef struct VirtIOSerialPort VirtIOSerialPort;
>>  typedef struct VirtIOSerialPortClass {
>>      DeviceClass parent_class;
>>  
>> -    /* Is this a device that binds with hvc in the guest? */
>> -    bool is_console;
>> -
>>      /*
>> -     * The per-port (or per-app) init function that's called when a
>> +     * The per-port (or per-app) realize function that's called when a
>>       * new device is found on the bus.
>>       */
>> -    int (*init)(VirtIOSerialPort *port);
>> +    DeviceRealize realize;
>>      /*
>> -     * Per-port exit function that's called when a port gets
>> +     * Per-port unrealize function that's called when a port gets
>>       * hot-unplugged or removed.
>>       */
>> -    int (*exit)(VirtIOSerialPort *port);
>> +    DeviceUnrealize unrealize;
>> +
>> +    /* Is this a device that binds with hvc in the guest? */
>> +    bool is_console;
> 
> is_console is moved unnecessarily?

Fixed. (Previously fields parent_realize were added right after parent_obj.)

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg