[Qemu-devel] [PATCH 0/3] Memory API mutators

[Qemu-devel] [PATCH 0/3] 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.

Please review.  Do we need a memory_region_set_size() as well?  Do we want

  memory_region_set_attributes(mr,
                               MR_ATTR_ENABLED | MR_ATTR_SIZE,
                               (MemoryRegionAttributes) {
                                   .enabled = s->enabled,
                                   .address = s->addr,
                               });

?

Avi Kivity (3):
  memory: introduce memory_region_set_enabled()
  memory: introduce memory_region_set_address()
  memory: optimize empty transactions due to mutators

 memory.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
 memory.h |   28 +++++++++++++++++++++++++++
 2 files changed, 82 insertions(+), 10 deletions(-)

-- 
1.7.6.3

[Qemu-devel] [PATCH 1/3] 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 |   38 ++++++++++++++++++++++++++++----------
 memory.h |   17 +++++++++++++++++
 2 files changed, 45 insertions(+), 10 deletions(-)

diff --git a/memory.c b/memory.c
index 101b67c..ce0f3fd 100644
--- a/memory.c
+++ b/memory.c
@@ -494,6 +494,10 @@ static void render_memory_region(FlatView *view,
     FlatRange fr;
     AddrRange tmp;
 
+    if (!mr->enabled) {
+        return;
+    }
+
     base += mr->addr;
 
     tmp = addrrange_make(base, mr->size);
@@ -710,12 +714,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);
     }
@@ -733,7 +741,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)
@@ -770,6 +778,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->size = size;
     mr->addr = 0;
     mr->offset = 0;
+    mr->enabled = true;
     mr->terminates = false;
     mr->readable = true;
     mr->destructor = memory_region_destructor_none;
@@ -1005,7 +1014,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,
@@ -1042,7 +1051,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);
     }
 }
 
@@ -1144,7 +1153,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,
@@ -1174,7 +1183,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,
@@ -1210,7 +1219,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);
 }
 
 
@@ -1239,17 +1248,26 @@ 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);
 }
diff --git a/memory.h b/memory.h
index 06b83ae..60b1449 100644
--- a/memory.h
+++ b/memory.h
@@ -114,6 +114,7 @@ struct MemoryRegion {
     IORange iorange;
     bool terminates;
     bool readable;
+    bool enabled;
     MemoryRegion *alias;
     target_phys_addr_t alias_offset;
     unsigned priority;
@@ -492,6 +493,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.6.3

[Qemu-devel] [PATCH 2/3] 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 |   20 ++++++++++++++++++++
 memory.h |   11 +++++++++++
 2 files changed, 31 insertions(+), 0 deletions(-)

diff --git a/memory.c b/memory.c
index ce0f3fd..3b0cc25 100644
--- a/memory.c
+++ b/memory.c
@@ -1260,6 +1260,26 @@ 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) {
+        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 60b1449..468970b 100644
--- a/memory.h
+++ b/memory.h
@@ -509,6 +509,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.6.3

[Qemu-devel] [PATCH 3/3] 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 3b0cc25..1370fac 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;
 
@@ -717,6 +718,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;
     }
 
@@ -730,6 +732,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)
@@ -741,7 +745,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.6.3

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

[Avi Kivity]

Jan, too, was interested in this.

On 09/14/2011 12:23 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.
>
> Please review.  Do we need a memory_region_set_size() as well?  Do we want
>
>    memory_region_set_attributes(mr,
>                                 MR_ATTR_ENABLED | MR_ATTR_SIZE,
>                                 (MemoryRegionAttributes) {
>                                     .enabled = s->enabled,
>                                     .address = s->addr,
>                                 });
>
> ?
>
> Avi Kivity (3):
>    memory: introduce memory_region_set_enabled()
>    memory: introduce memory_region_set_address()
>    memory: optimize empty transactions due to mutators
>
>   memory.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>   memory.h |   28 +++++++++++++++++++++++++++
>   2 files changed, 82 insertions(+), 10 deletions(-)
>


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

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

[Jan Kiszka]

On 2011-09-14 11:49, Avi Kivity wrote:
> Jan, too, was interested in this.
> 
> On 09/14/2011 12:23 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.
>>
>> Please review.  Do we need a memory_region_set_size() as well?  Do we want
>>
>>    memory_region_set_attributes(mr,
>>                                 MR_ATTR_ENABLED | MR_ATTR_SIZE,
>>                                 (MemoryRegionAttributes) {
>>                                     .enabled = s->enabled,
>>                                     .address = s->addr,
>>                                 });
>>
>> ?
>>
>> Avi Kivity (3):
>>    memory: introduce memory_region_set_enabled()
>>    memory: introduce memory_region_set_address()
>>    memory: optimize empty transactions due to mutators
>>
>>   memory.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
>>   memory.h |   28 +++++++++++++++++++++++++++
>>   2 files changed, 82 insertions(+), 10 deletions(-)
>>

Whatever the outcome is (tons of memory_region_set/get_X functions or
huge attribute structures + set/get_attributes), it should be consistent
for all attributes of a memory region. And there should be only one way
of doing this.

I think the decision multiple set/get vs. attribute struct depends on
some (estimated) usage stats: How many call sites will access multiple
attributes in one run and how may will only manipulate a single?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

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

[Avi Kivity]

On 09/14/2011 01:27 PM, Jan Kiszka wrote:
> >>
> >>  Avi Kivity (3):
> >>     memory: introduce memory_region_set_enabled()
> >>     memory: introduce memory_region_set_address()
> >>     memory: optimize empty transactions due to mutators
> >>
> >>    memory.c |   64 ++++++++++++++++++++++++++++++++++++++++++++++++++++---------
> >>    memory.h |   28 +++++++++++++++++++++++++++
> >>    2 files changed, 82 insertions(+), 10 deletions(-)
> >>
>
> Whatever the outcome is (tons of memory_region_set/get_X functions or
> huge attribute structures + set/get_attributes), it should be consistent
> for all attributes of a memory region. And there should be only one way
> of doing this.

Why just one way?  Different users may have different patterns.  Of 
course internally one will be implemented on top of the other.

> I think the decision multiple set/get vs. attribute struct depends on
> some (estimated) usage stats: How many call sites will access multiple
> attributes in one run and how may will only manipulate a single?
>

There won't be that many call sites to have real statistics.

Let's just go with the individual accessors and see.  When the entire 
tree is converted (including the already-converted sites that need 
revisiting to use this) we can see how it looks and update the API.

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

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

[Peter Maydell]

On 14 September 2011 10:23, Avi Kivity <avi@redhat.com> 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.
>
> Please review.  Do we need a memory_region_set_size() as well?

Would set_size() allow things like omap_gpmc() to avoid the need
to create an intermediate container subregion to enforce size
clipping on the child region it's trying to map?

(Strictly speaking what omap_gpmc() wants is not merely clipping
to a guest-specified size but also wrapping, so you can take a
16MB child region and map the bottom 4MB of it repeating into
a 32MB chunk of address space, say. But that would require a lot
of playing games with aliases to implement a bizarre corner
case that nobody uses in practice.)

-- PMM

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

[Avi Kivity]

On 09/14/2011 12:56 PM, Peter Maydell wrote:
> On 14 September 2011 10:23, Avi Kivity<avi@redhat.com>  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.
> >
> >  Please review.  Do we need a memory_region_set_size() as well?
>
> Would set_size() allow things like omap_gpmc() to avoid the need
> to create an intermediate container subregion to enforce size
> clipping on the child region it's trying to map?

I'd recommend not calling _set_size() on somebody else's region - this 
quickly leads to confusion.  Only call set_size() if you also called 
_init() and will call _destroy().

Can you point me at the code in question?

_set_size() may be useful for dynamic bridge windows and the like.

> (Strictly speaking what omap_gpmc() wants is not merely clipping
> to a guest-specified size but also wrapping, so you can take a
> 16MB child region and map the bottom 4MB of it repeating into
> a 32MB chunk of address space, say. But that would require a lot
> of playing games with aliases to implement a bizarre corner
> case that nobody uses in practice.)
>

That's best done in the memory core, the rendering loop can be adjusted 
to do this replication.

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

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

[Peter Maydell]

On 14 September 2011 11:02, Avi Kivity <avi@redhat.com> wrote:
> On 09/14/2011 12:56 PM, Peter Maydell wrote:
>>
>> On 14 September 2011 10:23, Avi Kivity<avi@redhat.com>  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.
>> >
>> >  Please review.  Do we need a memory_region_set_size() as well?
>>
>> Would set_size() allow things like omap_gpmc() to avoid the need
>> to create an intermediate container subregion to enforce size
>> clipping on the child region it's trying to map?
>
> I'd recommend not calling _set_size() on somebody else's region - this
> quickly leads to confusion.  Only call set_size() if you also called _init()
> and will call _destroy().
>
> Can you point me at the code in question?

hw/omap_gpmc.c:omap_gpmc_cs_map(). For each of the 8 children you
can connect to it, the GPMC has a base and mask register. The
hardware logic is effectively
 if ((address & mask) == base) { send transaction to this child }

(complicated only slightly by the register for base only having
bits [29:24] with the others implied-zero, and the register for
mask only having bits [27:24].) The effect is that you can use
the mask value to set the size of the area the child is mapped in.
(Silly mask settings with "holes" are discouraged by the TRM,
and the current code doesn't handle them.)

The repeated-in-the-space effect happens if the child is smaller
than the space it's in: the child hardware just ignores the higher
bits of the address so appears multiple times.

>> (Strictly speaking what omap_gpmc() wants is not merely clipping
>> to a guest-specified size but also wrapping, so you can take a
>> 16MB child region and map the bottom 4MB of it repeating into
>> a 32MB chunk of address space, say. But that would require a lot
>> of playing games with aliases to implement a bizarre corner
>> case that nobody uses in practice.)

> That's best done in the memory core, the rendering loop can be adjusted to
> do this replication.

That would be nice, although as I say nobody is actually relying
on it so probably not worth the effort unless there's another user
for it.

-- PMM

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

[Avi Kivity]

On 09/14/2011 01:21 PM, Peter Maydell wrote:
> On 14 September 2011 11:02, Avi Kivity<avi@redhat.com>  wrote:
> >  On 09/14/2011 12:56 PM, Peter Maydell wrote:
> >>
> >>  On 14 September 2011 10:23, Avi Kivity<avi@redhat.com>    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.
> >>  >
> >>  >    Please review.  Do we need a memory_region_set_size() as well?
> >>
> >>  Would set_size() allow things like omap_gpmc() to avoid the need
> >>  to create an intermediate container subregion to enforce size
> >>  clipping on the child region it's trying to map?
> >
> >  I'd recommend not calling _set_size() on somebody else's region - this
> >  quickly leads to confusion.  Only call set_size() if you also called _init()
> >  and will call _destroy().
> >
> >  Can you point me at the code in question?
>
> hw/omap_gpmc.c:omap_gpmc_cs_map(). For each of the 8 children you
> can connect to it, the GPMC has a base and mask register. The
> hardware logic is effectively
>   if ((address&  mask) == base) { send transaction to this child }
>
> (complicated only slightly by the register for base only having
> bits [29:24] with the others implied-zero, and the register for
> mask only having bits [27:24].) The effect is that you can use
> the mask value to set the size of the area the child is mapped in.
> (Silly mask settings with "holes" are discouraged by the TRM,
> and the current code doesn't handle them.)

Thanks; and I think that in this case the omap code should avoid 
touching the child region (who knows, its owner may call 
memory_region_size() one day) and use a container (or alias - seems a 
better fit?) instead.

btw, what does a truncating _set_size() do to a RAM region?  Discard the 
excess state?  Or remember it and maintain two size, one internal and 
one for show?

> The repeated-in-the-space effect happens if the child is smaller
> than the space it's in: the child hardware just ignores the higher
> bits of the address so appears multiple times.
>
> >>  (Strictly speaking what omap_gpmc() wants is not merely clipping
> >>  to a guest-specified size but also wrapping, so you can take a
> >>  16MB child region and map the bottom 4MB of it repeating into
> >>  a 32MB chunk of address space, say. But that would require a lot
> >>  of playing games with aliases to implement a bizarre corner
> >>  case that nobody uses in practice.)
>
> >  That's best done in the memory core, the rendering loop can be adjusted to
> >  do this replication.
>
> That would be nice, although as I say nobody is actually relying
> on it so probably not worth the effort unless there's another user
> for it.

There's another user in the infamous pflash_cfi02 - see 
pflash_setup_mappings().

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