diff mbox

Re: [PATCH v2 0/6] qdev reset refactoring and pci bus reset

Message ID 20101124052758.GA19320@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin Nov. 24, 2010, 5:27 a.m. UTC 
On Wed, Nov 24, 2010 at 11:37:25AM +0900, Isaku Yamahata wrote:
> On Tue, Nov 23, 2010 at 08:10:26PM +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 23, 2010 at 12:53:12AM +0200, Michael S. Tsirkin wrote:
> > > On Mon, Nov 22, 2010 at 07:43:37PM +0900, Isaku Yamahata wrote:
> > > > On Mon, Nov 22, 2010 at 09:54:02AM +0200, Michael S. Tsirkin wrote:
> > > > > On Fri, Nov 19, 2010 at 06:55:57PM +0900, Isaku Yamahata wrote:
> > > > > > Here is v2. I updated the comments, and dropped the pci qdev reset patch.
> > > > > > 
> > > > > > Patch description:
> > > > > > The goal of this patch series is to implement secondary bus reset
> > > > > > emulation in pci-to-pci bridge.
> > > > > > At first, this patch series refactors qdev reset,
> > > > > > and then cleans up pci bus reset. Lastly implements pci bridge control
> > > > > > secondary bus reset bit.
> > > > > > 
> > > > > > This patch series is for pci bus reset, which is ported
> > > > > > from the following repo.
> > > > > > git://repo.or.cz/qemu/aliguori.git qdev-refactor
> > > > > 
> > > > > I've put the series on my pci branch, tweaking patches 5 and 6 in the
> > > > > process.  Out of time to compile-tested only for now.
> > > > 
> > > > Thank you. The tweaking looks good.
> > > > Do you still want me to send another patch to add a comment on RST#?
> > > 
> > > OK, I got response, and yes, we need to fix code to avoid claiming
> > > transactions by devices on the secondary bus while secondary bus is in
> > > RST# for both pci and express.
> > 
> > I guess the following fixes it?
> 
> Almost. The following if claouse also needs modification.
> 
> commit 120fe782a198b787e13a9d4681afae8dc66dee50
> Author: "Michael S. Tsirkin" <mst@redhat.com>
> Date:   Wed Nov 24 11:34:59 2010 +0900
> 
>     pci: don't access bus while it's reset
>     
>     Devices on a bus should not respond while RST#
>     is asserted, which is controlled by PCI_BRIDGE_CTL_BUS_RESET.
>     Simply skip such a bus when propagating the configuration cycle.
>     
>     Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>     Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d02f980..e7df612 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1554,7 +1554,9 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  
>      /* try child bus */
>      if (!bus->parent_dev /* host pci bridge */ ||
> -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> +        (!(pci_get_word(bus->parent_dev->config + PCI_BRIDGE_CONTROL) &
> +           PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> +         bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
>           bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
>          for (; bus; bus = sec) {
>              QLIST_FOREACH(sec, &bus->child, sibling) {
> @@ -1562,8 +1564,11 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>                  if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
>                      return sec;
>                  }
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> +                if (pci_get_word(sec->parent_dev->config +
> +                                 PCI_BRIDGE_CONTROL) & PCI_BRIDGE_CTL_BUS_RESET
> +                    /* Don't walk the bus if it's reset. */ ||
> +                    bus_num < sec->parent_dev->config[PCI_SECONDARY_BUS] ||
> +                    sec->parent_dev->config[PCI_SUBORDINATE_BUS] < bus_num) {
>                      break;
>                  }
>              }

Right. So let's add an inline helper to avoid code duplication here?

pci: fix bus walk under secondary bus reset

Take into account secondary bus reset bit for
bus walk: devices behind a reset bus should not
respond to configuration cycles.

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



> -- 
> yamahata

Comments

Isaku Yamahata Nov. 24, 2010, 7:15 a.m. UTC | #1 
On Wed, Nov 24, 2010 at 07:27:58AM +0200, Michael S. Tsirkin wrote:
> Right. So let's add an inline helper to avoid code duplication here?
> 
> pci: fix bus walk under secondary bus reset
> 
> Take into account secondary bus reset bit for
> bus walk: devices behind a reset bus should not
> respond to configuration cycles.
> 
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> 
> 
> diff --git a/hw/pci.c b/hw/pci.c
> index d02f980..0c15b13 100644
> --- a/hw/pci.c
> +++ b/hw/pci.c
> @@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
>      }
>  }
>  
> +/* Whether a given bus number is in range of the secondary
> + * bus of the given bridge device. */
> +static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
> +{
> +    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
> +             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> +        dev->config[PCI_SECONDARY_BUS] < bus_num &&
> +        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
> +}
> +
>  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>  {
>      PCIBus *sec;
> @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
>          return bus;
>      }
>  
> +    /* Consider all bus numbers in range for the host pci bridge. */
> +    if (bus->parent_dev &&
> +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> +        return NULL;
> +    }
> +
>      /* try child bus */
> -    if (!bus->parent_dev /* host pci bridge */ ||
> -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> -        for (; bus; bus = sec) {
> -            QLIST_FOREACH(sec, &bus->child, sibling) {
> -                assert(sec->parent_dev);
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> -                    return sec;
> -                }
> -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> -                    break;
> -                }
> +    for (; bus; bus = sec) {
> +        QLIST_FOREACH(sec, &bus->child, sibling) {
> +            assert(sec->parent_dev);
> +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> +                return sec;
> +            }
> +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {

This condition should be "if (!pci_...)"


> +                break;
>              }
>          }
>      }
> 
> > -- 
> > yamahata
>
Michael S. Tsirkin Nov. 24, 2010, 10:59 a.m. UTC | #2 
On Wed, Nov 24, 2010 at 04:15:14PM +0900, Isaku Yamahata wrote:
> On Wed, Nov 24, 2010 at 07:27:58AM +0200, Michael S. Tsirkin wrote:
> > Right. So let's add an inline helper to avoid code duplication here?
> > 
> > pci: fix bus walk under secondary bus reset
> > 
> > Take into account secondary bus reset bit for
> > bus walk: devices behind a reset bus should not
> > respond to configuration cycles.
> > 
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > 
> > 
> > diff --git a/hw/pci.c b/hw/pci.c
> > index d02f980..0c15b13 100644
> > --- a/hw/pci.c
> > +++ b/hw/pci.c
> > @@ -1540,6 +1540,16 @@ void pci_bridge_update_mappings(PCIBus *b)
> >      }
> >  }
> >  
> > +/* Whether a given bus number is in range of the secondary
> > + * bus of the given bridge device. */
> > +static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
> > +{
> > +    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
> > +             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
> > +        dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > +        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
> > +}
> > +
> >  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >  {
> >      PCIBus *sec;
> > @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> >          return bus;
> >      }
> >  
> > +    /* Consider all bus numbers in range for the host pci bridge. */
> > +    if (bus->parent_dev &&
> > +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> > +        return NULL;
> > +    }
> > +
> >      /* try child bus */
> > -    if (!bus->parent_dev /* host pci bridge */ ||
> > -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> > -        for (; bus; bus = sec) {
> > -            QLIST_FOREACH(sec, &bus->child, sibling) {
> > -                assert(sec->parent_dev);
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > -                    return sec;
> > -                }
> > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> > -                    break;
> > -                }
> > +    for (; bus; bus = sec) {
> > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > +            assert(sec->parent_dev);
> > +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > +                return sec;
> > +            }
> > +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
> 
> This condition should be "if (!pci_...)"

Why? We are looking for a device on the given bus that claims the given
bus_num.  If we find one, break out of the inner loop and walk down to
the child.

> 
> > +                break;
> >              }
> >          }
> >      }
> > 
> > > -- 
> > > yamahata
> > 
> 
> -- 
> yamahata
Isaku Yamahata Nov. 24, 2010, 11:09 a.m. UTC | #3 
On Wed, Nov 24, 2010 at 12:59:43PM +0200, Michael S. Tsirkin wrote:
> > > @@ -1552,20 +1562,21 @@ PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > >          return bus;
> > >      }
> > >  
> > > +    /* Consider all bus numbers in range for the host pci bridge. */
> > > +    if (bus->parent_dev &&
> > > +        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
> > > +        return NULL;
> > > +    }
> > > +
> > >      /* try child bus */
> > > -    if (!bus->parent_dev /* host pci bridge */ ||
> > > -        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > > -         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
> > > -        for (; bus; bus = sec) {
> > > -            QLIST_FOREACH(sec, &bus->child, sibling) {
> > > -                assert(sec->parent_dev);
> > > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > > -                    return sec;
> > > -                }
> > > -                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
> > > -                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
> > > -                    break;
> > > -                }
> > > +    for (; bus; bus = sec) {
> > > +        QLIST_FOREACH(sec, &bus->child, sibling) {
> > > +            assert(sec->parent_dev);
> > > +            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
> > > +                return sec;
> > > +            }
> > > +            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
> > 
> > This condition should be "if (!pci_...)"
> 
> Why? We are looking for a device on the given bus that claims the given
> bus_num.  If we find one, break out of the inner loop and walk down to
> the child.

You're right. Sorry for noise.
diff mbox

Patch

diff --git a/hw/pci.c b/hw/pci.c
index d02f980..0c15b13 100644
--- a/hw/pci.c
+++ b/hw/pci.c
@@ -1540,6 +1540,16 @@  void pci_bridge_update_mappings(PCIBus *b)
     }
 }
 
+/* Whether a given bus number is in range of the secondary
+ * bus of the given bridge device. */
+static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num)
+{
+    return !(pci_get_word(dev->config + PCI_BRIDGE_CONTROL) &
+             PCI_BRIDGE_CTL_BUS_RESET) /* Don't walk the bus if it's reset. */ &&
+        dev->config[PCI_SECONDARY_BUS] < bus_num &&
+        bus_num <= dev->config[PCI_SUBORDINATE_BUS];
+}
+
 PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
 {
     PCIBus *sec;
@@ -1552,20 +1562,21 @@  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
         return bus;
     }
 
+    /* Consider all bus numbers in range for the host pci bridge. */
+    if (bus->parent_dev &&
+        !pci_secondary_bus_in_range(bus->parent_dev, bus_num)) {
+        return NULL;
+    }
+
     /* try child bus */
-    if (!bus->parent_dev /* host pci bridge */ ||
-        (bus->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
-         bus_num <= bus->parent_dev->config[PCI_SUBORDINATE_BUS])) {
-        for (; bus; bus = sec) {
-            QLIST_FOREACH(sec, &bus->child, sibling) {
-                assert(sec->parent_dev);
-                if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
-                    return sec;
-                }
-                if (sec->parent_dev->config[PCI_SECONDARY_BUS] < bus_num &&
-                    bus_num <= sec->parent_dev->config[PCI_SUBORDINATE_BUS]) {
-                    break;
-                }
+    for (; bus; bus = sec) {
+        QLIST_FOREACH(sec, &bus->child, sibling) {
+            assert(sec->parent_dev);
+            if (sec->parent_dev->config[PCI_SECONDARY_BUS] == bus_num) {
+                return sec;
+            }
+            if (pci_secondary_bus_in_range(sec->parent_dev, bus_num)) {
+                break;
             }
         }
     }