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

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

[Avi Kivity]

The following patches fix a resource leak, add a ROM/device API (for flash
devices which act like memory when read, and as an mmio device when written),
and correct typos in the documentation.

Avi Kivity (3):
  memory: reclaim resources when a memory region is destroyed for good
  memory: add API for creating ROM/device regions
  memory: correct documentation typos

 docs/memory.txt |    8 +++---
 memory.c        |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 memory.h        |   35 +++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 6 deletions(-)

-- 
1.7.5.3

[Qemu-devel] [PATCH 1/3] memory: reclaim resources when a memory region is destroyed for good

[Avi Kivity]

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

diff --git a/memory.c b/memory.c
index be891c6..5e3d966 100644
--- a/memory.c
+++ b/memory.c
@@ -661,6 +661,25 @@ void memory_region_transaction_commit(void)
     memory_region_update_topology();
 }
 
+static void memory_region_destructor_none(MemoryRegion *mr)
+{
+}
+
+static void memory_region_destructor_ram(MemoryRegion *mr)
+{
+    qemu_ram_free(mr->ram_addr);
+}
+
+static void memory_region_destructor_ram_from_ptr(MemoryRegion *mr)
+{
+    qemu_ram_free_from_ptr(mr->ram_addr);
+}
+
+static void memory_region_destructor_iomem(MemoryRegion *mr)
+{
+    cpu_unregister_io_memory(mr->ram_addr);
+}
+
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
@@ -671,6 +690,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->addr = 0;
     mr->offset = 0;
     mr->terminates = false;
+    mr->destructor = memory_region_destructor_none;
     mr->priority = 0;
     mr->may_overlap = false;
     mr->alias = NULL;
@@ -833,6 +853,7 @@ static void memory_region_prepare_ram_addr(MemoryRegion *mr)
         return;
     }
 
+    mr->destructor = memory_region_destructor_iomem;
     mr->ram_addr = cpu_register_io_memory(memory_region_read_thunk,
                                           memory_region_write_thunk,
                                           mr,
@@ -860,6 +881,7 @@ void memory_region_init_ram(MemoryRegion *mr,
 {
     memory_region_init(mr, name, size);
     mr->terminates = true;
+    mr->destructor = memory_region_destructor_ram;
     mr->ram_addr = qemu_ram_alloc(dev, name, size);
     mr->backend_registered = true;
 }
@@ -872,6 +894,7 @@ void memory_region_init_ram_ptr(MemoryRegion *mr,
 {
     memory_region_init(mr, name, size);
     mr->terminates = true;
+    mr->destructor = memory_region_destructor_ram_from_ptr;
     mr->ram_addr = qemu_ram_alloc_from_ptr(dev, name, size, ptr);
     mr->backend_registered = true;
 }
@@ -890,6 +913,7 @@ void memory_region_init_alias(MemoryRegion *mr,
 void memory_region_destroy(MemoryRegion *mr)
 {
     assert(QTAILQ_EMPTY(&mr->subregions));
+    mr->destructor(mr);
     memory_region_clear_coalescing(mr);
     qemu_free((char *)mr->name);
     qemu_free(mr->ioeventfds);
diff --git a/memory.h b/memory.h
index da00a3b..c9252a2 100644
--- a/memory.h
+++ b/memory.h
@@ -109,6 +109,7 @@ struct MemoryRegion {
     target_phys_addr_t addr;
     target_phys_addr_t offset;
     bool backend_registered;
+    void (*destructor)(MemoryRegion *mr);
     ram_addr_t ram_addr;
     IORange iorange;
     bool terminates;
-- 
1.7.5.3

[Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Avi Kivity]

ROM/device regions act as mapped RAM for reads, can I/O memory for
writes.  This allow emulation of flash devices.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 memory.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
 memory.h |   34 ++++++++++++++++++++++++++++++++++
 2 files changed, 78 insertions(+), 2 deletions(-)

diff --git a/memory.c b/memory.c
index 5e3d966..beff98c 100644
--- a/memory.c
+++ b/memory.c
@@ -125,6 +125,7 @@ struct FlatRange {
     target_phys_addr_t offset_in_region;
     AddrRange addr;
     uint8_t dirty_log_mask;
+    bool readable;
 };
 
 /* Flattened global view of current active memory hierarchy.  Kept in sorted
@@ -164,7 +165,8 @@ static bool flatrange_equal(FlatRange *a, FlatRange *b)
 {
     return a->mr == b->mr
         && addrrange_equal(a->addr, b->addr)
-        && a->offset_in_region == b->offset_in_region;
+        && a->offset_in_region == b->offset_in_region
+        && a->readable == b->readable;
 }
 
 static void flatview_init(FlatView *view)
@@ -200,7 +202,8 @@ static bool can_merge(FlatRange *r1, FlatRange *r2)
     return addrrange_end(r1->addr) == r2->addr.start
         && r1->mr == r2->mr
         && r1->offset_in_region + r1->addr.size == r2->offset_in_region
-        && r1->dirty_log_mask == r2->dirty_log_mask;
+        && r1->dirty_log_mask == r2->dirty_log_mask
+        && r1->readable == r2->readable;
 }
 
 /* Attempt to simplify a view by merging ajacent ranges */
@@ -241,6 +244,10 @@ static void as_memory_range_add(AddressSpace *as, FlatRange *fr)
         region_offset = 0;
     }
 
+    if (!fr->readable) {
+        phys_offset &= TARGET_PAGE_MASK;
+    }
+
     cpu_register_physical_memory_log(fr->addr.start,
                                      fr->addr.size,
                                      phys_offset,
@@ -462,6 +469,7 @@ static void render_memory_region(FlatView *view,
             fr.offset_in_region = offset_in_region;
             fr.addr = addrrange_make(base, now);
             fr.dirty_log_mask = mr->dirty_log_mask;
+            fr.readable = mr->readable;
             flatview_insert(view, i, &fr);
             ++i;
             base += now;
@@ -480,6 +488,7 @@ static void render_memory_region(FlatView *view,
         fr.offset_in_region = offset_in_region;
         fr.addr = addrrange_make(base, remain);
         fr.dirty_log_mask = mr->dirty_log_mask;
+        fr.readable = mr->readable;
         flatview_insert(view, i, &fr);
     }
 }
@@ -680,6 +689,12 @@ static void memory_region_destructor_iomem(MemoryRegion *mr)
     cpu_unregister_io_memory(mr->ram_addr);
 }
 
+static void memory_region_destructor_rom_device(MemoryRegion *mr)
+{
+    qemu_ram_free(mr->ram_addr & TARGET_PAGE_MASK);
+    cpu_unregister_io_memory(mr->ram_addr & ~(TARGET_PAGE_MASK | IO_MEM_ROMD));
+}
+
 void memory_region_init(MemoryRegion *mr,
                         const char *name,
                         uint64_t size)
@@ -690,6 +705,7 @@ void memory_region_init(MemoryRegion *mr,
     mr->addr = 0;
     mr->offset = 0;
     mr->terminates = false;
+    mr->readable = true;
     mr->destructor = memory_region_destructor_none;
     mr->priority = 0;
     mr->may_overlap = false;
@@ -910,6 +926,24 @@ void memory_region_init_alias(MemoryRegion *mr,
     mr->alias_offset = offset;
 }
 
+void memory_region_init_rom_device(MemoryRegion *mr,
+                                   const MemoryRegionOps *ops,
+                                   DeviceState *dev,
+                                   const char *name,
+                                   uint64_t size)
+{
+    memory_region_init(mr, name, size);
+    mr->terminates = true;
+    mr->destructor = memory_region_destructor_rom_device;
+    mr->ram_addr = qemu_ram_alloc(dev, name, size);
+    mr->ram_addr |= cpu_register_io_memory(memory_region_read_thunk,
+                                           memory_region_write_thunk,
+                                           mr,
+                                           mr->ops->endianness);
+    mr->ram_addr |= IO_MEM_ROMD;
+    mr->backend_registered = true;
+}
+
 void memory_region_destroy(MemoryRegion *mr)
 {
     assert(QTAILQ_EMPTY(&mr->subregions));
@@ -967,6 +1001,14 @@ void memory_region_set_readonly(MemoryRegion *mr, bool readonly)
     /* FIXME */
 }
 
+void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable)
+{
+    if (mr->readable != readable) {
+        mr->readable = readable;
+        memory_region_update_topology();
+    }
+}
+
 void memory_region_reset_dirty(MemoryRegion *mr, target_phys_addr_t addr,
                                target_phys_addr_t size, unsigned client)
 {
diff --git a/memory.h b/memory.h
index c9252a2..0553cc7 100644
--- a/memory.h
+++ b/memory.h
@@ -113,6 +113,7 @@ struct MemoryRegion {
     ram_addr_t ram_addr;
     IORange iorange;
     bool terminates;
+    bool readable;
     MemoryRegion *alias;
     target_phys_addr_t alias_offset;
     unsigned priority;
@@ -219,6 +220,25 @@ void memory_region_init_alias(MemoryRegion *mr,
                               MemoryRegion *orig,
                               target_phys_addr_t offset,
                               uint64_t size);
+
+/**
+ * memory_region_init_rom_device:  Initialize a ROM memory region.  Writes are
+ *                                 handled via callbacks.
+ *
+ * @mr: the #MemoryRegion to be initialized.
+ * @ops: callbacks for write access handling.
+ * @dev: a device associated with the region; may be %NULL.
+ * @name: the name of the region; the pair (@dev, @name) must be globally
+ *        unique.  The name is part of the save/restore ABI and so cannot be
+ *        changed.
+ * @size: size of the region.
+ */
+void memory_region_init_rom_device(MemoryRegion *mr,
+                                   const MemoryRegionOps *ops,
+                                   DeviceState *dev, /* FIXME: layering violation */
+                                   const char *name,
+                                   uint64_t size);
+
 /**
  * memory_region_destroy: Destroy a memory region and relaim all resources.
  *
@@ -331,6 +351,20 @@ void memory_region_reset_dirty(MemoryRegion *mr, target_phys_addr_t addr,
 void memory_region_set_readonly(MemoryRegion *mr, bool readonly);
 
 /**
+ * memory_region_rom_device_set_readable: enable/disable ROM readability
+ *
+ * Allows a ROM device (initialized with memory_region_init_rom_device() to
+ * to be marked as readable (default) or not readable.  When it is readable,
+ * the device is mapped to guest memory.  When not readable, reads are
+ * forwarded to the #MemoryRegion.read function.
+ *
+ * @mr: the memory region to be updated
+ * @readable: whether reads are satisified directly (%true) or via callbacks
+ *            (%false)
+ */
+void memory_region_rom_device_set_readable(MemoryRegion *mr, bool readable);
+
+/**
  * memory_region_set_coalescing: Enable memory coalescing for the region.
  *
  * Enabled writes to a region to be queued for later processing. MMIO ->write
-- 
1.7.5.3

Re: [Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Anthony Liguori]

On 08/08/2011 11:58 AM, Avi Kivity wrote:
> ROM/device regions act as mapped RAM for reads, can I/O memory for
> writes.  This allow emulation of flash devices.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>
> ---
>   memory.c |   46 ++++++++++++++++++++++++++++++++++++++++++++--
>   memory.h |   34 ++++++++++++++++++++++++++++++++++
>   2 files changed, 78 insertions(+), 2 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 5e3d966..beff98c 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -125,6 +125,7 @@ struct FlatRange {
>       target_phys_addr_t offset_in_region;
>       AddrRange addr;
>       uint8_t dirty_log_mask;
> +    bool readable;

In a follow up, it might be good to add a comment explaining that this 
whole readable thing is not just an optimization, but a hard requirement 
for KVM in order to be able to execute code from ROM.

Regards,

Anthony Liguori

Re: [Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Avi Kivity]

On 08/12/2011 06:48 AM, Anthony Liguori wrote:
>>       target_phys_addr_t offset_in_region;
>>       AddrRange addr;
>>       uint8_t dirty_log_mask;
>> +    bool readable;
> @@ -125,6 +125,7 @@ struct FlatRange {
>
> In a follow up, it might be good to add a comment explaining that this 
> whole readable thing is not just an optimization, but a hard 
> requirement for KVM in order to be able to execute code from ROM.

This has nothing to do with kvm (in fact, I think we cannot support it 
under kvm with current interfaces).  It's there to support devices that 
sometimes act as RAM and sometimes as mmio.

We could also do it with a container and playing with subregions.  I 
tried it and it was pretty complicated.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.

Re: [Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Anthony Liguori]

On 08/13/2011 10:05 PM, Avi Kivity wrote:
> On 08/12/2011 06:48 AM, Anthony Liguori wrote:
>>> target_phys_addr_t offset_in_region;
>>> AddrRange addr;
>>> uint8_t dirty_log_mask;
>>> + bool readable;
>> @@ -125,6 +125,7 @@ struct FlatRange {
>>
>> In a follow up, it might be good to add a comment explaining that this
>> whole readable thing is not just an optimization, but a hard
>> requirement for KVM in order to be able to execute code from ROM.
>
> This has nothing to do with kvm (in fact, I think we cannot support it
> under kvm with current interfaces). It's there to support devices that
> sometimes act as RAM and sometimes as mmio.

That is not a functional behavior but rather an optimization. 
Functionally speaking, there is absolutely no different between "acting 
as RAM" and "acting as mmio".

But you cannot remove the optimization because of the aforementioned 
limitation in KVM.

Maybe it's just me but I find this to be a very subtle detail so a 
comment would be helpful :-)

Regards,

Anthony Liguori

>
> We could also do it with a container and playing with subregions. I
> tried it and it was pretty complicated.
>

Re: [Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Alexander Graf]

Am 14.08.2011 um 06:29 schrieb Anthony Liguori <anthony@codemonkey.ws>:

> On 08/13/2011 10:05 PM, Avi Kivity wrote:
>> On 08/12/2011 06:48 AM, Anthony Liguori wrote:
>>>> target_phys_addr_t offset_in_region;
>>>> AddrRange addr;
>>>> uint8_t dirty_log_mask;
>>>> + bool readable;
>>> @@ -125,6 +125,7 @@ struct FlatRange {
>>> 
>>> In a follow up, it might be good to add a comment explaining that this
>>> whole readable thing is not just an optimization, but a hard
>>> requirement for KVM in order to be able to execute code from ROM.
>> 
>> This has nothing to do with kvm (in fact, I think we cannot support it
>> under kvm with current interfaces). It's there to support devices that
>> sometimes act as RAM and sometimes as mmio.
> 
> That is not a functional behavior but rather an optimization. Functionally speaking, there is absolutely no different between "acting as RAM" and "acting as mmio".
> 
> But you cannot remove the optimization because of the aforementioned limitation in KVM.
> 
> Maybe it's just me but I find this to be a very subtle detail so a comment would be helpful :-)

Well, TCG also can't execute from MMIO, right? So it's at least not KVM specific - TCG also gets the speedup from mapping roms as ram.


Alex

> 

Re: [Qemu-devel] [PATCH 2/3] memory: add API for creating ROM/device regions

[Anthony Liguori]

On 08/14/2011 10:43 AM, Alexander Graf wrote:
>
> Am 14.08.2011 um 06:29 schrieb Anthony Liguori<anthony@codemonkey.ws>:
>
>> On 08/13/2011 10:05 PM, Avi Kivity wrote:
>>> On 08/12/2011 06:48 AM, Anthony Liguori wrote:
>>>>> target_phys_addr_t offset_in_region;
>>>>> AddrRange addr;
>>>>> uint8_t dirty_log_mask;
>>>>> + bool readable;
>>>> @@ -125,6 +125,7 @@ struct FlatRange {
>>>>
>>>> In a follow up, it might be good to add a comment explaining that this
>>>> whole readable thing is not just an optimization, but a hard
>>>> requirement for KVM in order to be able to execute code from ROM.
>>>
>>> This has nothing to do with kvm (in fact, I think we cannot support it
>>> under kvm with current interfaces). It's there to support devices that
>>> sometimes act as RAM and sometimes as mmio.
>>
>> That is not a functional behavior but rather an optimization. Functionally speaking, there is absolutely no different between "acting as RAM" and "acting as mmio".
>>
>> But you cannot remove the optimization because of the aforementioned limitation in KVM.
>>
>> Maybe it's just me but I find this to be a very subtle detail so a comment would be helpful :-)
>
> Well, TCG also can't execute from MMIO, right?

Yes, it can.  To be fully correct, you basically have to.  During BIOS 
initialization, the BIOS uses PAM to move itself from ROM to RAM to 
speed up BIOS execution.  During this time period, memory reads come 
from one location (ROM) but writes go to another location (RAM).

Since we can't do this in KVM, SeaBIOS carries a hack to just skip this 
step if it detects its runnning on KVM.

> So it's at least not KVM specific - TCG also gets the speedup from mapping roms as ram.

Don't get me wrong, the optimization is important.  I'm just suggesting 
to document it since as can be demonstrated by this conversation, it's a 
subtle detail :-)

Regards,

Anthony Liguori

>
> Alex
>
>>
>

[Qemu-devel] [PATCH 3/3] memory: correct documentation typos

[Avi Kivity]

Noted by Drew Jones.

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

diff --git a/docs/memory.txt b/docs/memory.txt
index 4460c06..3fc1683 100644
--- a/docs/memory.txt
+++ b/docs/memory.txt
@@ -15,7 +15,7 @@ The memory model provides support for
  - setting up coalesced memory for kvm
  - setting up ioeventfd regions for kvm
 
-Memory is modelled as an tree (really acyclic graph) of MemoryRegion objects.
+Memory is modelled as a tree (really acyclic graph) of MemoryRegion objects.
 The root of the tree is memory as seen from the CPU's viewpoint (the system
 bus).  Nodes in the tree represent other buses, memory controllers, and
 memory regions that have been rerouted.  Leaves are RAM and MMIO regions.
@@ -87,7 +87,7 @@ guest accesses an address:
   descending priority order
   - if the address lies outside the region offset/size, the subregion is
     discarded
-  - if the subregion is a leaf (RAM or MMIO), the seach terminates
+  - if the subregion is a leaf (RAM or MMIO), the search terminates
   - if the subregion is a container, the same algorithm is used within the
     subregion (after the address is adjusted by the subregion offset)
   - if the subregion is an alias, the search is continues at the alias target
@@ -128,7 +128,7 @@ so-called PCI hole, that allows a 32-bit PCI bus to exist in a system with
 4GB of memory.
 
 The memory controller diverts addresses in the range 640K-768K to the PCI
-address space.  This is modeled using the "vga-window" alias, mapped at a
+address space.  This is modelled using the "vga-window" alias, mapped at a
 higher priority so it obscures the RAM at the same addresses.  The vga window
 can be removed by programming the memory controller; this is modelled by
 removing the alias and exposing the RAM underneath.
@@ -164,7 +164,7 @@ various constraints can be supplied to control how these callbacks are called:
  - .impl.min_access_size, .impl.max_access_size define the access sizes
    (in bytes) supported by the *implementation*; other access sizes will be
    emulated using the ones available.  For example a 4-byte write will be
-   emulated using four 1-byte write, is .impl.max_access_size = 1.
+   emulated using four 1-byte write, if .impl.max_access_size = 1.
  - .impl.valid specifies that the *implementation* only supports unaligned
    accesses; unaligned accesses will be emulated by two aligned accesses.
  - .old_portio and .old_mmio can be used to ease porting from code using
-- 
1.7.5.3

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

[Anthony Liguori]

On 08/08/2011 11:58 AM, Avi Kivity wrote:
> The following patches fix a resource leak, add a ROM/device API (for flash
> devices which act like memory when read, and as an mmio device when written),
> and correct typos in the documentation.

Applied all.  Thanks.

Regards,

Anthony Liguori

>
> Avi Kivity (3):
>    memory: reclaim resources when a memory region is destroyed for good
>    memory: add API for creating ROM/device regions
>    memory: correct documentation typos
>
>   docs/memory.txt |    8 +++---
>   memory.c        |   70 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   memory.h        |   35 +++++++++++++++++++++++++++
>   3 files changed, 107 insertions(+), 6 deletions(-)
>