From patchwork Sun Sep 4 13:41:02 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Michael S. Tsirkin" X-Patchwork-Id: 113274 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [140.186.70.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id C2ED7B6F71 for ; Sun, 4 Sep 2011 23:40:29 +1000 (EST) Received: from localhost ([::1]:51787 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0Cvw-0004UB-2S for incoming@patchwork.ozlabs.org; Sun, 04 Sep 2011 09:40:24 -0400 Received: from eggs.gnu.org ([140.186.70.92]:53953) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0Cvo-0004Tk-3y for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:40:18 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1R0Cvm-0002N9-An for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:40:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44426) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1R0Cvl-0002N5-N7 for qemu-devel@nongnu.org; Sun, 04 Sep 2011 09:40:14 -0400 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id p84De7s6008233 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK); Sun, 4 Sep 2011 09:40:07 -0400 Received: from redhat.com (vpn-200-96.tlv.redhat.com [10.35.200.96]) by int-mx12.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id p84De1wT019857; Sun, 4 Sep 2011 09:40:02 -0400 Date: Sun, 4 Sep 2011 16:41:02 +0300 From: "Michael S. Tsirkin" To: Avi Kivity Message-ID: <20110904134101.GA27239@redhat.com> References: <20110826094254.GA6520@redhat.com> <4E59F359.9040506@redhat.com> <20110828114142.GC4875@redhat.com> <4E5A3E36.4010709@redhat.com> <20110828134203.GA6751@redhat.com> <4E5A485D.1020904@redhat.com> <20110904123006.GA23500@redhat.com> <4E6371DA.7070304@redhat.com> <20110904130159.GB23500@redhat.com> <4E63778A.6050002@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <4E63778A.6050002@redhat.com> User-Agent: Mutt/1.5.21 (2010-09-15) X-Scanned-By: MIMEDefang 2.68 on 10.5.11.25 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Cc: Kevin Wolf , Isaku Yamahata , qemu-devel@nongnu.org Subject: Re: [Qemu-devel] [PATCH] pci: add standard bridge device X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org Sender: qemu-devel-bounces+incoming=patchwork.ozlabs.org@nongnu.org On Sun, Sep 04, 2011 at 04:05:14PM +0300, Avi Kivity wrote: > It follows naturally: OK, so it seems the following is more or less what you suggest? I'm not sure I create/destroy subregions properly. Both the alias and the subregion get the same start value? Is the region name for debugging only? When does priority matter? In case of overlap? diff --git a/hw/pci.c b/hw/pci.c index 57ff7b1..56dfa18 100644 --- a/hw/pci.c +++ b/hw/pci.c @@ -889,7 +889,6 @@ void pci_register_bar(PCIDevice *pci_dev, int region_num, r = &pci_dev->io_regions[region_num]; r->addr = PCI_BAR_UNMAPPED; r->size = size; - r->filtered_size = size; r->type = type; r->memory = NULL; @@ -920,41 +919,6 @@ pcibus_t pci_get_bar_addr(PCIDevice *pci_dev, int region_num) return pci_dev->io_regions[region_num].addr; } -static void pci_bridge_filter(PCIDevice *d, pcibus_t *addr, pcibus_t *size, - uint8_t type) -{ - pcibus_t base = *addr; - pcibus_t limit = *addr + *size - 1; - PCIDevice *br; - - for (br = d->bus->parent_dev; br; br = br->bus->parent_dev) { - uint16_t cmd = pci_get_word(d->config + PCI_COMMAND); - - if (type & PCI_BASE_ADDRESS_SPACE_IO) { - if (!(cmd & PCI_COMMAND_IO)) { - goto no_map; - } - } else { - if (!(cmd & PCI_COMMAND_MEMORY)) { - goto no_map; - } - } - - base = MAX(base, pci_bridge_get_base(br, type)); - limit = MIN(limit, pci_bridge_get_limit(br, type)); - } - - if (base > limit) { - goto no_map; - } - *addr = base; - *size = limit - base + 1; - return; -no_map: - *addr = PCI_BAR_UNMAPPED; - *size = 0; -} - static pcibus_t pci_bar_address(PCIDevice *d, int reg, uint8_t type, pcibus_t size) { @@ -1024,7 +988,7 @@ static void pci_update_mappings(PCIDevice *d) { PCIIORegion *r; int i; - pcibus_t new_addr, filtered_size; + pcibus_t new_addr; for(i = 0; i < PCI_NUM_REGIONS; i++) { r = &d->io_regions[i]; @@ -1035,14 +999,8 @@ static void pci_update_mappings(PCIDevice *d) new_addr = pci_bar_address(d, i, r->type, r->size); - /* bridge filtering */ - filtered_size = r->size; - if (new_addr != PCI_BAR_UNMAPPED) { - pci_bridge_filter(d, &new_addr, &filtered_size, r->type); - } - /* This bar isn't changed */ - if (new_addr == r->addr && filtered_size == r->filtered_size) + if (new_addr == r->addr) continue; /* now do the real mapping */ @@ -1050,15 +1008,7 @@ static void pci_update_mappings(PCIDevice *d) memory_region_del_subregion(r->address_space, r->memory); } r->addr = new_addr; - r->filtered_size = filtered_size; if (r->addr != PCI_BAR_UNMAPPED) { - /* - * TODO: currently almost all the map funcions assumes - * filtered_size == size and addr & ~(size - 1) == addr. - * However with bridge filtering, they aren't always true. - * Teach them such cases, such that filtered_size < size and - * addr & (size - 1) != 0. - */ if (r->type & PCI_BASE_ADDRESS_SPACE_IO) { memory_region_add_subregion_overlap(r->address_space, r->addr, @@ -1576,22 +1526,6 @@ PCIDevice *pci_nic_init_nofail(NICInfo *nd, const char *default_model, return res; } -static void pci_bridge_update_mappings_fn(PCIBus *b, PCIDevice *d) -{ - pci_update_mappings(d); -} - -void pci_bridge_update_mappings(PCIBus *b) -{ - PCIBus *child; - - pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn); - - QLIST_FOREACH(child, &b->child, sibling) { - pci_bridge_update_mappings(child); - } -} - /* Whether a given bus number is in range of the secondary * bus of the given bridge device. */ static bool pci_secondary_bus_in_range(PCIDevice *dev, int bus_num) diff --git a/hw/pci.h b/hw/pci.h index 391217e..65e1568 100644 --- a/hw/pci.h +++ b/hw/pci.h @@ -90,7 +90,6 @@ typedef struct PCIIORegion { pcibus_t addr; /* current PCI mapping address. -1 means not mapped */ #define PCI_BAR_UNMAPPED (~(pcibus_t)0) pcibus_t size; - pcibus_t filtered_size; uint8_t type; MemoryRegion *memory; MemoryRegion *address_space; @@ -277,7 +276,6 @@ int pci_read_devaddr(Monitor *mon, const char *addr, int *domp, int *busp, void do_pci_info_print(Monitor *mon, const QObject *data); void do_pci_info(Monitor *mon, QObject **ret_data); -void pci_bridge_update_mappings(PCIBus *b); void pci_device_deassert_intx(PCIDevice *dev); diff --git a/hw/pci_bridge.c b/hw/pci_bridge.c index e0b339e..469dfe8 100644 --- a/hw/pci_bridge.c +++ b/hw/pci_bridge.c @@ -135,6 +135,75 @@ pcibus_t pci_bridge_get_limit(const PCIDevice *bridge, uint8_t type) return limit; } +static pcibus_t pci_bridge_get_size(const PCIDevice *bridge, uint8_t type) +{ + return pci_bridge_get_limit(bridge, type) >= + pci_bridge_get_base(bridge, type) ? + pci_bridge_get_limit(bridge, type) - + pci_bridge_get_base(bridge, type) + 1 : 0; +} + +static void pci_bridge_region_init(PCIBridge *br) +{ + PCIBus *sec_bus = &br->sec_bus; + PCIBus *parent = br->dev.bus; + memory_region_init_alias(sec_bus->alias_pref_mem, "pci_bridge_pref_mem", + sec_bus->address_space_mem, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH), + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH)); + memory_region_add_subregion_overlap(parent->address_space_mem, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_MEM_PREFETCH), + sec_bus->alias_pref_mem, 1); + memory_region_init_alias(sec_bus->alias_mem, "pci_bridge_memory", + sec_bus->address_space_mem, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY), + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY)); + memory_region_add_subregion_overlap(parent->address_space_mem, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_MEMORY), + sec_bus->alias_mem, 1); + memory_region_init_alias(sec_bus->alias_io, "pci_bridge_io", + sec_bus->address_space_io, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO), + pci_bridge_get_size(&br->dev, PCI_BASE_ADDRESS_SPACE_IO)); + memory_region_add_subregion_overlap(parent->address_space_io, + pci_bridge_get_base(&br->dev, PCI_BASE_ADDRESS_SPACE_IO), + sec_bus->alias_io, 1); +} + +static void pci_bridge_region_cleanup(PCIBridge *br) +{ + PCIBus *sec_bus = &br->sec_bus; + PCIBus *parent = br->dev.bus; + memory_region_del_subregion(parent->address_space_mem, + sec_bus->alias_pref_mem); + memory_region_destroy(sec_bus->alias_pref_mem); + memory_region_del_subregion(parent->address_space_mem, + sec_bus->alias_mem); + memory_region_destroy(sec_bus->alias_mem); + memory_region_del_subregion(parent->address_space_io, + sec_bus->alias_io); + memory_region_destroy(sec_bus->alias_io); +} + +static void pci_bridge_update_mappings(PCIBridge *br) +{ + /* TODO: this doesn't handle the case of one VCPU + * updating the bridge while another accesses an unaffected + * region. To fix we'll need new memory region APIs. */ + pci_bridge_region_cleanup(br); + pci_bridge_region_init(br); + +#if 0 + TODO: do we need to propagate updates to child buses? + + pci_for_each_device_under_bus(b, pci_bridge_update_mappings_fn); + + QLIST_FOREACH(child, &b->child, sibling) { + pci_bridge_update_mappings(child); + } +#endif +} + /* default write_config function for PCI-to-PCI bridge */ void pci_bridge_write_config(PCIDevice *d, uint32_t address, uint32_t val, int len) @@ -151,7 +220,7 @@ void pci_bridge_write_config(PCIDevice *d, /* memory base/limit, prefetchable base/limit and io base/limit upper 16 */ ranges_overlap(address, len, PCI_MEMORY_BASE, 20)) { - pci_bridge_update_mappings(&s->sec_bus); + pci_bridge_update_mappings(s); } newctl = pci_get_word(d->config + PCI_BRIDGE_CONTROL); @@ -247,9 +316,14 @@ int pci_bridge_initfn(PCIDevice *dev) sec_bus->parent_dev = dev; sec_bus->map_irq = br->map_irq; /* TODO: use memory API to perform memory filtering. */ - sec_bus->address_space_mem = parent->address_space_mem; - sec_bus->address_space_io = parent->address_space_io; - + sec_bus->address_space_mem = g_new(MemoryRegion, 1); + memory_region_init(sec_bus->address_space_mem, "pci_pridge_pci", INT64_MAX); + sec_bus->address_space_io = g_new(MemoryRegion, 1); + memory_region_init(sec_bus->address_space_io, "pci_bridge_io", 65536); + sec_bus->alias_pref_mem = g_new(MemoryRegion, 1); + sec_bus->alias_mem = g_new(MemoryRegion, 1); + sec_bus->alias_io = g_new(MemoryRegion, 1); + pci_bridge_region_init(br); QLIST_INIT(&sec_bus->child); QLIST_INSERT_HEAD(&parent->child, sec_bus, sibling); return 0; @@ -259,8 +333,17 @@ int pci_bridge_initfn(PCIDevice *dev) int pci_bridge_exitfn(PCIDevice *pci_dev) { PCIBridge *s = DO_UPCAST(PCIBridge, dev, pci_dev); + PCIBus *sec_bus = &s->sec_bus; assert(QLIST_EMPTY(&s->sec_bus.child)); QLIST_REMOVE(&s->sec_bus, sibling); + pci_bridge_region_cleanup(s); + g_free(sec_bus->alias_pref_mem); + g_free(sec_bus->alias_mem); + g_free(sec_bus->alias_io); + memory_region_destroy(sec_bus->address_space_mem); + g_free(sec_bus->address_space_mem); + memory_region_destroy(sec_bus->address_space_io); + g_free(sec_bus->address_space_io); /* qbus_free() is called automatically by qdev_free() */ return 0; } diff --git a/hw/pci_internals.h b/hw/pci_internals.h index c7fd23d..578c8d2 100644 --- a/hw/pci_internals.h +++ b/hw/pci_internals.h @@ -27,6 +27,9 @@ struct PCIBus { target_phys_addr_t mem_base; MemoryRegion *address_space_mem; MemoryRegion *address_space_io; + MemoryRegion *alias_pref_mem; + MemoryRegion *alias_mem; + MemoryRegion *alias_io; QLIST_HEAD(, PCIBus) child; /* this will be replaced by qdev later */ QLIST_ENTRY(PCIBus) sibling;/* this will be replaced by qdev later */