| Message ID | 20121217214336.GA11991@redhat.com |
|---|---|
| State | New |
| Headers | show |
Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto: > On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote: >> After discussion with mst on the topic of resetting virtio devices, >> here is a series that hopefully clarifies the semantics of bus and >> device resets. >> >> After this series, there are two kinds of resets: > > So just to clarify, what I proposed was this > (on top of my type safety patch). Then > all transports can call virtio_config_reset > when appropriate (e.g. when PA is set to 0). > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > diff --git a/hw/virtio.c b/hw/virtio.c > index f40a8c5..e65d7c8 100644 > --- a/hw/virtio.c > +++ b/hw/virtio.c > @@ -554,6 +554,14 @@ void virtio_reset(void *opaque) > } > } > > +/* Device-specific reset through virtio config space. > + * Reset virtio config and backend child devices if any. > + */ > +void virtio_config_reset(VirtIODevice *vdev) > +{ > + qdev_reset_all(vdev->binding_opaque); > +} Yes, I had understood. As I said, this is the wrong direction. Resetting happens from vdev->binding_opaque, it can just do qdev_reset_all(myself). Paolo > uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) > { > uint8_t val; > diff --git a/hw/virtio.h b/hw/virtio.h > index 7c17f7b..e2e57a4 100644 > --- a/hw/virtio.h > +++ b/hw/virtio.h > @@ -187,11 +187,12 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); > void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); > void virtio_set_status(VirtIODevice *vdev, uint8_t val); > void virtio_reset(void *opaque); > +void virtio_config_reset(VirtIODevice *vdev); > void virtio_update_irq(VirtIODevice *vdev); > int virtio_set_features(VirtIODevice *vdev, uint32_t val); > > void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, > DeviceState *opaque); > > /* Base devices. */ > typedef struct VirtIOBlkConf VirtIOBlkConf; > >
Il 18/12/2012 08:27, Paolo Bonzini ha scritto: > Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto: >> On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote: >>> After discussion with mst on the topic of resetting virtio devices, >>> here is a series that hopefully clarifies the semantics of bus and >>> device resets. >>> >>> After this series, there are two kinds of resets: >> >> So just to clarify, what I proposed was this >> (on top of my type safety patch). Then >> all transports can call virtio_config_reset >> when appropriate (e.g. when PA is set to 0). >> >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> >> >> diff --git a/hw/virtio.c b/hw/virtio.c >> index f40a8c5..e65d7c8 100644 >> --- a/hw/virtio.c >> +++ b/hw/virtio.c >> @@ -554,6 +554,14 @@ void virtio_reset(void *opaque) >> } >> } >> >> +/* Device-specific reset through virtio config space. >> + * Reset virtio config and backend child devices if any. >> + */ >> +void virtio_config_reset(VirtIODevice *vdev) >> +{ >> + qdev_reset_all(vdev->binding_opaque); >> +} > > Yes, I had understood. As I said, this is the wrong direction. > Resetting happens from vdev->binding_opaque, it can just do > qdev_reset_all(myself). ... besides, this only works if the reset callback of vdev->binding_opaque remembers to call virtio_reset (in the s390 bindings, it doesn't and this series fixes it). So IMO it is not only useless, it is also misleading. Paolo
On Tue, Dec 18, 2012 at 09:35:02AM +0100, Paolo Bonzini wrote: > Il 18/12/2012 08:27, Paolo Bonzini ha scritto: > > Il 17/12/2012 22:43, Michael S. Tsirkin ha scritto: > >> On Mon, Dec 17, 2012 at 05:24:35PM +0100, Paolo Bonzini wrote: > >>> After discussion with mst on the topic of resetting virtio devices, > >>> here is a series that hopefully clarifies the semantics of bus and > >>> device resets. > >>> > >>> After this series, there are two kinds of resets: > >> > >> So just to clarify, what I proposed was this > >> (on top of my type safety patch). Then > >> all transports can call virtio_config_reset > >> when appropriate (e.g. when PA is set to 0). > >> > >> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > >> > >> diff --git a/hw/virtio.c b/hw/virtio.c > >> index f40a8c5..e65d7c8 100644 > >> --- a/hw/virtio.c > >> +++ b/hw/virtio.c > >> @@ -554,6 +554,14 @@ void virtio_reset(void *opaque) > >> } > >> } > >> > >> +/* Device-specific reset through virtio config space. > >> + * Reset virtio config and backend child devices if any. > >> + */ > >> +void virtio_config_reset(VirtIODevice *vdev) > >> +{ > >> + qdev_reset_all(vdev->binding_opaque); > >> +} > > > > Yes, I had understood. As I said, this is the wrong direction. > > Resetting happens from vdev->binding_opaque, it can just do > > qdev_reset_all(myself). It can but it's the wrong thing for transport to know about. Let PCI worry about PCI things. This is not a transport specific thing so belongs in virtio.c > ... besides, this only works if the reset callback of > vdev->binding_opaque remembers to call virtio_reset (in the s390 > bindings, it doesn't and this series fixes it). That's a separate bug I think. > So IMO it is not only > useless, it is also misleading. > > Paolo
Il 18/12/2012 10:49, Michael S. Tsirkin ha scritto: >>>> +/* Device-specific reset through virtio config space. >>>> + * Reset virtio config and backend child devices if any. >>>> + */ >>>> +void virtio_config_reset(VirtIODevice *vdev) >>>> +{ >>>> + qdev_reset_all(vdev->binding_opaque); >>>> +} >>> >>> Yes, I had understood. As I said, this is the wrong direction. >>> Resetting happens from vdev->binding_opaque, it can just do >>> qdev_reset_all(myself). > > It can but it's the wrong thing for transport to know about. The transport provides an implementation of dc->reset, not virtio.c (e.g. virtio_pci_reset). Sure it knows what the effect of qdev_reset_all are on itself. > Let PCI worry about PCI things. This is not > a transport specific thing so belongs in virtio.c This _is_ a transport specific thing. Sure it will reset the virtio device (virtio_reset), but it will also reset things such as MSI-X vectors and VIRTIO_PCI_FLAG_BUS_MASTER_BUG. It doesn't belong in virtio.c. >> ... besides, this only works if the reset callback of >> vdev->binding_opaque remembers to call virtio_reset (in the s390 >> bindings, it doesn't and this series fixes it). > > That's a separate bug I think. Yes, I agree. Paolo
On Tue, Dec 18, 2012 at 12:40:36PM +0100, Paolo Bonzini wrote: > >> ... besides, this only works if the reset callback of > >> vdev->binding_opaque remembers to call virtio_reset (in the s390 > >> bindings, it doesn't and this series fixes it). > > > > That's a separate bug I think. > > Yes, I agree. > > Paolo Anyway, I think the only argument is about a bit of code duplication, we can fix it up later. Andreas Färber pointed out what looks like a buglet in this patchset, were you going to repost a fixed one?
diff --git a/hw/virtio.c b/hw/virtio.c index f40a8c5..e65d7c8 100644 --- a/hw/virtio.c +++ b/hw/virtio.c @@ -554,6 +554,14 @@ void virtio_reset(void *opaque) } } +/* Device-specific reset through virtio config space. + * Reset virtio config and backend child devices if any. + */ +void virtio_config_reset(VirtIODevice *vdev) +{ + qdev_reset_all(vdev->binding_opaque); +} + uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) { uint8_t val; diff --git a/hw/virtio.h b/hw/virtio.h index 7c17f7b..e2e57a4 100644 --- a/hw/virtio.h +++ b/hw/virtio.h @@ -187,11 +187,12 @@ uint16_t virtio_queue_vector(VirtIODevice *vdev, int n); void virtio_queue_set_vector(VirtIODevice *vdev, int n, uint16_t vector); void virtio_set_status(VirtIODevice *vdev, uint8_t val); void virtio_reset(void *opaque); +void virtio_config_reset(VirtIODevice *vdev); void virtio_update_irq(VirtIODevice *vdev); int virtio_set_features(VirtIODevice *vdev, uint32_t val); void virtio_bind_device(VirtIODevice *vdev, const VirtIOBindings *binding, DeviceState *opaque); /* Base devices. */ typedef struct VirtIOBlkConf VirtIOBlkConf;