[Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Avi Kivity]

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.

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

Avi Kivity (6):
  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

 hw/cirrus_vga.c |   50 +++++++++++----------------------
 hw/piix_pci.c   |   20 ++++---------
 memory.c        |   81 +++++++++++++++++++++++++++++++++++++++++++++++-------
 memory.h        |   39 ++++++++++++++++++++++++++
 4 files changed, 132 insertions(+), 58 deletions(-)

-- 
1.7.7.1

[Qemu-devel] [PATCH v2 1/6] 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 v2 2/6] 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 v2 3/6] 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 |   13 ++++++++++++-
 2 files changed, 26 insertions(+), 1 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..2022de7 100644
--- a/memory.h
+++ b/memory.h
@@ -527,7 +527,18 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled);
  * @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);
+void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t offset);
+
+/*
+ * 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 addr);
 
 /* Start a transaction; changes will be accumulated and made visible only
  * when the transaction ends.
-- 
1.7.7.1

Re: [Qemu-devel] [PATCH v2 3/6] memory: introduce memory_region_set_alias_offset()

[Blue Swirl]

On Sun, Dec 4, 2011 at 18:09, Avi Kivity <avi@redhat.com> wrote:
> 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 |   13 ++++++++++++-
>  2 files changed, 26 insertions(+), 1 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..2022de7 100644
> --- a/memory.h
> +++ b/memory.h
> @@ -527,7 +527,18 @@ void memory_region_set_enabled(MemoryRegion *mr, bool enabled);
>  * @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);
> +void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t offset);

This isn't the function you are looking for, but still 'addr' is
changed to 'offset'.

> +
> +/*
> + * 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 addr);

Here 'addr' doesn't match the description above.

Re: [Qemu-devel] [PATCH v2 3/6] memory: introduce memory_region_set_alias_offset()

[Avi Kivity]

On 12/04/2011 11:34 PM, Blue Swirl wrote:
> On Sun, Dec 4, 2011 at 18:09, Avi Kivity <avi@redhat.com> wrote:
> > Add an API to update an alias offset of an active alias.  This can be
> > used to simplify implementation of dynamic memory banks.
> >
> >  */
> > -void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t addr);
> > +void memory_region_set_address(MemoryRegion *mr, target_phys_addr_t offset);
>
> This isn't the function you are looking for, but still 'addr' is
> changed to 'offset'.
>
> > +
> > +/*
> > + * 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 addr);
>
> Here 'addr' doesn't match the description above.

Thanks, fixed.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH v2 4/6] 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 v2 5/6] 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 v2 6/6] 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

Re: [Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Anthony Liguori]

On 12/04/2011 12:09 PM, Avi Kivity wrote:
> 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.

Based on previous discussions, any time these functions are used, the caller 
more than likely needs to call them again in a post_load hook during restore, 
correct?

It would be good to document this very clearly in the header docs for each function.

Other than Blue's comments, the rest looks good to me.

Regards,

Anthony Liguori

>
> v2:
>     - fix minor bug in set_address()
>     - add set_alias_offset()
>     - two example users
>
> Avi Kivity (6):
>    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
>
>   hw/cirrus_vga.c |   50 +++++++++++----------------------
>   hw/piix_pci.c   |   20 ++++---------
>   memory.c        |   81 +++++++++++++++++++++++++++++++++++++++++++++++-------
>   memory.h        |   39 ++++++++++++++++++++++++++
>   4 files changed, 132 insertions(+), 58 deletions(-)
>

Re: [Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Avi Kivity]

On 12/07/2011 05:52 PM, Anthony Liguori wrote:
> On 12/04/2011 12:09 PM, Avi Kivity wrote:
>> 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.
>
> Based on previous discussions, any time these functions are used, the
> caller more than likely needs to call them again in a post_load hook
> during restore, correct?
>
> It would be good to document this very clearly in the header docs for
> each function.

It's a layering violation, but I'll add something.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Anthony Liguori]

On 12/07/2011 09:54 AM, Avi Kivity wrote:
> On 12/07/2011 05:52 PM, Anthony Liguori wrote:
>> On 12/04/2011 12:09 PM, Avi Kivity wrote:
>>> 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.
>>
>> Based on previous discussions, any time these functions are used, the
>> caller more than likely needs to call them again in a post_load hook
>> during restore, correct?
>>
>> It would be good to document this very clearly in the header docs for
>> each function.
>
> It's a layering violation, but I'll add something.

Ok, let's add a docs/vmstate.txt and add a section that says "If you use the 
following functions, you probably need to call them again in post_load" and put 
these functions on the list.

Regards,

Anthony Liguori

>

Re: [Qemu-devel] [PATCH v2 0/6] Memory API mutators

[Peter Maydell]

On 7 December 2011 15:56, Anthony Liguori <anthony@codemonkey.ws> wrote:

> Ok, let's add a docs/vmstate.txt and add a section that says "If you use the
> following functions, you probably need to call them again in post_load" and
> put these functions on the list.

We've already got docs/migration.txt which talks about the postload hook.

-- PMM