diff mbox

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

Message ID 20130109121027.GA18244@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Jan. 9, 2013, 12:10 p.m. UTC 
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>

---

Comments

Paolo Bonzini Jan. 9, 2013, 5:46 p.m. UTC | #1 
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
Anthony Liguori Jan. 9, 2013, 8:40 p.m. UTC | #2 
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
Paolo Bonzini Jan. 9, 2013, 9:22 p.m. UTC | #3 
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
Michael S. Tsirkin Jan. 9, 2013, 9:40 p.m. UTC | #4 
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?
Paolo Bonzini Jan. 10, 2013, 8:31 a.m. UTC | #5 
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
Michael S. Tsirkin Jan. 10, 2013, 11:32 a.m. UTC | #6 
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.
diff mbox

Patch

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 */