[Qemu-devel] Re: [PATCH 20/23] pci: make bar update function aware of pci bridge.

[Michael S. Tsirkin]

On Wed, Oct 14, 2009 at 12:14:17AM +0900, Isaku Yamahata wrote:
> On Sat, Oct 10, 2009 at 10:20:36PM +0200, Michael S. Tsirkin wrote:
> > > > > @@ -1123,10 +1144,23 @@ static void pci_bridge_write_config(PCIDevice 
> > > > > *d,
> > > > >                               uint32_t address, uint32_t val, int len)
> > > > >  {
> > > > >      PCIBridge *s = (PCIBridge *)d;
> > > > > +    PCIBus *bus = s->bus;
> > > > > +    struct pci_config_update update;
> > > > >  
> > > > > -    pci_default_write_config(d, address, val, len);
> > > > > -    s->bus->bus_num = d->config[PCI_SECONDARY_BUS];
> > > > > -    s->bus->sub_bus = d->config[PCI_SUBORDINATE_BUS];
> > > > > +    pci_write_config_init(&update, d, address, val, len);
> > > > > +    pci_write_config_update(&update);
> > > > > +    if (pci_config_changed(&update,
> > > > > +                           PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 
> > > > > 4) ||
> > > > > +        pci_config_changed_with_size(&update, PCI_ROM_ADDRESS1, 4) ||
> > > > > +        pci_config_changed_with_size(&update, PCI_COMMAND, 1)) {
> > > > > +        pci_update_mappings(d);
> > > > > +    }
> > > > 
> > > > This is wrong I think. You must also take into account memory
> > > > base/limit registers, and redo mapping when these change.
> > > > If you do, you should note several things:
> > > > - BARs for devices placed behind a bridge who's memory
> > > >   is outside the bridge base/limit are effectively disabled.
> > > 
> > > I deliberately didn't implemented bridge io/memory filtering
> > > because linux doesn't depend on it. I'll add some comment on this.
> > > Linux boots happily without filtering emulation.
> > > However Linux was confused without correct emulation of
> > > reading/writing to/from base/limit. so wmask needs to be initialized.
> > > 
> > > If other OS needs filtering emulation, it will be implemented.
> > > I don't know other OSes. Especially windows.
> > > I suppose Solaris doesn't because apb_pci.c uses bridge.
> > 
> > Filtering is the only way to disable e.g. prefetchable memory in a
> > bridge accoring to PCI spec, and I know that some BIOSes take advantage
> > of this. Frankly, I think we should just try and stick to spec.
> > It's not hard at all.
> 
> BIOS for real hardware? 
> At least pcbios and seabios doesn't,

At the moment. But why assume things?

> so the above doesn't make sense.
> Implementing filtering would be another story.

What do you call filtering?
I think that all you have to do is
1. scan all child devices on bridge header write,
        and call update
2. when updating regions, check parent bus base/limit registers,
   scanning all bars upwards, and unmap registers that move
   outside the range, map registers that move inside the range.

Where's the difficulty?

> -- 
> yamahata