| Message ID | 20100706112327.GA20108@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Jul 06, 2010 at 02:23:27PM +0300, Michael S. Tsirkin wrote: > bridge config write should trigger updates > on the secondary bus. never on the primary bus. > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Compile-tested only. > Isaku Yamahata, could you review this please? > You wrote the code, and you seem to have some bridged setups. The code looks good. Should PCIBridge::bus be renamed to something like PCIBridge::secondary_bus (or sec_bus for short) in order to avoid confusion? The redundant local variable, secondary_bus, was deliberately introduced to emphasize that it's a secondary bus. And I was confused by that. Anyway such a change should be done by another patch. > > hw/pci.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 926cf63..011d83e 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1513,7 +1513,9 @@ static void pci_bridge_write_config(PCIDevice *d, > /* memory base/limit, prefetchable base/limit and > io base/limit upper 16 */ > ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { > - pci_bridge_update_mappings(d->bus); > + PCIBridge *s = container_of(d, PCIBridge, dev); > + PCIBus *secondary_bus = &s->bus; > + pci_bridge_update_mappings(secondary_bus); > } > } > > -- > 1.7.2.rc0.14.g41c1c >
On Tue, Jul 6, 2010 at 11:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > bridge config write should trigger updates > on the secondary bus. never on the primary bus. If this is true, shouldn't updates happen on all buses from secondary to subordinate? Do we know which of these are immediately below primary bus? > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > --- > > Compile-tested only. > Isaku Yamahata, could you review this please? > You wrote the code, and you seem to have some bridged setups. > > hw/pci.c | 4 +++- > 1 files changed, 3 insertions(+), 1 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index 926cf63..011d83e 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -1513,7 +1513,9 @@ static void pci_bridge_write_config(PCIDevice *d, > /* memory base/limit, prefetchable base/limit and > io base/limit upper 16 */ > ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { > - pci_bridge_update_mappings(d->bus); > + PCIBridge *s = container_of(d, PCIBridge, dev); > + PCIBus *secondary_bus = &s->bus; > + pci_bridge_update_mappings(secondary_bus); > } > } > > -- > 1.7.2.rc0.14.g41c1c > >
On Wed, Jul 07, 2010 at 05:31:39PM +0000, Blue Swirl wrote: > On Tue, Jul 6, 2010 at 11:23 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > > bridge config write should trigger updates > > on the secondary bus. never on the primary bus. > > If this is true, shouldn't updates happen on all buses from secondary > to subordinate? Do we know which of these are immediately below > primary bus? pci_bridge_update_mappings does this already. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Compile-tested only. > > Isaku Yamahata, could you review this please? > > You wrote the code, and you seem to have some bridged setups. > > > > hw/pci.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 926cf63..011d83e 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -1513,7 +1513,9 @@ static void pci_bridge_write_config(PCIDevice *d, > > /* memory base/limit, prefetchable base/limit and > > io base/limit upper 16 */ > > ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { > > - pci_bridge_update_mappings(d->bus); > > + PCIBridge *s = container_of(d, PCIBridge, dev); > > + PCIBus *secondary_bus = &s->bus; > > + pci_bridge_update_mappings(secondary_bus); > > } > > } > > > > -- > > 1.7.2.rc0.14.g41c1c > > > >
On Wed, Jul 07, 2010 at 11:04:51AM +0900, Isaku Yamahata wrote: > On Tue, Jul 06, 2010 at 02:23:27PM +0300, Michael S. Tsirkin wrote: > > bridge config write should trigger updates > > on the secondary bus. never on the primary bus. > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > --- > > > > Compile-tested only. > > Isaku Yamahata, could you review this please? > > You wrote the code, and you seem to have some bridged setups. > > The code looks good. > > Should PCIBridge::bus be renamed to something like > PCIBridge::secondary_bus (or sec_bus for short) in order > to avoid confusion? > The redundant local variable, secondary_bus, was deliberately > introduced to emphasize that it's a secondary bus. > And I was confused by that. > Anyway such a change should be done by another patch. Sure. Send a patch. > > > > hw/pci.c | 4 +++- > > 1 files changed, 3 insertions(+), 1 deletions(-) > > > > diff --git a/hw/pci.c b/hw/pci.c > > index 926cf63..011d83e 100644 > > --- a/hw/pci.c > > +++ b/hw/pci.c > > @@ -1513,7 +1513,9 @@ static void pci_bridge_write_config(PCIDevice *d, > > /* memory base/limit, prefetchable base/limit and > > io base/limit upper 16 */ > > ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { > > - pci_bridge_update_mappings(d->bus); > > + PCIBridge *s = container_of(d, PCIBridge, dev); > > + PCIBus *secondary_bus = &s->bus; > > + pci_bridge_update_mappings(secondary_bus); > > } > > } > > > > -- > > 1.7.2.rc0.14.g41c1c > > > > -- > yamahata
diff --git a/hw/pci.c b/hw/pci.c index 926cf63..011d83e 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -1513,7 +1513,9 @@ static void pci_bridge_write_config(PCIDevice *d, /* memory base/limit, prefetchable base/limit and io base/limit upper 16 */ ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { - pci_bridge_update_mappings(d->bus); + PCIBridge *s = container_of(d, PCIBridge, dev); + PCIBus *secondary_bus = &s->bus; + pci_bridge_update_mappings(secondary_bus); } }
bridge config write should trigger updates on the secondary bus. never on the primary bus. Signed-off-by: Michael S. Tsirkin <mst@redhat.com> --- Compile-tested only. Isaku Yamahata, could you review this please? You wrote the code, and you seem to have some bridged setups. hw/pci.c | 4 +++- 1 files changed, 3 insertions(+), 1 deletions(-)