diff mbox

[2/3] memory: add API for creating ROM/device regions

Message ID 1312822730-9577-3-git-send-email-avi@redhat.com
State New
Headers show

Commit Message

Avi Kivity Aug. 8, 2011, 4:58 p.m. UTC
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(-)

Comments

Anthony Liguori Aug. 12, 2011, 1:48 p.m. UTC | #1
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
Avi Kivity Aug. 14, 2011, 3:05 a.m. UTC | #2
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.
Anthony Liguori Aug. 14, 2011, 1:29 p.m. UTC | #3
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.
>
Anthony Liguori Aug. 14, 2011, 1:51 p.m. UTC | #4
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
>
>>
>
Alexander Graf Aug. 14, 2011, 3:43 p.m. UTC | #5
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

>
diff mbox

Patch

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