[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init
From: |
Amit Shah |
Subject: |
Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init |
Date: |
Tue, 24 Apr 2012 16:44:10 +0530 |
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 <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.
> >
> 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.
> 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.
<Better to use a non-zeroed memory alloc than zero'ed, we can at least
be careful to set and reset values explicitly rather than invite such
bugs.>
Amit
- [Qemu-devel] [PULL] virtio-serial: fix probing for features before driver init, Amit Shah, 2012/04/24
- [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Amit Shah, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Michael S. Tsirkin, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Amit Shah, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Michael S. Tsirkin, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init,
Amit Shah <=
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Michael S. Tsirkin, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Amit Shah, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Michael S. Tsirkin, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Andreas Färber, 2012/04/24
- Re: [Qemu-devel] [PATCH 1/1] virtio-serial-bus: fix guest_connected init before driver init, Alon Levy, 2012/04/24