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

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

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

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

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