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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 24 Jun 2011, Peter Maydell wrote:
> On 24 June 2011 12:08,  <stefano.stabellini@xxxxxxxxxxxxx> wrote:
> > From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> >
> > 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;
 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel