On (Tue) 24 Apr 2012 [14:21:36], Michael S. Tsirkin wrote: > On Tue, Apr 24, 2012 at 04:44:10PM +0530, Amit Shah wrote: > > On (Tue) 24 Apr 2012 [13:49:53], Michael S. Tsirkin wrote: > > > On Tue, Apr 24, 2012 at 03:56:44PM +0530, Amit Shah wrote: > > > > On (Tue) 24 Apr 2012 [13:17:16], Michael S. Tsirkin wrote: > > > > > On Tue, Apr 24, 2012 at 02:55:26PM +0530, Amit Shah wrote: > > > > > > From: Alon Levy <al...@redhat.com> > > > > > > > > > > > > guest_connected should be false before guest driver initialization, > > > > > > and > > > > > > true after, both for multiport aware and non multiport aware > > > > > > drivers. > > > > > > > > > > > > Don't set it before the guest_features are available; instead use > > > > > > set_status which is called by io to VIRTIO_PCI_STATUS with > > > > > > VIRTIO_CONFIG_S_DRIVER_OK by even older non multiport drivers. > > > > > > > > > > > > [Amit: Add comment, tweak summary] > > > > > > > > > > Logic also changed fron Alon's patch. Why? > > > > > > > > Yes, forgot to mention that here. > > > > > > > > > > Signed-off-by: Alon Levy <al...@redhat.com> > > > > > > Acked-by: Michael S. Tsirkin <m...@redhat.com> > > > > > > Signed-off-by: Amit Shah <amit.s...@redhat.com> > > > > > > --- > > > > > > hw/virtio-serial-bus.c | 29 +++++++++++++++++++++-------- > > > > > > 1 files changed, 21 insertions(+), 8 deletions(-) > > > > > > > > > > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > > > > > > index e22940e..796224b 100644 > > > > > > --- a/hw/virtio-serial-bus.c > > > > > > +++ b/hw/virtio-serial-bus.c > > > > > > @@ -528,6 +528,26 @@ static void set_config(VirtIODevice *vdev, > > > > > > const uint8_t *config_data) > > > > > > memcpy(&config, config_data, sizeof(config)); > > > > > > } > > > > > > > > > > > > +static void set_status(VirtIODevice *vdev, uint8_t status) > > > > > > +{ > > > > > > + VirtIOSerial *vser; > > > > > > + VirtIOSerialPort *port; > > > > > > + > > > > > > + vser = DO_UPCAST(VirtIOSerial, vdev, vdev); > > > > > > + port = find_port_by_id(vser, 0); > > > > > > + > > > > > > + if (port && !use_multiport(port->vser) > > > > > > + && (status & VIRTIO_CONFIG_S_DRIVER_OK)) { > > > > > > + /* > > > > > > + * Non-multiport guests won't be able to tell us guest > > > > > > + * open/close status. Such guests can only have a port at > > > > > > id > > > > > > + * 0, so set guest_connected for such ports as soon as > > > > > > guest > > > > > > + * is up. > > > > > > + */ > > > > > > + port->guest_connected = true; > > > > > > + } > > > > > > +} > > > > > > + > > > > > > > > > > Weird. Don't you want to set guest_connected = false when driver > > > > > is unloaded? > > > > > This is what Alon's patch did: > > > > > port->guest_connected = status & VIRTIO_CONFIG_S_DRIVER_OK; > > > > > > > > Setting guest_connected to false when driver is unloaded is something > > > > that's not done before this patch (and not noted in the commit log > > > > too). > > > > > > > > And that case isn't specific to just port 0 (in the !multiport case); > > > > all ports will have to be updated for such a change. > > > > > > > > Is that something we should do? It's obviously correct to do that. > > > > Will it affect anything? I doubt it. But needs a separate patch and > > > > discussion for that change. > > > > > > > Let's not add code to preserve a bug so we can have a discussion. Let's > > > have a discussion right here and fix it properly. > > > > It can be added to the series, but has to be a different patch. But I > > don't think we should hold up this fix because we found other bugs. > > Sure. So fix it for single port now and for multiport in a > separate patch. Alon's patch is a single line and it made sense, yours > differs in that you have added more code to implement a bug.
Disagree; it was not obvious in that patch; a single patch did more thing than one, commit log didn't note it (it was a side-effect), and the code was not easy to read. With the patch I will propose, there won't be any differentiation for the multiport and not multiport case. And for virtio-serial, multiport is the more important use-case (i.e. all recent guests have multiport support). > > > BTW guest_connected is not cleared on reset either - a bug too? > > > > Yes, I think we'll just have to scan the list of things that we want > > zero'ed at start. > > Not at start, at reset. Similar case. Amit