From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MukYy-0004Ll-F8 for qemu-devel@nongnu.org; Mon, 05 Oct 2009 06:13:04 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MukYt-0004G4-Iv for qemu-devel@nongnu.org; Mon, 05 Oct 2009 06:13:03 -0400 Received: from [199.232.76.173] (port=53548 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MukYs-0004Fd-TI for qemu-devel@nongnu.org; Mon, 05 Oct 2009 06:12:58 -0400 Received: from mx1.redhat.com ([209.132.183.28]:12753) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MukYs-000814-AZ for qemu-devel@nongnu.org; Mon, 05 Oct 2009 06:12:58 -0400 Date: Mon, 5 Oct 2009 12:10:53 +0200 From: "Michael S. Tsirkin" Message-ID: <20091005101053.GC30081@redhat.com> References: <1254514577-11896-1-git-send-email-yamahata@valinux.co.jp> <1254514577-11896-22-git-send-email-yamahata@valinux.co.jp> <20091004105345.GO16887@redhat.com> <20091005094739.GG24813%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091005094739.GG24813%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 Mon, Oct 05, 2009 at 06:47:39PM +0900, Isaku Yamahata wrote: > On Sun, Oct 04, 2009 at 12:53:45PM +0200, Michael S. Tsirkin wrote: > > 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 > > Unfortunately PCI_ROM_ADDRESS1 is the one defined in pci_regs.h. > So PCI_ROM_ADDRESS1 is reasonable. You are right. Sorry about the noise. > > > > /* Size of the standard PCI config header */ > > > #define PCI_CONFIG_HEADER_SIZE 0x40 > > > > -- > yamahata