[Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

Currently the VirtIOConsole struct is allocated from the call
to virtio_common_init, also doing an UP_CAST implicitly.

The new multiport functionality will need a few arrays and
it's easier to move to the new VMState infrastructure by
keeping it all within one struct.

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

diff --git a/hw/virtio-console.c b/hw/virtio-console.c
index 57f8f89..5d08321 100644
--- a/hw/virtio-console.c
+++ b/hw/virtio-console.c
@@ -19,11 +19,13 @@
 
 typedef struct VirtIOConsole
 {
-    VirtIODevice vdev;
+    VirtIODevice *vdev;
     VirtQueue *ivq, *ovq;
     CharDriverState *chr;
 } VirtIOConsole;
 
+VirtIOConsole virtconsole;
+
 static VirtIOConsole *to_virtio_console(VirtIODevice *vdev)
 {
     return (VirtIOConsole *)vdev;
@@ -61,7 +63,7 @@ static int vcon_can_read(void *opaque)
     VirtIOConsole *s = (VirtIOConsole *) opaque;
 
     if (!virtio_queue_ready(s->ivq) ||
-        !(s->vdev.status & VIRTIO_CONFIG_S_DRIVER_OK) ||
+        !(s->vdev->status & VIRTIO_CONFIG_S_DRIVER_OK) ||
         virtio_queue_empty(s->ivq))
         return 0;
 
@@ -97,7 +99,7 @@ static void vcon_read(void *opaque, const uint8_t *buf, int size)
         }
         virtqueue_push(s->ivq, &elem, size);
     }
-    virtio_notify(&s->vdev, s->ivq);
+    virtio_notify(s->vdev, s->ivq);
 }
 
 static void vcon_event(void *opaque, int event)
@@ -109,7 +111,7 @@ static void virtio_console_save(QEMUFile *f, void *opaque)
 {
     VirtIOConsole *s = opaque;
 
-    virtio_save(&s->vdev, f);
+    virtio_save(s->vdev, f);
 }
 
 static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
@@ -119,25 +121,28 @@ static int virtio_console_load(QEMUFile *f, void *opaque, int version_id)
     if (version_id != 1)
         return -EINVAL;
 
-    virtio_load(&s->vdev, f);
+    virtio_load(s->vdev, f);
     return 0;
 }
 
 VirtIODevice *virtio_console_init(DeviceState *dev)
 {
-    VirtIOConsole *s;
-    s = (VirtIOConsole *)virtio_common_init("virtio-console",
-                                            VIRTIO_ID_CONSOLE,
-                                            0, sizeof(VirtIOConsole));
-    s->vdev.get_features = virtio_console_get_features;
+    virtconsole.vdev = virtio_common_init("virtio-console",
+                                          VIRTIO_ID_CONSOLE,
+                                          0, sizeof(VirtIODevice));
+    virtconsole.vdev->get_features = virtio_console_get_features;
 
-    s->ivq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_input);
-    s->ovq = virtio_add_queue(&s->vdev, 128, virtio_console_handle_output);
+    virtconsole.ivq = virtio_add_queue(virtconsole.vdev, 128,
+                                       virtio_console_handle_input);
+    virtconsole.ovq = virtio_add_queue(virtconsole.vdev, 128,
+                                       virtio_console_handle_output);
 
-    s->chr = qdev_init_chardev(dev);
-    qemu_chr_add_handlers(s->chr, vcon_can_read, vcon_read, vcon_event, s);
+    virtconsole.chr = qdev_init_chardev(dev);
+    qemu_chr_add_handlers(virtconsole.chr, vcon_can_read, vcon_read, vcon_event,
+                          &virtconsole);
 
-    register_savevm("virtio-console", -1, 1, virtio_console_save, virtio_console_load, s);
+    register_savevm("virtio-console", -1, 1, virtio_console_save,
+                    virtio_console_load, &virtconsole);
 
-    return &s->vdev;
+    return virtconsole.vdev;
 }
-- 
1.6.2.5

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Blue Swirl]

On Fri, Sep 4, 2009 at 12:14 PM, Amit Shah<amit.shah@redhat.com> wrote:
> Currently the VirtIOConsole struct is allocated from the call
> to virtio_common_init, also doing an UP_CAST implicitly.
>
> The new multiport functionality will need a few arrays and
> it's easier to move to the new VMState infrastructure by
> keeping it all within one struct.

> +VirtIOConsole virtconsole;

IMHO this is going to wrong direction. What kind of code would need a
static instance?

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

On (Fri) Sep 04 2009 [19:26:27], Blue Swirl wrote:
> On Fri, Sep 4, 2009 at 12:14 PM, Amit Shah<amit.shah@redhat.com> wrote:
> > Currently the VirtIOConsole struct is allocated from the call
> > to virtio_common_init, also doing an UP_CAST implicitly.
> >
> > The new multiport functionality will need a few arrays and
> > it's easier to move to the new VMState infrastructure by
> > keeping it all within one struct.
> 
> > +VirtIOConsole virtconsole;
> 
> IMHO this is going to wrong direction. What kind of code would need a
> static instance?

Adding multiple ports to the console device, we'll have to store an
array of ports here as well as config space. Both of these are
device-specific.

		Amit

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Blue Swirl]

On Fri, Sep 4, 2009 at 7:33 PM, Amit Shah<amit.shah@redhat.com> wrote:
> On (Fri) Sep 04 2009 [19:26:27], Blue Swirl wrote:
>> On Fri, Sep 4, 2009 at 12:14 PM, Amit Shah<amit.shah@redhat.com> wrote:
>> > Currently the VirtIOConsole struct is allocated from the call
>> > to virtio_common_init, also doing an UP_CAST implicitly.
>> >
>> > The new multiport functionality will need a few arrays and
>> > it's easier to move to the new VMState infrastructure by
>> > keeping it all within one struct.
>>
>> > +VirtIOConsole virtconsole;
>>
>> IMHO this is going to wrong direction. What kind of code would need a
>> static instance?
>
> Adding multiple ports to the console device, we'll have to store an
> array of ports here as well as config space. Both of these are
> device-specific.

There could be a master device which managed all ports.

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

On (Fri) Sep 04 2009 [19:37:25], Blue Swirl wrote:
> On Fri, Sep 4, 2009 at 7:33 PM, Amit Shah<amit.shah@redhat.com> wrote:
> > On (Fri) Sep 04 2009 [19:26:27], Blue Swirl wrote:
> >> On Fri, Sep 4, 2009 at 12:14 PM, Amit Shah<amit.shah@redhat.com> wrote:
> >> > Currently the VirtIOConsole struct is allocated from the call
> >> > to virtio_common_init, also doing an UP_CAST implicitly.
> >> >
> >> > The new multiport functionality will need a few arrays and
> >> > it's easier to move to the new VMState infrastructure by
> >> > keeping it all within one struct.
> >>
> >> > +VirtIOConsole virtconsole;
> >>
> >> IMHO this is going to wrong direction. What kind of code would need a
> >> static instance?
> >
> > Adding multiple ports to the console device, we'll have to store an
> > array of ports here as well as config space. Both of these are
> > device-specific.
> 
> There could be a master device which managed all ports.

There's only one instance of a virtio device created, and this device
hosts multiple ports. And VirIOConsole is the master structure.

Or did I miss what it is you're saying?

		Amit

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Blue Swirl]

On Fri, Sep 4, 2009 at 7:40 PM, Amit Shah<amit.shah@redhat.com> wrote:
> On (Fri) Sep 04 2009 [19:37:25], Blue Swirl wrote:
>> On Fri, Sep 4, 2009 at 7:33 PM, Amit Shah<amit.shah@redhat.com> wrote:
>> > On (Fri) Sep 04 2009 [19:26:27], Blue Swirl wrote:
>> >> On Fri, Sep 4, 2009 at 12:14 PM, Amit Shah<amit.shah@redhat.com> wrote:
>> >> > Currently the VirtIOConsole struct is allocated from the call
>> >> > to virtio_common_init, also doing an UP_CAST implicitly.
>> >> >
>> >> > The new multiport functionality will need a few arrays and
>> >> > it's easier to move to the new VMState infrastructure by
>> >> > keeping it all within one struct.
>> >>
>> >> > +VirtIOConsole virtconsole;
>> >>
>> >> IMHO this is going to wrong direction. What kind of code would need a
>> >> static instance?
>> >
>> > Adding multiple ports to the console device, we'll have to store an
>> > array of ports here as well as config space. Both of these are
>> > device-specific.
>>
>> There could be a master device which managed all ports.
>
> There's only one instance of a virtio device created, and this device
> hosts multiple ports. And VirIOConsole is the master structure.

But instead of this, you should have a separate structure for the
master one, if that way you can avoid the static instance.

Re: [Qemu-devel] [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

On (Fri) Sep 04 2009 [19:45:41], Blue Swirl wrote:
> On Fri, Sep 4, 2009 at 7:40 PM, Amit Shah<amit.shah@redhat.com> wrote:
> >> >> > Currently the VirtIOConsole struct is allocated from the call
> >> >> > to virtio_common_init, also doing an UP_CAST implicitly.
> >> >> >
> >> >> > The new multiport functionality will need a few arrays and
> >> >> > it's easier to move to the new VMState infrastructure by
> >> >> > keeping it all within one struct.
> >> >>
> >> >> > +VirtIOConsole virtconsole;
> >> >>
> >> >> IMHO this is going to wrong direction. What kind of code would need a
> >> >> static instance?
> >> >
> >> > Adding multiple ports to the console device, we'll have to store an
> >> > array of ports here as well as config space. Both of these are
> >> > device-specific.
> >>
> >> There could be a master device which managed all ports.
> >
> > There's only one instance of a virtio device created, and this device
> > hosts multiple ports. And VirIOConsole is the master structure.
> 
> But instead of this, you should have a separate structure for the
> master one, if that way you can avoid the static instance.

The problem with that is that the config space and the ports array have
to be made static anyway (because they get used at command-line parsing
time, before the virtio-console init function is called). So there's no
net gain for doing it that way and we're just keeping things outside of
the struct. And that doesn't fit well with the new proposed VMState
handlers.

		Amit

[Qemu-devel] Re: [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

On (Fri) Sep 04 2009 [19:36:33], Juan Quintela wrote:
> >> >
> >> > There's only one instance of a virtio device created, and this device
> >> > hosts multiple ports. And VirIOConsole is the master structure.
> >> 
> >> But instead of this, you should have a separate structure for the
> >> master one, if that way you can avoid the static instance.
> >
> > The problem with that is that the config space and the ports array have
> > to be made static anyway (because they get used at command-line parsing
> > time, before the virtio-console init function is called). So there's no
> > net gain for doing it that way and we're just keeping things outside of
> > the struct. And that doesn't fit well with the new proposed VMState
> > handlers.
> 
> I haven't looked at that, but qdev should help here.
> There has to be a way to create multiple devices from the command line,
> or qdev is doomed :)

We don't want multiple devices; just one virtio-console device with
several ports.

> What I haven't looked is at what part of how virt-console share the
> ports.

Share the ports in what sense?

		Amit

[Qemu-devel] Re: [PATCH] virtio-console: Have a static instance of virtconsole

[Anthony Liguori]

Amit Shah wrote:
> Currently the VirtIOConsole struct is allocated from the call
> to virtio_common_init, also doing an UP_CAST implicitly.
>
> The new multiport functionality will need a few arrays and
> it's easier to move to the new VMState infrastructure by
> keeping it all within one struct.
>
> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>   

This seems bad.  Even though the device is multiport capable, we should 
still supporting having multiple devices per guest.

I think Gerd would suggest this is a use-case for capabilities.  I'm not 
sure we have a similar device that can take multiple char drivers.

Maybe we ought to treat virtio-console like a bus?  Do you have any 
suggestions Gerd?

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] virtio-console: Have a static instance of virtconsole

[Gerd Hoffmann]

On 09/08/09 16:08, Anthony Liguori wrote:

>> Currently the VirtIOConsole struct is allocated from the call
>> to virtio_common_init, also doing an UP_CAST implicitly.

> Maybe we ought to treat virtio-console like a bus? Do you have any
> suggestions Gerd?

i.e. the virtio-console pci devices provides some virtual qdev bus (say 
vmch), then each port is modeled as separate qdev device for that bus? 
I think that handles it pretty nicely.

cheers,
   Gerd

[Qemu-devel] Re: [PATCH] virtio-console: Have a static instance of virtconsole

[Amit Shah]

On (Tue) Sep 08 2009 [09:08:03], Anthony Liguori wrote:
> Amit Shah wrote:
>> Currently the VirtIOConsole struct is allocated from the call
>> to virtio_common_init, also doing an UP_CAST implicitly.
>>
>> The new multiport functionality will need a few arrays and
>> it's easier to move to the new VMState infrastructure by
>> keeping it all within one struct.
>>
>> Signed-off-by: Amit Shah <amit.shah@redhat.com>
>>   
>
> This seems bad.  Even though the device is multiport capable, we should  
> still supporting having multiple devices per guest.

OK; so I'll drop this for now. I'd much rather we get the multiple ports
patch in first and then add support for multiple devices.

		Amit