From 6bba19ba4e2b8c89496569439ca38a328cf29a77 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 14 Sep 2011 11:54:58 +0300 Subject: [PATCH 1/7] memory: introduce memory_region_set_enabled() This allows users to disable a memory region without removing it from the hierarchy, simplifying the implementation of memory routers. Signed-off-by: Avi Kivity --- memory.c | 40 +++++++++++++++++++++++++++++----------- memory.h | 17 +++++++++++++++++ 2 files changed, 46 insertions(+), 11 deletions(-) diff --git a/memory.c b/memory.c index adfdf1470c..d0f90ca350 100644 --- a/memory.c +++ b/memory.c @@ -528,6 +528,10 @@ static void render_memory_region(FlatView *view, FlatRange fr; AddrRange tmp; + if (!mr->enabled) { + return; + } + int128_addto(&base, int128_make64(mr->addr)); readonly |= mr->readonly; @@ -750,12 +754,16 @@ static void address_space_update_topology(AddressSpace *as) address_space_update_ioeventfds(as); } -static void memory_region_update_topology(void) +static void memory_region_update_topology(MemoryRegion *mr) { if (memory_region_transaction_depth) { return; } + if (mr && !mr->enabled) { + return; + } + if (address_space_memory.root) { address_space_update_topology(&address_space_memory); } @@ -773,7 +781,7 @@ void memory_region_transaction_commit(void) { assert(memory_region_transaction_depth); --memory_region_transaction_depth; - memory_region_update_topology(); + memory_region_update_topology(NULL); } static void memory_region_destructor_none(MemoryRegion *mr) @@ -813,6 +821,7 @@ void memory_region_init(MemoryRegion *mr, } mr->addr = 0; mr->offset = 0; + mr->enabled = true; mr->terminates = false; mr->readable = true; mr->readonly = false; @@ -1058,7 +1067,7 @@ void memory_region_set_log(MemoryRegion *mr, bool log, unsigned client) uint8_t mask = 1 << client; mr->dirty_log_mask = (mr->dirty_log_mask & ~mask) | (log * mask); - memory_region_update_topology(); + memory_region_update_topology(mr); } bool memory_region_get_dirty(MemoryRegion *mr, target_phys_addr_t addr, @@ -1090,7 +1099,7 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly) { if (mr->readonly != readonly) { mr->readonly = readonly; - memory_region_update_topology(); + memory_region_update_topology(mr); } } @@ -1098,7 +1107,7 @@ void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable) { if (mr->readable != readable) { mr->readable = readable; - memory_region_update_topology(); + memory_region_update_topology(mr); } } @@ -1203,7 +1212,7 @@ void memory_region_add_eventfd(MemoryRegion *mr, memmove(&mr->ioeventfds[i+1], &mr->ioeventfds[i], sizeof(*mr->ioeventfds) * (mr->ioeventfd_nb-1 - i)); mr->ioeventfds[i] = mrfd; - memory_region_update_topology(); + memory_region_update_topology(mr); } void memory_region_del_eventfd(MemoryRegion *mr, @@ -1233,7 +1242,7 @@ void memory_region_del_eventfd(MemoryRegion *mr, --mr->ioeventfd_nb; mr->ioeventfds = g_realloc(mr->ioeventfds, sizeof(*mr->ioeventfds)*mr->ioeventfd_nb + 1); - memory_region_update_topology(); + memory_region_update_topology(mr); } static void memory_region_add_subregion_common(MemoryRegion *mr, @@ -1274,7 +1283,7 @@ static void memory_region_add_subregion_common(MemoryRegion *mr, } QTAILQ_INSERT_TAIL(&mr->subregions, subregion, subregions_link); done: - memory_region_update_topology(); + memory_region_update_topology(mr); } @@ -1303,19 +1312,28 @@ void memory_region_del_subregion(MemoryRegion *mr, assert(subregion->parent == mr); subregion->parent = NULL; QTAILQ_REMOVE(&mr->subregions, subregion, subregions_link); - memory_region_update_topology(); + memory_region_update_topology(mr); +} + +void memory_region_set_enabled(MemoryRegion *mr, bool enabled) +{ + if (enabled == mr->enabled) { + return; + } + mr->enabled = enabled; + memory_region_update_topology(NULL); } void set_system_memory_map(MemoryRegion *mr) { address_space_memory.root = mr; - memory_region_update_topology(); + memory_region_update_topology(NULL); } void set_system_io_map(MemoryRegion *mr) { address_space_io.root = mr; - memory_region_update_topology(); + memory_region_update_topology(NULL); } typedef struct MemoryRegionList MemoryRegionList; diff --git a/memory.h b/memory.h index 53bf261792..c6997c4ee3 100644 --- a/memory.h +++ b/memory.h @@ -123,6 +123,7 @@ struct MemoryRegion { bool terminates; bool readable; bool readonly; /* For RAM regions */ + bool enabled; MemoryRegion *alias; target_phys_addr_t alias_offset; unsigned priority; @@ -501,6 +502,22 @@ void memory_region_add_subregion_overlap(MemoryRegion *mr, void memory_region_del_subregion(MemoryRegion *mr, MemoryRegion *subregion); + +/* + * memory_region_set_enabled: dynamically enable or disable a region + * + * Enables or disables a memory region. A disabled memory region + * ignores all accesses to itself and its subregions. It does not + * obscure sibling subregions with lower priority - it simply behaves as + * if it was removed from the hierarchy. + * + * Regions default to being enabled. + * + * @mr: the region to be updated + * @enabled: whether to enable or disable the region + */ +void memory_region_set_enabled(MemoryRegion *mr, bool enabled); + /* Start a transaction; changes will be accumulated and made visible only * when the transaction ends. */ From 2282e1af408db47bab0f900344c0ba0935835f48 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 14 Sep 2011 12:10:12 +0300 Subject: [PATCH 2/7] memory: introduce memory_region_set_address() Allow changing the address of a memory region while it is in the memory hierarchy. Signed-off-by: Avi Kivity --- memory.c | 21 +++++++++++++++++++++ memory.h | 11 +++++++++++ 2 files changed, 32 insertions(+) diff --git a/memory.c b/memory.c index d0f90ca350..a080d210c1 100644 --- a/memory.c +++ b/memory.c @@ -1324,6 +1324,27 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled) memory_region_update_topology(NULL); } +void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr) +{ + MemoryRegion *parent = mr->parent; + unsigned priority = mr->priority; + bool may_overlap = mr->may_overlap; + + if (addr == mr->addr || !parent) { + mr->addr = addr; + return; + } + + memory_region_transaction_begin(); + memory_region_del_subregion(parent, mr); + if (may_overlap) { + memory_region_add_subregion_overlap(parent, addr, mr, priority); + } else { + memory_region_add_subregion(parent, addr, mr); + } + memory_region_transaction_commit(); +} + void set_system_memory_map(MemoryRegion *mr) { address_space_memory.root = mr; diff --git a/memory.h b/memory.h index c6997c4ee3..db53422b2b 100644 --- a/memory.h +++ b/memory.h @@ -518,6 +518,17 @@ void memory_region_del_subregion(MemoryRegion *mr, */ void memory_region_set_enabled(MemoryRegion *mr, bool enabled); +/* + * memory_region_set_address: dynamically update the address of a region + * + * Dynamically updates the address of a region, relative to its parent. + * May be used on regions are currently part of a memory hierarchy. + * + * @mr: the region to be updated + * @addr: new address, relative to parent region + */ +void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr); + /* Start a transaction; changes will be accumulated and made visible only * when the transaction ends. */ From 4703359e0eeb27c382fdf2951b842cf8bde26672 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 4 Dec 2011 19:16:50 +0200 Subject: [PATCH 3/7] memory: introduce memory_region_set_alias_offset() Add an API to update an alias offset of an active alias. This can be used to simplify implementation of dynamic memory banks. Signed-off-by: Avi Kivity --- memory.c | 14 ++++++++++++++ memory.h | 12 ++++++++++++ 2 files changed, 26 insertions(+) diff --git a/memory.c b/memory.c index a080d210c1..7e842b3ad5 100644 --- a/memory.c +++ b/memory.c @@ -1345,6 +1345,20 @@ void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr) memory_region_transaction_commit(); } +void memory_region_set_alias_offset(MemoryRegion *mr, target_phys_addr_t offset) +{ + target_phys_addr_t old_offset = mr->alias_offset; + + assert(mr->alias); + mr->alias_offset = offset; + + if (offset == old_offset || !mr->parent) { + return; + } + + memory_region_update_topology(mr); +} + void set_system_memory_map(MemoryRegion *mr) { address_space_memory.root = mr; diff --git a/memory.h b/memory.h index db53422b2b..c07bcca306 100644 --- a/memory.h +++ b/memory.h @@ -529,6 +529,18 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled); */ void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr); +/* + * memory_region_set_alias_offset: dynamically update a memory alias's offset + * + * Dynamically updates the offset into the target region that an alias points + * to, as if the fourth argument to memory_region_init_alias() has changed. + * + * @mr: the #MemoryRegion to be updated; should be an alias. + * @offset: the new offset into the target memory region + */ +void memory_region_set_alias_offset(MemoryRegion *mr, + target_phys_addr_t offset); + /* Start a transaction; changes will be accumulated and made visible only * when the transaction ends. */ From e87c099f1c9e461dcaa093b1b40b14e7e899e70f Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 14 Sep 2011 12:16:20 +0300 Subject: [PATCH 4/7] memory: optimize empty transactions due to mutators The mutating memory APIs can easily cause empty transactions, where the mutators don't actually change anything, or perhaps only modify disabled regions. Detect these conditions and avoid regenerating the memory topology. Signed-off-by: Avi Kivity --- memory.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/memory.c b/memory.c index 7e842b3ad5..87639ab6ea 100644 --- a/memory.c +++ b/memory.c @@ -19,6 +19,7 @@ #include unsigned memory_region_transaction_depth = 0; +static bool memory_region_update_pending = false; typedef struct AddrRange AddrRange; @@ -757,6 +758,7 @@ static void address_space_update_topology(AddressSpace *as) static void memory_region_update_topology(MemoryRegion *mr) { if (memory_region_transaction_depth) { + memory_region_update_pending |= !mr || mr->enabled; return; } @@ -770,6 +772,8 @@ static void memory_region_update_topology(MemoryRegion *mr) if (address_space_io.root) { address_space_update_topology(&address_space_io); } + + memory_region_update_pending = false; } void memory_region_transaction_begin(void) @@ -781,7 +785,9 @@ void memory_region_transaction_commit(void) { assert(memory_region_transaction_depth); --memory_region_transaction_depth; - memory_region_update_topology(NULL); + if (!memory_region_transaction_depth && memory_region_update_pending) { + memory_region_update_topology(NULL); + } } static void memory_region_destructor_none(MemoryRegion *mr) From 7969d9ed5c9eca24e63f2f033b29131981c64a09 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 4 Dec 2011 19:49:22 +0200 Subject: [PATCH 5/7] cirrus_vga: adapt to memory mutators API Simplify the code by avoiding dynamic creation and destruction of memory regions. Signed-off-by: Avi Kivity --- hw/cirrus_vga.c | 48 ++++++++++++++++-------------------------------- 1 file changed, 16 insertions(+), 32 deletions(-) diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c index c7e365b2a6..9f7fea19e1 100644 --- a/hw/cirrus_vga.c +++ b/hw/cirrus_vga.c @@ -205,7 +205,7 @@ typedef struct CirrusVGAState { bool linear_vram; /* vga.vram mapped over cirrus_linear_io */ MemoryRegion low_mem_container; /* container for 0xa0000-0xc0000 */ MemoryRegion low_mem; /* always mapped, overridden by: */ - MemoryRegion *cirrus_bank[2]; /* aliases at 0xa0000-0xb0000 */ + MemoryRegion cirrus_bank[2]; /* aliases at 0xa0000-0xb0000 */ uint32_t cirrus_addr_mask; uint32_t linear_mmio_mask; uint8_t cirrus_shadow_gr0; @@ -2363,40 +2363,16 @@ static const MemoryRegionOps cirrus_linear_bitblt_io_ops = { }, }; -static void unmap_bank(CirrusVGAState *s, unsigned bank) -{ - if (s->cirrus_bank[bank]) { - memory_region_del_subregion(&s->low_mem_container, - s->cirrus_bank[bank]); - memory_region_destroy(s->cirrus_bank[bank]); - g_free(s->cirrus_bank[bank]); - s->cirrus_bank[bank] = NULL; - } -} - static void map_linear_vram_bank(CirrusVGAState *s, unsigned bank) { - MemoryRegion *mr; - static const char *names[] = { "vga.bank0", "vga.bank1" }; - - if (!(s->cirrus_srcptr != s->cirrus_srcptr_end) + MemoryRegion *mr = &s->cirrus_bank[bank]; + bool enabled = !(s->cirrus_srcptr != s->cirrus_srcptr_end) && !((s->vga.sr[0x07] & 0x01) == 0) && !((s->vga.gr[0x0B] & 0x14) == 0x14) - && !(s->vga.gr[0x0B] & 0x02)) { + && !(s->vga.gr[0x0B] & 0x02); - mr = g_malloc(sizeof(*mr)); - memory_region_init_alias(mr, names[bank], &s->vga.vram, - s->cirrus_bank_base[bank], 0x8000); - memory_region_add_subregion_overlap( - &s->low_mem_container, - 0x8000 * bank, - mr, - 1); - unmap_bank(s, bank); - s->cirrus_bank[bank] = mr; - } else { - unmap_bank(s, bank); - } + memory_region_set_enabled(mr, enabled); + memory_region_set_alias_offset(mr, s->cirrus_bank_base[bank]); } static void map_linear_vram(CirrusVGAState *s) @@ -2415,8 +2391,8 @@ static void unmap_linear_vram(CirrusVGAState *s) s->linear_vram = false; memory_region_del_subregion(&s->pci_bar, &s->vga.vram); } - unmap_bank(s, 0); - unmap_bank(s, 1); + memory_region_set_enabled(&s->cirrus_bank[0], false); + memory_region_set_enabled(&s->cirrus_bank[1], false); } /* Compute the memory access functions */ @@ -2856,6 +2832,14 @@ static void cirrus_init_common(CirrusVGAState * s, int device_id, int is_pci, memory_region_init_io(&s->low_mem, &cirrus_vga_mem_ops, s, "cirrus-low-memory", 0x20000); memory_region_add_subregion(&s->low_mem_container, 0, &s->low_mem); + for (i = 0; i < 2; ++i) { + static const char *names[] = { "vga.bank0", "vga.bank1" }; + MemoryRegion *bank = &s->cirrus_bank[i]; + memory_region_init_alias(bank, names[i], &s->vga.vram, 0, 0x8000); + memory_region_set_enabled(bank, false); + memory_region_add_subregion_overlap(&s->low_mem_container, i * 0x8000, + bank, 1); + } memory_region_add_subregion_overlap(system_memory, isa_mem_base + 0x000a0000, &s->low_mem_container, From b41e1ed4b35b4aa35018a348d80c46e40e6f4163 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Sun, 4 Dec 2011 20:06:16 +0200 Subject: [PATCH 6/7] piix_pci: adapt smram mapping to use memory mutators Eliminates fake state ->smram_enabled. Signed-off-by: Avi Kivity --- hw/piix_pci.c | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/hw/piix_pci.c b/hw/piix_pci.c index d183443b2f..ac3d8980e8 100644 --- a/hw/piix_pci.c +++ b/hw/piix_pci.c @@ -81,7 +81,6 @@ struct PCII440FXState { PAMMemoryRegion pam_regions[13]; MemoryRegion smram_region; uint8_t smm_enabled; - bool smram_enabled; PIIX3State *piix3; }; @@ -141,6 +140,7 @@ static void i440fx_update_memory_mappings(PCII440FXState *d) { int i, r; uint32_t smram; + bool smram_enabled; memory_region_transaction_begin(); update_pam(d, 0xf0000, 0x100000, (d->dev.config[I440FX_PAM] >> 4) & 3, @@ -151,18 +151,8 @@ static void i440fx_update_memory_mappings(PCII440FXState *d) &d->pam_regions[i+1]); } smram = d->dev.config[I440FX_SMRAM]; - if ((d->smm_enabled && (smram & 0x08)) || (smram & 0x40)) { - if (!d->smram_enabled) { - memory_region_del_subregion(d->system_memory, &d->smram_region); - d->smram_enabled = true; - } - } else { - if (d->smram_enabled) { - memory_region_add_subregion_overlap(d->system_memory, 0xa0000, - &d->smram_region, 1); - d->smram_enabled = false; - } - } + smram_enabled = (d->smm_enabled && (smram & 0x08)) || (smram & 0x40); + memory_region_set_enabled(&d->smram_region, !smram_enabled); memory_region_transaction_commit(); } @@ -307,7 +297,9 @@ static PCIBus *i440fx_common_init(const char *device_name, } memory_region_init_alias(&f->smram_region, "smram-region", f->pci_address_space, 0xa0000, 0x20000); - f->smram_enabled = true; + memory_region_add_subregion_overlap(f->system_memory, 0xa0000, + &f->smram_region, 1); + memory_region_set_enabled(&f->smram_region, false); /* 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 a6c5c07990059c94bf50b0422e953af1368353c0 Mon Sep 17 00:00:00 2001 From: Avi Kivity Date: Wed, 7 Dec 2011 18:57:41 +0200 Subject: [PATCH 7/7] docs: document memory API interaction with migration Signed-off-by: Avi Kivity --- docs/migration.txt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/docs/migration.txt b/docs/migration.txt index 4848c1e52d..f3ddd2f1a8 100644 --- a/docs/migration.txt +++ b/docs/migration.txt @@ -219,6 +219,18 @@ The functions to do that are inside a vmstate definition, and are called: Example: You can look at hpet.c, that uses the three function to massage the state that is transferred. +If you use memory API functions that update memory layout outside +initialization (i.e., in response to a guest action), this is a strong +indication that you need to call these functions in a post_load callback. +Examples of such memory API functions are: + + - memory_region_add_subregion() + - memory_region_del_subregion() + - memory_region_set_readonly() + - memory_region_set_enabled() + - memory_region_set_address() + - memory_region_set_alias_offset() + === Subsections === The use of version_id allows to be able to migrate from older versions