[Qemu-devel] [PULL v3 0/7] Memory API mutators

[Qemu-devel] [PULL v3 0/7] Memory API mutators

[Avi Kivity]

[repost w/ qemu-devel copied this time]

This patchset introduces memory_region_set_enabled() and
memory_region_set_address() to avoid the requirement on memory
routers to track the internal state of the memory API (so they know
whether they need to add or remove a region).  Instead, they can
simply copy the state of the region from the guest-exposed register
to the memory core, via the new mutator functions.

Please pull from

  git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git memory/mutators

v3:
   - fix confusion in patch 3 wrt function arguments and doc comments
   - add migration documentation

v2:
   - fix minor bug in set_address()
   - add set_alias_offset()
   - two example users

Avi Kivity (7):
  memory: introduce memory_region_set_enabled()
  memory: introduce memory_region_set_address()
  memory: introduce memory_region_set_alias_offset()
  memory: optimize empty transactions due to mutators
  cirrus_vga: adapt to memory mutators API
  piix_pci: adapt smram mapping to use memory mutators
  docs: document memory API interaction with migration

 docs/migration.txt |   12 ++++++++
 hw/cirrus_vga.c    |   50 +++++++++++---------------------
 hw/piix_pci.c      |   20 ++++---------
 memory.c           |   81 ++++++++++++++++++++++++++++++++++++++++++++-------
 memory.h           |   40 +++++++++++++++++++++++++
 5 files changed, 145 insertions(+), 58 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH 1/7] memory: introduce memory_region_set_enabled()

[Avi Kivity]

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 <avi@redhat.com>
---
 memory.c |   40 +++++++++++++++++++++++++++++-----------
 memory.h |   17 +++++++++++++++++
 2 files changed, 46 insertions(+), 11 deletions(-)

diff --git a/memory.c b/memory.c
index adfdf14..d0f90ca 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 53bf261..c6997c4 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.
  */
-- 
1.7.7.1

[Qemu-devel] [PATCH 2/7] memory: introduce memory_region_set_address()

[Avi Kivity]

Allow changing the address of a memory region while it is
in the memory hierarchy.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c |   21 +++++++++++++++++++++
 memory.h |   11 +++++++++++
 2 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index d0f90ca..a080d21 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 c6997c4..db53422 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.
  */
-- 
1.7.7.1

[Qemu-devel] [PATCH 3/7] memory: introduce memory_region_set_alias_offset()

[Avi Kivity]

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 <avi@redhat.com>
---
 memory.c |   14 ++++++++++++++
 memory.h |   12 ++++++++++++
 2 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index a080d21..7e842b3 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 db53422..c07bcca 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.
  */
-- 
1.7.7.1

[Qemu-devel] [PATCH 4/7] memory: optimize empty transactions due to mutators

[Avi Kivity]

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 <avi@redhat.com>
---
 memory.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/memory.c b/memory.c
index 7e842b3..87639ab 100644
--- a/memory.c
+++ b/memory.c
@@ -19,6 +19,7 @@
 #include <assert.h>
 
 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)
-- 
1.7.7.1

[Qemu-devel] [PATCH 5/7] cirrus_vga: adapt to memory mutators API

[Avi Kivity]

Simplify the code by avoiding dynamic creation and destruction of
memory regions.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/cirrus_vga.c |   50 +++++++++++++++++---------------------------------
 1 files changed, 17 insertions(+), 33 deletions(-)

diff --git a/hw/cirrus_vga.c b/hw/cirrus_vga.c
index c7e365b..9f7fea1 100644
--- a/hw/cirrus_vga.c
+++ b/hw/cirrus_vga.c
@@ -205,7 +205,7 @@ typedef void (*cirrus_fill_t)(struct CirrusVGAState *s,
     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 void cirrus_linear_bitblt_write(void *opaque,
     },
 };
 
-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)) {
-
-        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);
-    }
+        && !(s->vga.gr[0x0B] & 0x02);
+
+    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,
-- 
1.7.7.1

[Qemu-devel] [PATCH 6/7] piix_pci: adapt smram mapping to use memory mutators

[Avi Kivity]

Eliminates fake state ->smram_enabled.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 hw/piix_pci.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/hw/piix_pci.c b/hw/piix_pci.c
index d183443..ac3d898 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 int i440fx_initfn(PCIDevice *dev)
     }
     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
-- 
1.7.7.1

[Qemu-devel] [PATCH 7/7] docs: document memory API interaction with migration

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 docs/migration.txt |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/docs/migration.txt b/docs/migration.txt
index 4848c1e..f3ddd2f 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
-- 
1.7.7.1

Re: [Qemu-devel] [PULL v3 0/7] Memory API mutators

[Anthony Liguori]

On 12/15/2011 09:18 AM, Avi Kivity wrote:
> [repost w/ qemu-devel copied this time]
>
> This patchset introduces memory_region_set_enabled() and
> memory_region_set_address() to avoid the requirement on memory
> routers to track the internal state of the memory API (so they know
> whether they need to add or remove a region).  Instead, they can
> simply copy the state of the region from the guest-exposed register
> to the memory core, via the new mutator functions.

Pulled.  Thanks.

Regards,

Anthony Liguori

>
> Please pull from
>
>    git://git.kernel.org/pub/scm/virt/kvm/qemu-kvm.git memory/mutators
>
> v3:
>     - fix confusion in patch 3 wrt function arguments and doc comments
>     - add migration documentation
>
> v2:
>     - fix minor bug in set_address()
>     - add set_alias_offset()
>     - two example users
>
> Avi Kivity (7):
>    memory: introduce memory_region_set_enabled()
>    memory: introduce memory_region_set_address()
>    memory: introduce memory_region_set_alias_offset()
>    memory: optimize empty transactions due to mutators
>    cirrus_vga: adapt to memory mutators API
>    piix_pci: adapt smram mapping to use memory mutators
>    docs: document memory API interaction with migration
>
>   docs/migration.txt |   12 ++++++++
>   hw/cirrus_vga.c    |   50 +++++++++++---------------------
>   hw/piix_pci.c      |   20 ++++---------
>   memory.c           |   81 ++++++++++++++++++++++++++++++++++++++++++++-------
>   memory.h           |   40 +++++++++++++++++++++++++
>   5 files changed, 145 insertions(+), 58 deletions(-)
>