From 28f362be6e7f45ea9b7a57a08555c4c784f36198 Mon Sep 17 00:00:00 2001 From: Alexander Graf Date: Mon, 15 Oct 2012 20:30:28 +0200 Subject: [PATCH 1/3] memory: Make eventfd adhere to device endianness Our memory API MMIO regions know the concept of device endianness. This is used to automatically swap endianness between devices and host CPU, depending on whether buses in between would swizzle the bits. The ioeventfd value comparison does not adhere to that semantic though. Probably because nobody has been running ioeventfd on a BE platform and the only device implementing ioeventfd right now is LE (PCI) based. So add swizzling to ioeventfd registration / deletion to make the rest of the code as consistent as possible. Thanks a lot to Michael Tsirkin to point me towards the right direction. Signed-off-by: Alexander Graf Signed-off-by: Avi Kivity --- memory.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/memory.c b/memory.c index 4f3ade06dd..d2f2fd66fb 100644 --- a/memory.c +++ b/memory.c @@ -1217,6 +1217,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, }; unsigned i; + adjust_endianness(mr, &mrfd.data, size); memory_region_transaction_begin(); for (i = 0; i < mr->ioeventfd_nb; ++i) { if (memory_region_ioeventfd_before(mrfd, mr->ioeventfds[i])) { @@ -1248,6 +1249,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, }; unsigned i; + adjust_endianness(mr, &mrfd.data, size); memory_region_transaction_begin(); for (i = 0; i < mr->ioeventfd_nb; ++i) { if (memory_region_ioeventfd_equal(mrfd, mr->ioeventfds[i])) { From 2725aec70114cf1bee00443aeb47a305f9b0c665 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 17 Oct 2012 17:10:04 +0200 Subject: [PATCH 2/3] i440fx: avoid destroying memory regions within a transaction 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 537fc1973c..5bca41d67b 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -69,8 +69,8 @@ typedef struct PIIX3State { } 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 From 2be0e25f4b6a4f91e39388cc365bbe53b56ab62a Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 17 Oct 2012 17:14:41 +0200 Subject: [PATCH 3/3] memory: abort if a memory region is destroyed during a transaction Destroying a memory region is illegal within a transaction, as until the transaction is committed, the memory core may hold references to the region. Add an assert to check for violations of this rule. Signed-off-by: Avi Kivity --- memory.c | 1 + 1 file changed, 1 insertion(+) diff --git a/memory.c b/memory.c index d2f2fd66fb..94049a797d 100644 --- a/memory.c +++ b/memory.c @@ -1022,6 +1022,7 @@ void memory_region_init_reservation(MemoryRegion *mr, void memory_region_destroy(MemoryRegion *mr) { assert(QTAILQ_EMPTY(&mr->subregions)); + assert(memory_region_transaction_depth == 0); mr->destructor(mr); memory_region_clear_coalescing(mr); g_free((char *)mr->name);