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

[Isaku Yamahata]

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 <address@hidden>
> 
> 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.


> > +           (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.


> - 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.

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.




> > +    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