From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MumGJ-0001Bh-Ks for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:01:55 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MumGF-0001A3-OF for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:01:55 -0400 Received: from [199.232.76.173] (port=34237 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MumGF-00019y-Ii for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:01:51 -0400 Received: from mx1.redhat.com ([209.132.183.28]:29531) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MumGE-0003KY-Tp for qemu-devel@nongnu.org; Mon, 05 Oct 2009 08:01:51 -0400 Date: Mon, 5 Oct 2009 13:59:51 +0200 From: "Michael S. Tsirkin" Message-ID: <20091005115950.GA30799@redhat.com> References: <1254737223-16129-1-git-send-email-yamahata@valinux.co.jp> <1254737223-16129-21-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254737223-16129-21-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 20/23] pci: make bar update function aware of pci bridge. List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Isaku Yamahata Cc: qemu-devel@nongnu.org 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 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