From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MuOks-0003KL-0n for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:55:54 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MuOkn-0003HC-Na for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:55:53 -0400 Received: from [199.232.76.173] (port=45187 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MuOkn-0003Gx-Ck for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:55:49 -0400 Received: from mx1.redhat.com ([209.132.183.28]:58323) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MuOkm-000158-GW for qemu-devel@nongnu.org; Sun, 04 Oct 2009 06:55:48 -0400 Date: Sun, 4 Oct 2009 12:53:45 +0200 From: "Michael S. Tsirkin" Message-ID: <20091004105345.GO16887@redhat.com> References: <1254514577-11896-1-git-send-email-yamahata@valinux.co.jp> <1254514577-11896-22-git-send-email-yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1254514577-11896-22-git-send-email-yamahata@valinux.co.jp> Subject: [Qemu-devel] Re: [PATCH 21/25] 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 Sat, Oct 03, 2009 at 05:16:13AM +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. There are some typos in the comment > Signed-off-by: Isaku Yamahata > --- > hw/pci.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++---------- > hw/pci.h | 1 + > 2 files changed, 56 insertions(+), 11 deletions(-) > > diff --git a/hw/pci.c b/hw/pci.c > index ed9b785..b708d71 100644 > --- a/hw/pci.c > +++ b/hw/pci.c > @@ -498,6 +498,35 @@ int pci_unregister_device(PCIDevice *pci_dev) > return 0; > } > > +#define PCI_BAR_INVALID UINT32_MAX Just use -1 to report error. > +static uint32_t pci_bar_config_offset(PCIDevice *d, int region_num) Why uint32_t? I think it should be just int. Please add small comment documenting what function does. > +{ This functions does too much sanity checking, which is where all the complexity comes from. It's not the best place to do sanity checking: do it when adding region, if you must. So this should be simply: static int pci_bar_config_offset(PCIDevice *d, int region_num) { if (region_num != PCI_ROM_ADDRESS) return PCI_BASE_ADDRESS_0 + region_num * 4; return (d->config[PCI_HEADER_TYPE] & PCI_HEADER_TYPE_BRIDGE) ? PCI_ROM_ADDRESS : PCI_BRIDGE_ROM_ADDRESS; } > + switch (d->config[PCI_HEADER_TYPE] & ~PCI_HEADER_TYPE_MULTI_FUNCTION) { > + case PCI_HEADER_TYPE_NORMAL: > + /* BAR 0-5 and Expantion ROM*/ Nit: space before */ > + if (region_num < PCI_ROM_SLOT) { > + return PCI_BASE_ADDRESS_0 + region_num * 4; > + } else if (region_num == PCI_ROM_SLOT) { > + return PCI_ROM_ADDRESS; > + } > + break; > + case PCI_HEADER_TYPE_BRIDGE: > + /* BAR 0-1 and Expantion ROM */ typo > + if (region_num < 2) { what's 2? > + return PCI_BASE_ADDRESS_0 + region_num * 4; > + } else if (region_num == PCI_ROM_SLOT) { > + return PCI_ROM_ADDRESS1; > + } > + break; > + case PCI_HEADER_TYPE_CARDBUS: the last line is useless > + default: > + break; > + } > + fprintf(stderr, "ERROR: %s: unknow PCI config header type %d or bar %d\n", > + __func__, d->config[PCI_HEADER_TYPE], region_num); > + return PCI_BAR_INVALID; Caller does not seem to check PCI_BAR_INVALID. > +} > + > void pci_register_bar(PCIDevice *pci_dev, int region_num, > pcibus_t size, int type, > PCIMapIORegionFunc *map_func) > @@ -521,13 +550,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_64bit(r)) { > @@ -549,11 +576,9 @@ 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; > - } > + kill extra empty line > + config_ofs = pci_bar_config_offset(d, i); > + > if (r->size != 0) { > if (r->type & PCI_ADDRESS_SPACE_IO) { > if (cmd & PCI_COMMAND_IO) { > @@ -1133,10 +1158,29 @@ static void pci_bridge_write_config(PCIDevice *d, > uint32_t address, uint32_t val, int len) > { > PCIBridge *s = (PCIBridge *)d; > + PCIBus *bus = s->bus; > + uint8_t *orig = pci_write_config_init(d, address, len); > + > + pci_default_write_config_common(d, address, val, len); > > - 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]; > + if (pci_config_changed(orig, d->config, address, len, > + PCI_BASE_ADDRESS_0, PCI_BASE_ADDRESS_2 + 4) || > + pci_config_changed_with_size(orig, d->config, address, len, > + PCI_ROM_ADDRESS1, 4) || > + pci_config_changed_with_size(orig, d->config, address, len, > + PCI_COMMAND, 1)) { > + pci_update_mappings(d); > + } > + if (pci_config_changed_with_size(orig, d->config, address, len, > + PCI_SECONDARY_BUS, 1)) { > + bus->bus_num = d->config[PCI_SECONDARY_BUS]; > + } > + if (pci_config_changed_with_size(orig, d->config, address, len, > + PCI_SECONDARY_BUS, 1)) { > + bus->sub_bus = d->config[PCI_SUBORDINATE_BUS]; > + } So simply d->config[PCI_SUBORDINATE_BUS] != orig[PCI_SUBORDINATE_BUS] would be enough if we keep orig and config in sync as I suggested in my previous post. > + > + pci_write_config_done(orig); > } > > PCIBus *pci_find_bus(PCIBus *bus, int bus_num) > diff --git a/hw/pci.h b/hw/pci.h > index f19b7a8..645dacd 100644 > --- a/hw/pci.h > +++ b/hw/pci.h > @@ -174,6 +174,7 @@ static inline int pci_bar_is_64bit(const PCIIORegion *r) > > /* 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 */ PCI_BRIDGE_ROM_ADDRESS would be clearer. Also don't mix your own macros and ones imported from pci_regs.h > > /* Size of the standard PCI config header */ > #define PCI_CONFIG_HEADER_SIZE 0x40 > -- > 1.6.0.2 > >