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

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Oct 09, 2009 at 12:27:35PM +0900, Isaku Yamahata wrote:
> On Mon, Oct 05, 2009 at 01:59:51PM +0200, Michael S. Tsirkin wrote:
> > On Mon, Oct 05, 2009 at 07:07:00PM +0900, Isaku Yamahata wrote:
> > > header type of 01 has differenct BAR to type 00.
> > > It has only BAR0,1 and expantion rom whose offset address
> > > is different from type 00 one.
> > > 
> > > Signed-off-by: Isaku Yamahata <yamahata@valinux.co.jp>
> > 
> > The existing code is actually correct, and I think this breaks it.  I
> > think it's a good idea to add pci_bar_config_offset, though.
> > Split this up to a separate patch?
> > 
> > > ---
> > >  hw/pci.c |   56 +++++++++++++++++++++++++++++++++++++++++++++-----------
> > >  hw/pci.h |    2 ++
> > >  2 files changed, 47 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/hw/pci.c b/hw/pci.c
> > > index b8d2f8f..af864c6 100644
> > > --- a/hw/pci.c
> > > +++ b/hw/pci.c
> > > @@ -505,6 +505,33 @@ static inline int pci_bar_is_mem64(const PCIIORegion *r)
> > >          PCI_ADDRESS_SPACE_MEM_TYPE_64;
> > >  }
> > >  
> > > +/*
> > > + * return offset in pci configuration space for a given BAR of region_num.
> > > + * header type
> > > + * normal  = 0: bar 0-5 and rom
> > > + * bridge  = 1: bar 0,1 and rom
> > > + * cardbus = 2: bar 0
> > > + */
> > > +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num)
> > > +{
> > > +    uint8_t header_type =
> > > +        d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION;
> > > +
> > > +    assert((header_type == PCI_HEADER_TYPE_NORMAL &&
> > > +            ((0 <= region_num && region_num < 6) ||
> > > +             region_num == PCI_ROM_SLOT)) ||
> > 
> > && already binds stronger than ||, so don't put () around &&
> 
> gcc wants parens like
> hw/pci.c:531: error: suggest parentheses around && within ||
> note:qemu configure script enables -Werror by defaults.

I see.

> 
> > > +           (header_type == PCI_HEADER_TYPE_BRIDGE &&
> > > +            ((0 <= region_num && region_num < 2) ||
> > > +             region_num == PCI_ROM_SLOT)) ||
> > > +           (header_type == PCI_HEADER_TYPE_CARDBUS && region_num == 0));
> > 
> > This is not the best place to put assertions.
> > If you really need them, pci_register_bar is a better place.
> > 
> > > +
> > > +    if (region_num != PCI_ROM_ADDRESS)
> > > +        return PCI_BASE_ADDRESS_0 + region_num * 4;
> > 
> > BTW, how does it work for 64 bit regions?
> > 
> > > +
> > > +    return header_type == PCI_HEADER_TYPE_BRIDGE?
> > > +        PCI_ROM_ADDRESS1: PCI_ROM_ADDRESS;
> > 
> > space before :
> > 
> > > +}
> > > +
> > >  void pci_register_bar(PCIDevice *pci_dev, int region_num,
> > >                              pcibus_t size, int type,
> > >                              PCIMapIORegionFunc *map_func)
> > > @@ -528,13 +555,11 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num,
> > >      r->type = type;
> > >      r->map_func = map_func;
> > >  
> > > +    addr = pci_bar_config_offset(pci_dev, region_num);
> > >      wmask = ~(size - 1);
> > >      if (region_num == PCI_ROM_SLOT) {
> > > -        addr = 0x30;
> > >          /* ROM enable bit is writeable */
> > >          wmask |= PCI_ROM_ADDRESS_ENABLE;
> > > -    } else {
> > > -        addr = 0x10 + region_num * 4;
> > >      }
> > >      pci_set_long(pci_dev->config + addr, type);
> > >      if (pci_bar_is_mem64(r)) {
> > > @@ -556,11 +581,7 @@ static void pci_update_mappings(PCIDevice *d)
> > >      cmd = pci_get_word(d->config + PCI_COMMAND);
> > >      for(i = 0; i < PCI_NUM_REGIONS; i++) {
> > >          r = &d->io_regions[i];
> > > -        if (i == PCI_ROM_SLOT) {
> > > -            config_ofs = 0x30;
> > > -        } else {
> > > -            config_ofs = 0x10 + i * 4;
> > > -        }
> > > +        config_ofs = pci_bar_config_offset(d, i);
> > >          if (r->size != 0) {
> > >              if (r->type & PCI_ADDRESS_SPACE_IO) {
> > >                  if (cmd & PCI_COMMAND_IO) {
> > > @@ -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.

> > - Base addresses for pci devices overlap base/limit for
> >   bridge. For this reason, default_write_config
> >   was already doing the right thing, doing update
> >   when the bridge base/limit are changed.
> 
> I don't understand "the right thing" that the old code is doing
> and don't understand why the patch is wrong.

The patch is wrong in that it does not implement filtering.
As you show below, handling for PCI_ROM_ADDRESS1 is missing,
let's fix that, it's a single line change.

> BAR0 or BAR1 case:
> Okay. pci_default_write_config() properly update BAR0/1 mappings.
> 
> BAR2-5 case = memory/io base/limit case,:
> pci_default_write_config() calls pci_update_mapping(),
> but pci_update_mapping() does nothing because bar 2 - 5 must not registered.
> It's just overhead to do nothing.
> 
> PCI_ROM_ADDRESS = IO base/limit upper 16 case:
> If the bug is fixed, pci_default_write_config() calls pci_update_mapping().
> but it does nothing.
> 
> PCI_ROM_ADDRESS1 case
> And when PCI_ROM_ADDRESS1 (!= PCI_ROM_ADDRESS) is changed,
> pci_update_mapping() should be called. but pci_default_write_config() doesn't.
> 

Ah, I see. Let's just teach default_config_write to
call pci_update_mapping when PCI_ROM_ADDRESS1 is changed.
For regular devices this does no harm.

> 
> 
> > > +    if (pci_config_changed_with_size(&update, PCI_SECONDARY_BUS, 1)) {
> > > +        bus->bus_num = d->config[PCI_SECONDARY_BUS];
> > > +    }
> > > +    if (pci_config_changed_with_size(&update, PCI_SUBORDINATE_BUS, 1)) {
> > > +        bus->sub_bus = d->config[PCI_SUBORDINATE_BUS];
> > > +    }
> > >  }
> > >  
> > >  PCIBus *pci_find_bus(PCIBus *bus, int bus_num)
> > > diff --git a/hw/pci.h b/hw/pci.h
> > > index 37c2c23..1d45437 100644
> > > --- a/hw/pci.h
> > > +++ b/hw/pci.h
> > > @@ -118,6 +118,7 @@ typedef struct PCIIORegion {
> > >  #define  PCI_HEADER_TYPE_CARDBUS	2
> > >  #define  PCI_HEADER_TYPE_MULTI_FUNCTION 0x80
> > >  #define PCI_BASE_ADDRESS_0	0x10	/* 32 bits */
> > > +#define PCI_BASE_ADDRESS_2	0x18	/* 32 bits */
> > >  #define PCI_BASE_ADDRESS_5	0x24	/* 32 bits */
> > >  #define PCI_PRIMARY_BUS		0x18	/* Primary bus number */
> > >  #define PCI_SECONDARY_BUS	0x19	/* Secondary bus number */
> > > @@ -163,6 +164,7 @@ typedef struct PCIIORegion {
> > >  
> > >  /* Header type 1 (PCI-to-PCI bridges) */
> > >  #define PCI_SUBORDINATE_BUS     0x1a    /* Highest bus number behind the bridge */
> > > +#define PCI_ROM_ADDRESS1        0x38    /* Same as PCI_ROM_ADDRESS, but for htype 1 */
> > >  
> > >  /* Size of the standard PCI config header */
> > >  #define PCI_CONFIG_HEADER_SIZE 0x40
> > 
> 
> -- 
> yamahata