Message ID | 1252055670-26958-1-git-send-email-amit.shah@redhat.com |
---|---|
State | Superseded |
Headers | show |
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?
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
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.
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
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.
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
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
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
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
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
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; }
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(-)