[Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[stefano.stabellini]

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(-)

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().
-- 
1.7.2.3

Re: [Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[Peter Maydell]

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

Re: [Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[Stefano Stabellini]

On Fri, 24 Jun 2011, Peter Maydell wrote:
> 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.)

I would rather fix qemu_ram_ptr_length to handle 0 as size argument, and
then call qemu_ram_ptr_length with 0 size from cpu_physical_memory_map
(see appended patch).


> 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

The compiler is not smart enough, unfortunately.


> (b) initialise it to an obviously
> bogus value if we have to do so to shut the compiler up.

All right, I am going to use ULONG_MAX.


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

Agreed, but it is the same as before :)
Do you have any better suggestion? Maybe raddr? I admit I am not very
imaginative with names.


---

diff --git a/exec.c b/exec.c
index 7f62332..e6dbddb 100644
--- a/exec.c
+++ b/exec.c
@@ -3137,6 +3137,8 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
  * but takes a size argument */
 void *qemu_ram_ptr_length(ram_addr_t addr, ram_addr_t *size)
 {
+    if (*size == 0)
+        return NULL;
     if (xen_mapcache_enabled())
         return qemu_map_cache(addr, *size, 1);
     else {
@@ -4017,7 +4019,7 @@ void *cpu_physical_memory_map(target_phys_addr_t addr,
     target_phys_addr_t page;
     unsigned long pd;
     PhysPageDesc *p;
-    ram_addr_t addr1 = addr;
+    ram_addr_t addr1 = ULONG_MAX;
     ram_addr_t rlen;
     void *raddr;
 

Re: [Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[Peter Maydell]

On 27 June 2011 14:34, Stefano Stabellini
<stefano.stabellini@eu.citrix.com> wrote:
> On Fri, 24 Jun 2011, Peter Maydell wrote:
>> 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.)
>
> I would rather fix qemu_ram_ptr_length to handle 0 as size argument, and
> then call qemu_ram_ptr_length with 0 size from cpu_physical_memory_map
> (see appended patch).

OK, that should work too.

>> (Also 'addr1' is not a fantastic variable name :-))
>
> Agreed, but it is the same as before :)
> Do you have any better suggestion? Maybe raddr? I admit I am not very
> imaginative with names.

I think raddr is better than addr1, yes.

> +    if (*size == 0)
> +        return NULL;

Incidentally, QEMU coding style has braces here. scripts/checkpatch.pl
can catch this kind of minor style nit for you (although it is not
infallible...)

-- PMM

Re: [Qemu-devel] [PATCH] qemu_ram_ptr_length: take ram_addr_t as arguments

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 1322 bytes --]

On Mon, 27 Jun 2011, Peter Maydell wrote:
> On 27 June 2011 14:34, Stefano Stabellini
> <stefano.stabellini@eu.citrix.com> wrote:
> > On Fri, 24 Jun 2011, Peter Maydell wrote:
> >> 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.)
> >
> > I would rather fix qemu_ram_ptr_length to handle 0 as size argument, and
> > then call qemu_ram_ptr_length with 0 size from cpu_physical_memory_map
> > (see appended patch).
> 
> OK, that should work too.
> 
> >> (Also 'addr1' is not a fantastic variable name :-))
> >
> > Agreed, but it is the same as before :)
> > Do you have any better suggestion? Maybe raddr? I admit I am not very
> > imaginative with names.
> 
> I think raddr is better than addr1, yes.

OK, I'll use that instead.


> > +    if (*size == 0)
> > +        return NULL;
> 
> Incidentally, QEMU coding style has braces here. scripts/checkpatch.pl
> can catch this kind of minor style nit for you (although it is not
> infallible...)

Thanks for the tip, I'll have to start using that script for real: I
work on three different projects and they all have very very similar
code styles apart from this rule about the braces in one line
statements, my brain cannot cope :/