From patchwork Wed Oct 17 15:17:44 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Avi Kivity X-Patchwork-Id: 192075 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.gnu.org (lists.gnu.org [208.118.235.17]) (using TLSv1 with cipher AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 4A1FD2C0092 for ; Thu, 18 Oct 2012 02:18:51 +1100 (EST) Received: from localhost ([::1]:56788 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOVOT-0005t9-B1 for incoming@patchwork.ozlabs.org; Wed, 17 Oct 2012 11:18:49 -0400 Received: from eggs.gnu.org ([208.118.235.92]:58017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOVNd-0004Dw-11 for qemu-devel@nongnu.org; Wed, 17 Oct 2012 11:18:05 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1TOVNb-0006dT-Al for qemu-devel@nongnu.org; Wed, 17 Oct 2012 11:17:56 -0400 Received: from mx1.redhat.com ([209.132.183.28]:27088) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1TOVNb-0006aH-1P for qemu-devel@nongnu.org; Wed, 17 Oct 2012 11:17:55 -0400 Received: from int-mx11.intmail.prod.int.phx2.redhat.com (int-mx11.intmail.prod.int.phx2.redhat.com [10.5.11.24]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q9HFHpqv020881 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 17 Oct 2012 11:17:51 -0400 Received: from s01.tlv.redhat.com (s01.tlv.redhat.com [10.35.255.8]) by int-mx11.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id q9HFHmlc002552; Wed, 17 Oct 2012 11:17:50 -0400 From: Avi Kivity To: qemu-devel@nongnu.org Date: Wed, 17 Oct 2012 17:17:44 +0200 Message-Id: <1350487065-32022-2-git-send-email-avi@redhat.com> In-Reply-To: <1350487065-32022-1-git-send-email-avi@redhat.com> References: <1350487065-32022-1-git-send-email-avi@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.24 X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 209.132.183.28 Subject: [Qemu-devel] [PATCH 1/2] i440fx: avoid destroying memory regions within a transaction 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 Calling memory_region_destroy() within a transaction is illegal, since the memory API is allowed to continue to dispatch to a region until the transaction commits. 440fx does that however when managing PAM registers. This bug is benign, since the regions are all aliases (which the memory core tends to throw anyway), and since we don't do concurrent dispatch yet, but instead of relying on that, tighten ship ahead of the coming concurrency storm. Fix by having a predefined set of regions, of which one will be enabled at any time. Signed-off-by: Avi Kivity --- hw/piix_pci.c | 69 ++++++++++++++++++++++++++++++----------------------------- 1 file changed, 35 insertions(+), 34 deletions(-) diff --git a/hw/piix_pci.c b/hw/piix_pci.c index 537fc19..5bca41d 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -69,8 +69,8 @@ } PIIX3State; typedef struct PAMMemoryRegion { - MemoryRegion mem; - bool initialized; + MemoryRegion alias[4]; /* index = PAM value */ + unsigned current; } PAMMemoryRegion; struct PCII440FXState { @@ -105,37 +105,35 @@ static int pci_slot_get_pirq(PCIDevice *pci_dev, int pci_intx) return (pci_intx + slot_addend) & 3; } -static void update_pam(PCII440FXState *d, uint32_t start, uint32_t end, int r, - PAMMemoryRegion *mem) +static void init_pam(PCII440FXState *d, PAMMemoryRegion *mem, + uint32_t start, uint32_t size) { - if (mem->initialized) { - memory_region_del_subregion(d->system_memory, &mem->mem); - memory_region_destroy(&mem->mem); - } + int i; - // printf("ISA mapping %08x-0x%08x: %d\n", start, end, r); - switch(r) { - case 3: - /* RAM */ - memory_region_init_alias(&mem->mem, "pam-ram", d->ram_memory, - start, end - start); - break; - case 1: - /* ROM (XXX: not quite correct) */ - memory_region_init_alias(&mem->mem, "pam-rom", d->ram_memory, - start, end - start); - memory_region_set_readonly(&mem->mem, true); - break; - case 2: - case 0: - /* XXX: should distinguish read/write cases */ - memory_region_init_alias(&mem->mem, "pam-pci", d->pci_address_space, - start, end - start); - break; + /* RAM */ + memory_region_init_alias(&mem->alias[3], "pam-ram", d->ram_memory, start, size); + /* ROM (XXX: not quite correct) */ + memory_region_init_alias(&mem->alias[1], "pam-rom", d->ram_memory, start, size); + memory_region_set_readonly(&mem->alias[1], true); + + /* XXX: should distinguish read/write cases */ + memory_region_init_alias(&mem->alias[0], "pam-pci", d->pci_address_space, + start, size); + memory_region_init_alias(&mem->alias[2], "pam-pci", d->pci_address_space, + start, size); + + for (i = 0; i < 4; ++i) { + memory_region_set_enabled(&mem->alias[i], false); + memory_region_add_subregion_overlap(d->system_memory, start, &mem->alias[i], 1); } - memory_region_add_subregion_overlap(d->system_memory, - start, &mem->mem, 1); - mem->initialized = true; + mem->current = 0; +} + +static void update_pam(PAMMemoryRegion *pam, unsigned r) +{ + memory_region_set_enabled(&pam->alias[pam->current], false); + pam->current = r; + memory_region_set_enabled(&pam->alias[pam->current], true); } static void i440fx_update_memory_mappings(PCII440FXState *d) @@ -145,12 +143,10 @@ static void i440fx_update_memory_mappings(PCII440FXState *d) bool smram_enabled; memory_region_transaction_begin(); - update_pam(d, 0xf0000, 0x100000, (d->dev.config[I440FX_PAM] >> 4) & 3, - &d->pam_regions[0]); + update_pam(&d->pam_regions[0], (d->dev.config[I440FX_PAM] >> 4) & 3); for(i = 0; i < 12; i++) { r = (d->dev.config[(i >> 1) + (I440FX_PAM + 1)] >> ((i & 1) * 4)) & 3; - update_pam(d, 0xc0000 + 0x4000 * i, 0xc0000 + 0x4000 * (i + 1), r, - &d->pam_regions[i+1]); + update_pam(&d->pam_regions[i+1], r); } smram = d->dev.config[I440FX_SMRAM]; smram_enabled = (d->smm_enabled && (smram & 0x08)) || (smram & 0x40); @@ -272,6 +268,7 @@ static PCIBus *i440fx_common_init(const char *device_name, PCIHostState *s; PIIX3State *piix3; PCII440FXState *f; + unsigned i; dev = qdev_create(NULL, "i440FX-pcihost"); s = PCI_HOST_BRIDGE(dev); @@ -303,6 +300,10 @@ static PCIBus *i440fx_common_init(const char *device_name, memory_region_add_subregion_overlap(f->system_memory, 0xa0000, &f->smram_region, 1); memory_region_set_enabled(&f->smram_region, false); + init_pam(f, &f->pam_regions[0], 0xf0000, 0x10000); + for (i = 0; i < 12; ++i) { + init_pam(f, &f->pam_regions[i+1], 0xc0000 + i * 0x4000, 0x4000); + } /* Xen supports additional interrupt routes from the PCI devices to * the IOAPIC: the four pins of each PCI device on the bus are also