Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init

[Amit Shah]

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 <address@hidden>
> > 
> > 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 <address@hidden>
> > Acked-by: Michael S. Tsirkin <address@hidden>
> > Signed-off-by: Amit Shah <address@hidden>
> > ---
> >  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.

                Amit