Patchwork qemu_ram_ptr_length: take ram_addr_t as arguments

login
register
mail settings
Submitter Stefano Stabellini
Date June 24, 2011, 11:08 a.m.
Message ID <1308913718-7201-1-git-send-email-stefano.stabellini@eu.citrix.com>
Download mbox | patch
Permalink /patch/101791/
State New
Headers show

Comments

Stefano Stabellini - June 24, 2011, 11:08 a.m.
From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

qemu_ram_ptr_length should take ram_addr_t as argument rather than
target_phys_addr_t because is doing comparisons with RAMBlock addresses.

cpu_physical_memory_map should create a ram_addr_t address to pass to
qemu_ram_ptr_length from PhysPageDesc phys_offset.

Remove code after abort() in qemu_ram_ptr_length.

Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
---
 cpu-common.h |    2 +-
 exec.c       |   18 +++++++++++-------
 2 files changed, 12 insertions(+), 8 deletions(-)
Peter Maydell - June 24, 2011, 4:37 p.m.
On 24 June 2011 12:08,  <stefano.stabellini@eu.citrix.com> wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> qemu_ram_ptr_length should take ram_addr_t as argument rather than
> target_phys_addr_t because is doing comparisons with RAMBlock addresses.
>
> cpu_physical_memory_map should create a ram_addr_t address to pass to
> qemu_ram_ptr_length from PhysPageDesc phys_offset.
>
> Remove code after abort() in qemu_ram_ptr_length.

This does fix vexpress. However I think we're still doing the wrong
thing if the bounce buffer is already in use and addr points at an
IO page. In the old code, we would break out of the loop on the
if (done || bounce.buffer) condition, set *plen to 0 [because done==0
since this is the first page] and return. Now we break out of the
loop but will fall into the call to qemu_ram_ptr_length() with a
bogus addr1 and probably abort().

You probably want to only call qemu_ram_ptr_length() if (todo).
(I don't know if anybody ever calls this routine with a zero input
length, but that would handle that case too.)
It would also be better to either (a) not initialise addr1, if
the compiler is smart enough to know it can't get to the use
without it being initialised or (b) initialise it to an obviously
bogus value if we have to do so to shut the compiler up.

(Also 'addr1' is not a fantastic variable name :-))

-- PMM

Patch

diff --git a/cpu-common.h b/cpu-common.h
index 085aacb..ceaa631 100644
--- a/cpu-common.h
+++ b/cpu-common.h
@@ -64,7 +64,7 @@  void qemu_ram_free(ram_addr_t addr);
 void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
 /* This should only be used for ram local to a device.  */
 void *qemu_get_ram_ptr(ram_addr_t addr);
-void *qemu_ram_ptr_length(target_phys_addr_t addr, target_phys_addr_t *size);
+void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size);
 /* Same but slower, to use for migration, where the order of
  * RAMBlocks must not change. */
 void *qemu_safe_ram_ptr(ram_addr_t addr);
diff --git a/exec.c b/exec.c
index aebb23b..5b9390e 100644
--- a/exec.c
+++ b/exec.c
@@ -3115,7 +3115,7 @@  void *qemu_safe_ram_ptr(ram_addr_t addr)
 
 /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr
  * but takes a size argument */
-void *qemu_ram_ptr_length(target_phys_addr_t addr, target_phys_addr_t *size)
+void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
 {
     if (xen_mapcache_enabled())
         return qemu_map_cache(addr, *size, 1);
@@ -3132,9 +3132,6 @@  void *qemu_ram_ptr_length(target_phys_addr_t addr, target_phys_addr_t *size)
 
         fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
         abort();
-
-        *size = 0;
-        return NULL;
     }
 }
 
@@ -4000,7 +3997,9 @@  void *cpu_physical_memory_map(target_phys_addr_t addr,
     target_phys_addr_t page;
     unsigned long pd;
     PhysPageDesc *p;
-    target_phys_addr_t addr1 = addr;
+    ram_addr_t addr1 = addr;
+    ram_addr_t rlen;
+    void *raddr;
 
     while (len > 0) {
         page = addr & TARGET_PAGE_MASK;
@@ -4028,13 +4027,18 @@  void *cpu_physical_memory_map(target_phys_addr_t addr,
             *plen = l;
             return bounce.buffer;
         }
+        if (!todo) {
+            addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
+        }
 
         len -= l;
         addr += l;
         todo += l;
     }
-    *plen = todo;
-    return qemu_ram_ptr_length(addr1, plen);
+    rlen = todo;
+    raddr = qemu_ram_ptr_length(addr1, &rlen);
+    *plen = rlen;
+    return raddr;
 }
 
 /* Unmaps a memory region previously mapped by cpu_physical_memory_map().