[Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

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:

1) device-level reset is the kind of reset that you get with a register
write on the device.  It will clear interrupts and DMAs among other things,
but not any bus-level state, for example it will not clear PCI BARs and
other configuration space data.  It is done with qdev_reset_all.

2) bus-level reset is the kind of reset that you get with a register
write on the device that exports the bus (including triggering a device-level
reset on the device that exports the bus).  It will do a device-level
reset on the child, but also clear bus-level state such as PCI BARs and
other configuration space data.  It can be triggered for all devices
on a bus with qbus_reset_all.  There is still no API for a bus-level
reset of a single device (like PCI FLR), this can be added later.

Patches 1-5 are miscellaneous fixes to the reset paths.

Patches 6-8 introduce qbus_reset_all and uses it.

Patches 9-12 switch qdev_reset_all and qbus_reset_all from pre-order
to post-order, and document the resulting semantics.

Finally, patches 13-15 adjust the virtio implementations to use qdev
device-level reset more extensively.

Paolo Bonzini (15):
  qdev: do not reset a device until the parent has been initialized
  intel-hda: do not reset codecs from intel_hda_reset
  pci: clean up resetting of IRQs
  virtio-pci: reset device before PCI layer
  virtio-s390: add a reset function to virtio-s390 devices
  qdev: add qbus_reset_all
  pci: do not export pci_bus_reset
  lsi: use qbus_reset_all to reset SCSI bus
  qdev: allow both pre- and post-order vists in qdev walking functions
  qdev: switch reset to post-order
  qdev: remove device_reset
  qdev: document reset semantics
  virtio-pci: reset all qbuses too when writing to the status field
  virtio-s390: reset all qbuses too when writing to the status field
  virtio-serial: do not perform bus reset by hand

 hw/intel-hda.c         |  1 -
 hw/lsi53c895a.c        |  7 +----
 hw/pci.c               | 16 ++++------
 hw/pci.h               |  1 -
 hw/pci_bridge.c        |  2 +-
 hw/qdev-core.h         | 83 ++++++++++++++++++++++++++++++++++++++++++--------
 hw/qdev.c              | 70 +++++++++++++++++++++++++++---------------
 hw/s390-virtio-bus.c   | 16 +++++++++-
 hw/virtio-pci.c        | 27 +++++++---------
 hw/virtio-serial-bus.c | 19 +++---------
 10 files changed, 156 insertions(+), 86 deletions(-)

-- 
1.8.0.2

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

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);
+}
+
 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;

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

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;
> 
> 

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

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

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Sorry for breaking threading... I've fixed that bug.

FWIW, the reason this got notified but other patches haven't is that as
of this afternoon, this patch no longer applies but it did apply before my earlier
commits.  So it's the first patch series to get identified by my scripts.

Regards,

Anthony Liguori

Anthony Liguori <aliguori@us.ibm.com> writes:

> Hi,
>
> This is an automated message generated from the QEMU Patches.
> Thank you for submitting this patch.  This patch no longer applies to qemu.git.
>
> This may have occurred due to:
>  
>   1) Changes in mainline requiring your patch to be rebased and re-tested.
>
>   2) Sending the mail using a tool other than git-send-email.  Please use
>      git-send-email to send patches to QEMU.
>
>   3) Basing this patch off of a branch that isn't tracking the QEMU
>      master branch.  If that was done purposefully, please include the name
>      of the tree in the subject line in the future to prevent this message.
>
>      For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature"
>
>   4) You no longer wish for this patch to be applied to QEMU.  No additional
>      action is required on your part.
>
> Nacked-by: QEMU Patches <aliguori@us.ibm.com>
>
> Below is the output from git-am:
>
>     Applying: qdev: do not reset a device until the parent has been initialized
>     Applying: intel-hda: do not reset codecs from intel_hda_reset
>     Applying: pci: clean up resetting of IRQs
>     Using index info to reconstruct a base tree...
>     A	hw/pci.c
>     Falling back to patching base and 3-way merge...
>     Auto-merging hw/pci/pci.c
>     Applying: virtio-pci: reset device before PCI layer
>     Using index info to reconstruct a base tree...
>     M	hw/virtio-pci.c
>     Falling back to patching base and 3-way merge...
>     Auto-merging hw/virtio-pci.c
>     CONFLICT (content): Merge conflict in hw/virtio-pci.c
>     Failed to merge in the changes.
>     Patch failed at 0004 virtio-pci: reset device before PCI layer
>     The copy of the patch that failed is found in:
>        /home/aliguori/patches/qemu.git/.git/rebase-apply/patch
>     When you have resolved this problem run "git am --resolved".
>     If you would prefer to skip this patch, instead run "git am --skip".
>     To restore the original branch and stop patching run "git am --abort".

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Hi,

This is an automated message generated from the QEMU Patches.
Thank you for submitting this patch.  This patch no longer applies to qemu.git.

This may have occurred due to:
 
  1) Changes in mainline requiring your patch to be rebased and re-tested.

  2) Sending the mail using a tool other than git-send-email.  Please use
     git-send-email to send patches to QEMU.

  3) Basing this patch off of a branch that isn't tracking the QEMU
     master branch.  If that was done purposefully, please include the name
     of the tree in the subject line in the future to prevent this message.

     For instance: "[PATCH block-next 1/10] qcow3: add fancy new feature"

  4) You no longer wish for this patch to be applied to QEMU.  No additional
     action is required on your part.

Nacked-by: QEMU Patches <aliguori@us.ibm.com>

Below is the output from git-am:

    Applying: qdev: do not reset a device until the parent has been initialized
    Applying: intel-hda: do not reset codecs from intel_hda_reset
    Applying: pci: clean up resetting of IRQs
    Using index info to reconstruct a base tree...
    A	hw/pci.c
    Falling back to patching base and 3-way merge...
    Auto-merging hw/pci/pci.c
    Applying: virtio-pci: reset device before PCI layer
    Using index info to reconstruct a base tree...
    M	hw/virtio-pci.c
    Falling back to patching base and 3-way merge...
    Auto-merging hw/virtio-pci.c
    CONFLICT (content): Merge conflict in hw/virtio-pci.c
    Failed to merge in the changes.
    Patch failed at 0004 virtio-pci: reset device before PCI layer
    The copy of the patch that failed is found in:
       /home/aliguori/patches/qemu.git/.git/rebase-apply/patch
    When you have resolved this problem run "git am --resolved".
    If you would prefer to skip this patch, instead run "git am --skip".
    To restore the original branch and stop patching run "git am --abort".

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

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?

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> 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:
>
> 1) device-level reset is the kind of reset that you get with a register
> write on the device.  It will clear interrupts and DMAs among other things,
> but not any bus-level state, for example it will not clear PCI BARs and
> other configuration space data.  It is done with qdev_reset_all.
>
> 2) bus-level reset is the kind of reset that you get with a register
> write on the device that exports the bus (including triggering a device-level
> reset on the device that exports the bus).  It will do a device-level
> reset on the child, but also clear bus-level state such as PCI BARs and
> other configuration space data.  It can be triggered for all devices
> on a bus with qbus_reset_all.  There is still no API for a bus-level
> reset of a single device (like PCI FLR), this can be added later.

I don't really understand this dual abstraction.  I suspect it's
overgeneralizing something that's the result of poor modeling.

Very often with PCI devices, you have an otherwise independent chipset
embedded on a PCI card  There's a clear separate between the chipset and
the card.

It may be possible to poke the chipset directly to do a reset and that
may only affect state that's on the chipset but not any card-specific
state.

But this has nothing to do with busses, this is just about
encapsulation.  IOW, what you have is:

E1000 is-a PCIDevice
  has-a E1000 chipset
  PCIDevice::reset()
    -> calls chipset->reset()

But with the right register write, chipset->reset() can be called w/o
PCIDevice::reset().  But again, this has nothing to do with busses.

What I'm missing with this series is what problem are we trying to
solve?  I don't think we model reset correctly today because I don't
think there's a single notion of reset.

I think reset really ought to just be a bus level concept with
individual implementations for each bus.

Regards,

Anthony Liguori

>
> Patches 1-5 are miscellaneous fixes to the reset paths.
>
> Patches 6-8 introduce qbus_reset_all and uses it.
>
> Patches 9-12 switch qdev_reset_all and qbus_reset_all from pre-order
> to post-order, and document the resulting semantics.
>
> Finally, patches 13-15 adjust the virtio implementations to use qdev
> device-level reset more extensively.
>
> Paolo Bonzini (15):
>   qdev: do not reset a device until the parent has been initialized
>   intel-hda: do not reset codecs from intel_hda_reset
>   pci: clean up resetting of IRQs
>   virtio-pci: reset device before PCI layer
>   virtio-s390: add a reset function to virtio-s390 devices
>   qdev: add qbus_reset_all
>   pci: do not export pci_bus_reset
>   lsi: use qbus_reset_all to reset SCSI bus
>   qdev: allow both pre- and post-order vists in qdev walking functions
>   qdev: switch reset to post-order
>   qdev: remove device_reset
>   qdev: document reset semantics
>   virtio-pci: reset all qbuses too when writing to the status field
>   virtio-s390: reset all qbuses too when writing to the status field
>   virtio-serial: do not perform bus reset by hand
>
>  hw/intel-hda.c         |  1 -
>  hw/lsi53c895a.c        |  7 +----
>  hw/pci.c               | 16 ++++------
>  hw/pci.h               |  1 -
>  hw/pci_bridge.c        |  2 +-
>  hw/qdev-core.h         | 83 ++++++++++++++++++++++++++++++++++++++++++--------
>  hw/qdev.c              | 70 +++++++++++++++++++++++++++---------------
>  hw/s390-virtio-bus.c   | 16 +++++++++-
>  hw/virtio-pci.c        | 27 +++++++---------
>  hw/virtio-serial-bus.c | 19 +++---------
>  10 files changed, 156 insertions(+), 86 deletions(-)
>
> -- 
> 1.8.0.2

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Paolo Bonzini <pbonzini@redhat.com> writes:
>> 1) device-level reset is the kind of reset that you get with a register
>> write on the device.  It will clear interrupts and DMAs among other things,
>> but not any bus-level state, for example it will not clear PCI BARs and
>> other configuration space data.  It is done with qdev_reset_all.

This isn't really right -- often writing the register on the device will
reset some things but not the whole device state the way a qdev
reset does. qdev reset (to the extent it's modelling anything) is more
like yanking power to the device and reapplying it.

>> 2) bus-level reset is the kind of reset that you get with a register
>> write on the device that exports the bus (including triggering a device-level
>> reset on the device that exports the bus).  It will do a device-level
>> reset on the child, but also clear bus-level state such as PCI BARs and
>> other configuration space data.  It can be triggered for all devices
>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>> reset of a single device (like PCI FLR), this can be added later.

This doesn't sound very plausible: when would you do a bus level
reset anyway?

> I don't really understand this dual abstraction.  I suspect it's
> overgeneralizing something that's the result of poor modeling.

Agreed.

> What I'm missing with this series is what problem are we trying to
> solve?  I don't think we model reset correctly today because I don't
> think there's a single notion of reset.

Also agreed.

> I think reset really ought to just be a bus level concept with
> individual implementations for each bus.

I'm not sure I really agree here, especially since QOM/qdev are
moving away from the idea that there is a single bus tree and every
device is on a single bus. It's quite common for a bus to include a
reset signal but not all device reset is handled by a signal on a bus.

If we want to model reset properly we should model actual reset
lines (and/or power-cycling). If we don't care we can continue with
whatever fudge we like :-)

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Paolo Bonzini <pbonzini@redhat.com> writes:
>>> 1) device-level reset is the kind of reset that you get with a register
>>> write on the device.  It will clear interrupts and DMAs among other things,
>>> but not any bus-level state, for example it will not clear PCI BARs and
>>> other configuration space data.  It is done with qdev_reset_all.
>
> This isn't really right -- often writing the register on the device will
> reset some things but not the whole device state the way a qdev
> reset does. qdev reset (to the extent it's modelling anything) is more
> like yanking power to the device and reapplying it.
>
>>> 2) bus-level reset is the kind of reset that you get with a register
>>> write on the device that exports the bus (including triggering a device-level
>>> reset on the device that exports the bus).  It will do a device-level
>>> reset on the child, but also clear bus-level state such as PCI BARs and
>>> other configuration space data.  It can be triggered for all devices
>>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>>> reset of a single device (like PCI FLR), this can be added later.
>
> This doesn't sound very plausible: when would you do a bus level
> reset anyway?
>
>> I don't really understand this dual abstraction.  I suspect it's
>> overgeneralizing something that's the result of poor modeling.
>
> Agreed.
>
>> What I'm missing with this series is what problem are we trying to
>> solve?  I don't think we model reset correctly today because I don't
>> think there's a single notion of reset.
>
> Also agreed.
>
>> I think reset really ought to just be a bus level concept with
>> individual implementations for each bus.
>
> I'm not sure I really agree here, especially since QOM/qdev are
> moving away from the idea that there is a single bus tree and every
> device is on a single bus.

I don't mean a BusState level concept, I mean a PCIBus concept.

There is clearly such a thing as a PCI bus reset.  In fact, there are
multiple types of PCI bus resets.  There should be a PCIBus method that
calls out to PCIDevices on the bus.

But that isn't something that should be fitted into generalized to a
BusState::reset method.

> It's quite common for a bus to include a
> reset signal but not all device reset is handled by a signal on a bus.

Agreed.

>
> If we want to model reset properly we should model actual reset
> lines (and/or power-cycling). If we don't care we can continue with
> whatever fudge we like :-)

Yes, and that's basically what qemu_system_reset() is.  Of course, we
model it like everything is directly connected to a single power source
which is true 99% of the time.  That's why we've gotten away with it for
so long.

Regards,

Anthony Liguori

> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 7 January 2013 20:20, Anthony Liguori <aliguori@us.ibm.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>> I think reset really ought to just be a bus level concept with
>>> individual implementations for each bus.
>>
>> I'm not sure I really agree here, especially since QOM/qdev are
>> moving away from the idea that there is a single bus tree and every
>> device is on a single bus.
>
> I don't mean a BusState level concept, I mean a PCIBus concept.
>
> There is clearly such a thing as a PCI bus reset.  In fact, there are
> multiple types of PCI bus resets.  There should be a PCIBus method that
> calls out to PCIDevices on the bus.
>
> But that isn't something that should be fitted into generalized to a
> BusState::reset method.

Ah, I see what you mean now -- yes, definitely.

>> If we want to model reset properly we should model actual reset
>> lines (and/or power-cycling). If we don't care we can continue with
>> whatever fudge we like :-)
>
> Yes, and that's basically what qemu_system_reset() is.  Of course, we
> model it like everything is directly connected to a single power source
> which is true 99% of the time.  That's why we've gotten away with it for
> so long.

I think the bit that's most creaky here is what you do about devices
that want to assert outgoing irq/whatever lines immediately on powerup.

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 7 January 2013 20:20, Anthony Liguori <aliguori@us.ibm.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> On 7 January 2013 19:10, Anthony Liguori <aliguori@us.ibm.com> wrote:
>>>> I think reset really ought to just be a bus level concept with
>>>> individual implementations for each bus.
>>>
>>> I'm not sure I really agree here, especially since QOM/qdev are
>>> moving away from the idea that there is a single bus tree and every
>>> device is on a single bus.
>>
>> I don't mean a BusState level concept, I mean a PCIBus concept.
>>
>> There is clearly such a thing as a PCI bus reset.  In fact, there are
>> multiple types of PCI bus resets.  There should be a PCIBus method that
>> calls out to PCIDevices on the bus.
>>
>> But that isn't something that should be fitted into generalized to a
>> BusState::reset method.
>
> Ah, I see what you mean now -- yes, definitely.
>
>>> If we want to model reset properly we should model actual reset
>>> lines (and/or power-cycling). If we don't care we can continue with
>>> whatever fudge we like :-)
>>
>> Yes, and that's basically what qemu_system_reset() is.  Of course, we
>> model it like everything is directly connected to a single power source
>> which is true 99% of the time.  That's why we've gotten away with it for
>> so long.
>
> I think the bit that's most creaky here is what you do about devices
> that want to assert outgoing irq/whatever lines immediately on
> powerup.

This isn't so much a reset problem as a consequence of stateless IRQ
lines.  If IRQ lines were stateful, this problem wouldn't exist.

And yes, I will post Pins one day...

Regards,

Anthony Liguori

>
> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

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.

what started all this is a bug that affects virtio scsi.
Since there's still a lot of argument about reworking reset,
how about we fix the bug first, and attempt to rework reset
on top?
Will help stable etc as well.

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 07/01/2013 20:10, Anthony Liguori ha scritto:
> Paolo Bonzini <pbonzini@redhat.com> writes:
> 
>> 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:
>>
>> 1) device-level reset is the kind of reset that you get with a register
>> write on the device.  It will clear interrupts and DMAs among other things,
>> but not any bus-level state, for example it will not clear PCI BARs and
>> other configuration space data.  It is done with qdev_reset_all.
>>
>> 2) bus-level reset is the kind of reset that you get with a register
>> write on the device that exports the bus (including triggering a device-level
>> reset on the device that exports the bus).  It will do a device-level
>> reset on the child, but also clear bus-level state such as PCI BARs and
>> other configuration space data.  It can be triggered for all devices
>> on a bus with qbus_reset_all.  There is still no API for a bus-level
>> reset of a single device (like PCI FLR), this can be added later.
> 
> I don't really understand this dual abstraction.  I suspect it's
> overgeneralizing something that's the result of poor modeling.

It's possible.  I'll move the SCSI bus away from qdev reset.
Anthony/Michael, can you help doing the same with PCIDevice?  And
perhaps Peter and Andreas with sysbus?

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 10:33:37AM +0100, Paolo Bonzini wrote:
> Il 07/01/2013 20:10, Anthony Liguori ha scritto:
> > Paolo Bonzini <pbonzini@redhat.com> writes:
> > 
> >> 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:
> >>
> >> 1) device-level reset is the kind of reset that you get with a register
> >> write on the device.  It will clear interrupts and DMAs among other things,
> >> but not any bus-level state, for example it will not clear PCI BARs and
> >> other configuration space data.  It is done with qdev_reset_all.
> >>
> >> 2) bus-level reset is the kind of reset that you get with a register
> >> write on the device that exports the bus (including triggering a device-level
> >> reset on the device that exports the bus).  It will do a device-level
> >> reset on the child, but also clear bus-level state such as PCI BARs and
> >> other configuration space data.  It can be triggered for all devices
> >> on a bus with qbus_reset_all.  There is still no API for a bus-level
> >> reset of a single device (like PCI FLR), this can be added later.
> > 
> > I don't really understand this dual abstraction.  I suspect it's
> > overgeneralizing something that's the result of poor modeling.
> 
> It's possible.  I'll move the SCSI bus away from qdev reset.
> Anthony/Michael, can you help doing the same with PCIDevice?  And
> perhaps Peter and Andreas with sysbus?
> 
> Paolo

I'm not sure what would you like to change with PCIDevice.

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> > It's possible.  I'll move the SCSI bus away from qdev reset.
> > Anthony/Michael, can you help doing the same with PCIDevice?  And
> > perhaps Peter and Andreas with sysbus?
> 
> I'm not sure what would you like to change with PCIDevice.

Replace the DeviceState reset method with one in PCIDevice, and call it
from the PCI bus reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> > > It's possible.  I'll move the SCSI bus away from qdev reset.
> > > Anthony/Michael, can you help doing the same with PCIDevice?  And
> > > perhaps Peter and Andreas with sysbus?
> > 
> > I'm not sure what would you like to change with PCIDevice.
> 
> Replace the DeviceState reset method with one in PCIDevice, and call it
> from the PCI bus reset.
> 
> Paolo

You are talking about the call to qdev_reset_all(&dev->qdev)
in pci_device_reset, and you want to detect, there, that device is a bridge
and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
for the secondary bus?

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 12:09, Michael S. Tsirkin ha scritto:
> On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
>> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
>>>> It's possible.  I'll move the SCSI bus away from qdev reset.
>>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>> perhaps Peter and Andreas with sysbus?
>>>
>>> I'm not sure what would you like to change with PCIDevice.
>>
>> Replace the DeviceState reset method with one in PCIDevice, and call it
>> from the PCI bus reset.
>>
>> Paolo
> 
> You are talking about the call to qdev_reset_all(&dev->qdev)
> in pci_device_reset

Yes.

> , and you want to detect, there, that device is a bridge
> and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
> for the secondary bus?

The bridge would call pci_bus_reset for its secondary bus in the
PCIDevice reset method.

I.e. all bus navigation would have to be explicit in the reset
callbacks.  I'm going to do that for the SCSI HBAs.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 12:12:55PM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 12:09, Michael S. Tsirkin ha scritto:
> > On Wed, Jan 09, 2013 at 11:53:00AM +0100, Paolo Bonzini wrote:
> >> Il 09/01/2013 11:22, Michael S. Tsirkin ha scritto:
> >>>> It's possible.  I'll move the SCSI bus away from qdev reset.
> >>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
> >>>> perhaps Peter and Andreas with sysbus?
> >>>
> >>> I'm not sure what would you like to change with PCIDevice.
> >>
> >> Replace the DeviceState reset method with one in PCIDevice, and call it
> >> from the PCI bus reset.
> >>
> >> Paolo
> > 
> > You are talking about the call to qdev_reset_all(&dev->qdev)
> > in pci_device_reset
> 
> Yes.
> 
> > , and you want to detect, there, that device is a bridge
> > and replace qdev_reset_all(&dev->qdev) with cast and call to pci_bus_reset
> > for the secondary bus?
> 
> The bridge would call pci_bus_reset for its secondary bus in the
> PCIDevice reset method.
> 
> I.e. all bus navigation would have to be explicit in the reset
> callbacks.  I'm going to do that for the SCSI HBAs.
> 
> Paolo

OK you mean like this this? Completely untested - if you
and Anthony think we need this change please let me know
and I'll test and repost.

-->

pci: make secondary bus reset explicit

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

---

diff --git a/hw/pci/pci.c b/hw/pci/pci.c
index 94840c4..ad81040 100644
--- a/hw/pci/pci.c
+++ b/hw/pci/pci.c
@@ -172,7 +172,7 @@ void pci_device_reset(PCIDevice *dev)
 {
     int r;
 
-    qdev_reset_all(&dev->qdev);
+    device_reset(&dev->qdev);
 
     dev->irq_state = 0;
     pci_update_irq_status(dev);
diff --git a/hw/pci/pci_bridge.c b/hw/pci/pci_bridge.c
index 995842a..a08b479 100644
--- a/hw/pci/pci_bridge.c
+++ b/hw/pci/pci_bridge.c
@@ -258,10 +258,12 @@ void pci_bridge_disable_base_limit(PCIDevice *dev)
     pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0);
 }
 
-/* reset bridge specific configuration registers */
+/* reset bridge specific configuration registers.
+ * Propagate reset on the secondary bus. */
 void pci_bridge_reset(DeviceState *qdev)
 {
     PCIDevice *dev = PCI_DEVICE(qdev);
+    PCIBridge *br = DO_UPCAST(PCIBridge, dev, dev);
     uint8_t *conf = dev->config;
 
     conf[PCI_PRIMARY_BUS] = 0;
@@ -295,6 +297,8 @@ void pci_bridge_reset(DeviceState *qdev)
     pci_set_long(conf + PCI_PREF_LIMIT_UPPER32, 0);
 
     pci_set_word(conf + PCI_BRIDGE_CONTROL, 0);
+
+    pci_bus_reset(&br->sec_bus);
 }
 
 /* default qdev initialization function for PCI-to-PCI bridge */

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 13:10, Michael S. Tsirkin ha scritto:
> OK you mean like this this? Completely untested - if you
> and Anthony think we need this change please let me know
> and I'll test and repost.
> 
> -->
> 
> pci: make secondary bus reset explicit
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Yes, but this can only be done after looking at all PCI devices that
have buses below (of which the bridge is a special case).  And Anthony
also metioned using a new method of PCIDevice instead of device_reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 09/01/2013 13:10, Michael S. Tsirkin ha scritto:
>> OK you mean like this this? Completely untested - if you
>> and Anthony think we need this change please let me know
>> and I'll test and repost.
>> 
>> -->
>> 
>> pci: make secondary bus reset explicit
>> 
>> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> Yes, but this can only be done after looking at all PCI devices that
> have buses below (of which the bridge is a special case).  And Anthony
> also metioned using a new method of PCIDevice instead of device_reset.

Sorry, what's the bug here?  Is this a reset bug with cold reset or with
a form of warm reset?

Regards,

Anthony Liguori

>
> Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 21:40, Anthony Liguori ha scritto:
>> > Yes, but this can only be done after looking at all PCI devices that
>> > have buses below (of which the bridge is a special case).  And Anthony
>> > also metioned using a new method of PCIDevice instead of device_reset.
> Sorry, what's the bug here?  Is this a reset bug with cold reset or with
> a form of warm reset?

Warm reset (virtio status register reset) of virtio-scsi wasn't
propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
now I'll do it manually in the HBA.  I'm fine with that as long as all
buses move away from device_reset.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Wed, Jan 09, 2013 at 10:22:58PM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 21:40, Anthony Liguori ha scritto:
> >> > Yes, but this can only be done after looking at all PCI devices that
> >> > have buses below (of which the bridge is a special case).  And Anthony
> >> > also metioned using a new method of PCIDevice instead of device_reset.
> > Sorry, what's the bug here?  Is this a reset bug with cold reset or with
> > a form of warm reset?
> 
> Warm reset (virtio status register reset) of virtio-scsi wasn't
> propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> now I'll do it manually in the HBA.  I'm fine with that as long as all
> buses move away from device_reset.
> 
> Paolo

You also pointed out some bug related to status register handling in virtio
on s390, right?

-- 
MST

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 09/01/2013 22:40, Michael S. Tsirkin ha scritto:
> > Warm reset (virtio status register reset) of virtio-scsi wasn't
> > propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> > now I'll do it manually in the HBA.  I'm fine with that as long as all
> > buses move away from device_reset.
> 
> You also pointed out some bug related to status register handling in virtio
> on s390, right?

Yes, that would be a separate patch.  A large part of this series can
also be kept.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Thu, Jan 10, 2013 at 09:31:05AM +0100, Paolo Bonzini wrote:
> Il 09/01/2013 22:40, Michael S. Tsirkin ha scritto:
> > > Warm reset (virtio status register reset) of virtio-scsi wasn't
> > > propagated down to the SCSI bus.  I fixed it using qdev_reset_all, but
> > > now I'll do it manually in the HBA.  I'm fine with that as long as all
> > > buses move away from device_reset.
> > 
> > You also pointed out some bug related to status register handling in virtio
> > on s390, right?
> 
> Yes, that would be a separate patch.  A large part of this series can
> also be kept.
> 
> Paolo

Let's start with bugfixes, no need to bundle them in a series.

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 9 January 2013 09:33, Paolo Bonzini <pbonzini@redhat.com> wrote:
> It's possible.  I'll move the SCSI bus away from qdev reset.
> Anthony/Michael, can you help doing the same with PCIDevice?  And
> perhaps Peter and Andreas with sysbus?

What does it even mean to reset a sysbus? Do we do it anywhere?
(it looks like vl.c does, just as a shortcut so memory mapped devices
get their reset hooks called?)

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 12:46, Peter Maydell ha scritto:
>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>> > perhaps Peter and Andreas with sysbus?
> What does it even mean to reset a sysbus? Do we do it anywhere?
> (it looks like vl.c does, just as a shortcut so memory mapped devices
> get their reset hooks called?)

Yes, exactly.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 11:47, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 12:46, Peter Maydell ha scritto:
>>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>> > perhaps Peter and Andreas with sysbus?
>> What does it even mean to reset a sysbus? Do we do it anywhere?
>> (it looks like vl.c does, just as a shortcut so memory mapped devices
>> get their reset hooks called?)

So how should it work instead? I kind of feel like all qdev devices should
get their reset hook called on machine reset, regardless of bus [since it's
modelling power cycling the whole system], but would that break
something?

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>> >> get their reset hooks called?)
> So how should it work instead? I kind of feel like all qdev devices should
> get their reset hook called on machine reset, regardless of bus [since it's
> modelling power cycling the whole system], but would that break
> something?

It's just an implementation detail.  Right now we have a common
callback.  The idea is to give each bus its own callback.  In the case
of sysbus it would just call a method; for PCI it would reset some
configuration and then call a method; for SCSI there is no need to call
a method at all; and so on.

In addition, navigating the qdev tree should be explicit in the methods.
 It will not happen anymore via the "magic" qdev_reset_all.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>> >> get their reset hooks called?)
>> So how should it work instead? I kind of feel like all qdev devices should
>> get their reset hook called on machine reset, regardless of bus [since it's
>> modelling power cycling the whole system], but would that break
>> something?
>
> It's just an implementation detail.  Right now we have a common
> callback.  The idea is to give each bus its own callback.  In the case
> of sysbus it would just call a method; for PCI it would reset some
> configuration and then call a method; for SCSI there is no need to call
> a method at all; and so on.

But machine reset shouldn't call bus specific PCI or SCSI reset
methods -- we've just effectively yanked the power to the VM
so everything should just reset as if it was freshly constructed.

A bus-specific reset method would be for buses where the bus
itself has some sort of guest-triggerable reset (by prodding the
chipset, for instance).

> In addition, navigating the qdev tree should be explicit in the methods.
>  It will not happen anymore via the "magic" qdev_reset_all.

*Something* has to say "call reset for every qdev object in the
system", surely?

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 13:31, Peter Maydell ha scritto:
> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>>>>>>> It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>>>>>>> Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>>>>>>> perhaps Peter and Andreas with sysbus?
>>>>>>> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>>>>> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>>>>> get their reset hooks called?)
>>> So how should it work instead? I kind of feel like all qdev devices should
>>> get their reset hook called on machine reset, regardless of bus [since it's
>>> modelling power cycling the whole system], but would that break
>>> something?
>>
>> It's just an implementation detail.  Right now we have a common
>> callback.  The idea is to give each bus its own callback.  In the case
>> of sysbus it would just call a method; for PCI it would reset some
>> configuration and then call a method; for SCSI there is no need to call
>> a method at all; and so on.
> 
> But machine reset shouldn't call bus specific PCI or SCSI reset
> methods -- we've just effectively yanked the power to the VM
> so everything should just reset as if it was freshly constructed.

Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.

> A bus-specific reset method would be for buses where the bus
> itself has some sort of guest-triggerable reset (by prodding the
> chipset, for instance).

Yes.

>> In addition, navigating the qdev tree should be explicit in the methods.
>>  It will not happen anymore via the "magic" qdev_reset_all.
> 
> *Something* has to say "call reset for every qdev object in the
> system", surely?

It will call it on sysbus, which will call it on every sysbus child.
Devices that have a child bus will propagate it further down.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Peter Maydell]

On 10 January 2013 12:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 10/01/2013 13:31, Peter Maydell ha scritto:
>> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>> It's just an implementation detail.  Right now we have a common
>>> callback.  The idea is to give each bus its own callback.  In the case
>>> of sysbus it would just call a method; for PCI it would reset some
>>> configuration and then call a method; for SCSI there is no need to call
>>> a method at all; and so on.
>>
>> But machine reset shouldn't call bus specific PCI or SCSI reset
>> methods -- we've just effectively yanked the power to the VM
>> so everything should just reset as if it was freshly constructed.
>
> Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.

qdev reset is cold reset, not warm reset.

>> A bus-specific reset method would be for buses where the bus
>> itself has some sort of guest-triggerable reset (by prodding the
>> chipset, for instance).
>
> Yes.
>
>>> In addition, navigating the qdev tree should be explicit in the methods.
>>>  It will not happen anymore via the "magic" qdev_reset_all.
>>
>> *Something* has to say "call reset for every qdev object in the
>> system", surely?
>
> It will call it on sysbus, which will call it on every sysbus child.
> Devices that have a child bus will propagate it further down.

This doesn't work when we get rid of the idea that every qdev
device is attached to a tree of buses.

(At this point in the conversation I'm pretty confused about exactly
what we're trying to do :-))

-- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 14:01, Peter Maydell ha scritto:
> On 10 January 2013 12:45, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 13:31, Peter Maydell ha scritto:
>>> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>> It's just an implementation detail.  Right now we have a common
>>>> callback.  The idea is to give each bus its own callback.  In the case
>>>> of sysbus it would just call a method; for PCI it would reset some
>>>> configuration and then call a method; for SCSI there is no need to call
>>>> a method at all; and so on.
>>>
>>> But machine reset shouldn't call bus specific PCI or SCSI reset
>>> methods -- we've just effectively yanked the power to the VM
>>> so everything should just reset as if it was freshly constructed.
>>
>> Yes, but we do not have a way to do that, so QEMU resorts to a warm reset.
> 
> qdev reset is cold reset, not warm reset.

Whatever. :)  It resorts to a reset (as if it was freshly constructed)
instead of _really_ doing a fresh construction.

>>> A bus-specific reset method would be for buses where the bus
>>> itself has some sort of guest-triggerable reset (by prodding the
>>> chipset, for instance).
>>
>> Yes.
>>
>>>> In addition, navigating the qdev tree should be explicit in the methods.
>>>>  It will not happen anymore via the "magic" qdev_reset_all.
>>>
>>> *Something* has to say "call reset for every qdev object in the
>>> system", surely?
>>
>> It will call it on sysbus, which will call it on every sysbus child.
>> Devices that have a child bus will propagate it further down.
> 
> This doesn't work when we get rid of the idea that every qdev
> device is attached to a tree of buses.

At least for now, devices that should be bus-free are sysbus devices,
aren't they?

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Anthony Liguori]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> Il 10/01/2013 12:59, Peter Maydell ha scritto:
>>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
>>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
>>>>>>> >>> > perhaps Peter and Andreas with sysbus?
>>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
>>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
>>>>> >> get their reset hooks called?)
>>> So how should it work instead? I kind of feel like all qdev devices should
>>> get their reset hook called on machine reset, regardless of bus [since it's
>>> modelling power cycling the whole system], but would that break
>>> something?
>>
>> It's just an implementation detail.  Right now we have a common
>> callback.  The idea is to give each bus its own callback.  In the case
>> of sysbus it would just call a method; for PCI it would reset some
>> configuration and then call a method; for SCSI there is no need to call
>> a method at all; and so on.
>
> But machine reset shouldn't call bus specific PCI or SCSI reset
> methods -- we've just effectively yanked the power to the VM
> so everything should just reset as if it was freshly constructed.
>
> A bus-specific reset method would be for buses where the bus
> itself has some sort of guest-triggerable reset (by prodding the
> chipset, for instance).

The challenge is how we go from what we have to what we want.

Right now we have DeviceState::reset.  This is used both as a soft and
hard reset.

What I would propose is that we:

  s/DeviceState::reset/DeviceState::hard_reset/g

Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
call soft_reset() instead of hard_reset.

Over time, it would be great if we could find a way to implement
hard_reset in terms of device destruction/recreation but we're not there
yet.

I think the reset/hard_reset rename can be done via sed mostly.

Would this solve the bug that you're trying to fix Michael/Paolo?

Regards,

Anthony Liguori


>> In addition, navigating the qdev tree should be explicit in the methods.
>>  It will not happen anymore via the "magic" qdev_reset_all.
>
> *Something* has to say "call reset for every qdev object in the
> system", surely?
>
> -- PMM

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Paolo Bonzini]

Il 10/01/2013 15:14, Anthony Liguori ha scritto:
> What I would propose is that we:
> 
>   s/DeviceState::reset/DeviceState::hard_reset/g
> 
> Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
> call soft_reset() instead of hard_reset.
> 
> Over time, it would be great if we could find a way to implement
> hard_reset in terms of device destruction/recreation but we're not there
> yet.
> 
> I think the reset/hard_reset rename can be done via sed mostly.
> 
> Would this solve the bug that you're trying to fix Michael/Paolo?

I can fix the bug easily just in virtio-scsi.  I don't want anywone to
trip on it again due to false expectations about reset, though.

Paolo

Re: [Qemu-devel] [PATCH 00/15] qdev: make reset semantics more clear and consistent, reset qbuses under virtio devices

[Michael S. Tsirkin]

On Thu, Jan 10, 2013 at 08:14:43AM -0600, Anthony Liguori wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
> 
> > On 10 January 2013 12:12, Paolo Bonzini <pbonzini@redhat.com> wrote:
> >> Il 10/01/2013 12:59, Peter Maydell ha scritto:
> >>>>>>> >>> > It's possible.  I'll move the SCSI bus away from qdev reset.
> >>>>>>> >>> > Anthony/Michael, can you help doing the same with PCIDevice?  And
> >>>>>>> >>> > perhaps Peter and Andreas with sysbus?
> >>>>> >> What does it even mean to reset a sysbus? Do we do it anywhere?
> >>>>> >> (it looks like vl.c does, just as a shortcut so memory mapped devices
> >>>>> >> get their reset hooks called?)
> >>> So how should it work instead? I kind of feel like all qdev devices should
> >>> get their reset hook called on machine reset, regardless of bus [since it's
> >>> modelling power cycling the whole system], but would that break
> >>> something?
> >>
> >> It's just an implementation detail.  Right now we have a common
> >> callback.  The idea is to give each bus its own callback.  In the case
> >> of sysbus it would just call a method; for PCI it would reset some
> >> configuration and then call a method; for SCSI there is no need to call
> >> a method at all; and so on.
> >
> > But machine reset shouldn't call bus specific PCI or SCSI reset
> > methods -- we've just effectively yanked the power to the VM
> > so everything should just reset as if it was freshly constructed.
> >
> > A bus-specific reset method would be for buses where the bus
> > itself has some sort of guest-triggerable reset (by prodding the
> > chipset, for instance).
> 
> The challenge is how we go from what we have to what we want.
> 
> Right now we have DeviceState::reset.  This is used both as a soft and
> hard reset.
> 
> What I would propose is that we:
> 
>   s/DeviceState::reset/DeviceState::hard_reset/g
> 
> Then introduce PCIDevice::soft_reset.  We can convert the PCI layer to
> call soft_reset() instead of hard_reset.
> 
> Over time, it would be great if we could find a way to implement
> hard_reset in terms of device destruction/recreation but we're not there
> yet.
> 
> I think the reset/hard_reset rename can be done via sed mostly.
> 
> Would this solve the bug that you're trying to fix Michael/Paolo?
> 
> Regards,
> 
> Anthony Liguori

I don't think we need a rename to fix a specific bug.
The bugs Paolo found can be fixed in virtio and virtio-scsi.

> 
> >> In addition, navigating the qdev tree should be explicit in the methods.
> >>  It will not happen anymore via the "magic" qdev_reset_all.
> >
> > *Something* has to say "call reset for every qdev object in the
> > system", surely?
> >
> > -- PMM