From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1MwiSo-0002St-0T for qemu-devel@nongnu.org; Sat, 10 Oct 2009 16:22:50 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1MwiSi-0002OL-8o for qemu-devel@nongnu.org; Sat, 10 Oct 2009 16:22:48 -0400 Received: from [199.232.76.173] (port=56875 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1MwiSi-0002OI-35 for qemu-devel@nongnu.org; Sat, 10 Oct 2009 16:22:44 -0400 Received: from mx1.redhat.com ([209.132.183.28]:39093) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1MwiSh-00063F-JF for qemu-devel@nongnu.org; Sat, 10 Oct 2009 16:22:44 -0400 Date: Sat, 10 Oct 2009 22:20:36 +0200 From: "Michael S. Tsirkin" Message-ID: <20091010202036.GJ14275@redhat.com> References: <1254737223-16129-1-git-send-email-yamahata@valinux.co.jp> <1254737223-16129-21-git-send-email-yamahata@valinux.co.jp> <20091005115950.GA30799@redhat.com> <20091009032735.GO32367%yamahata@valinux.co.jp> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20091009032735.GO32367%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 Fri, Oct 09, 2009 at 12:27:35PM +0900, Isaku Yamahata wrote: > 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 > > > > 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. I see. > > > > + (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. Filtering is the only way to disable e.g. prefetchable memory in a bridge accoring to PCI spec, and I know that some BIOSes take advantage of this. Frankly, I think we should just try and stick to spec. It's not hard at all. > > - 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. The patch is wrong in that it does not implement filtering. As you show below, handling for PCI_ROM_ADDRESS1 is missing, let's fix that, it's a single line change. > 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. > Ah, I see. Let's just teach default_config_write to call pci_update_mapping when PCI_ROM_ADDRESS1 is changed. For regular devices this does no harm. > > > > > + 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