Patchwork [00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

login
register
mail settings
Submitter Michael S. Tsirkin
Date Dec. 17, 2012, 9:43 p.m.
Message ID <20121217214336.GA11991@redhat.com>
Download mbox | patch
Permalink /patch/207003/
State New
Headers show

Comments

Michael S. Tsirkin - Dec. 17, 2012, 9:43 p.m.
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>
Paolo Bonzini - Dec. 18, 2012, 7:27 a.m.
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;
> 
>
Paolo Bonzini - Dec. 18, 2012, 8:35 a.m.
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
Michael S. Tsirkin - Dec. 18, 2012, 9:49 a.m.
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
Paolo Bonzini - Dec. 18, 2012, 11:40 a.m.
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
Michael S. Tsirkin - Jan. 7, 2013, 5:46 p.m.
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?

Patch

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;