This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
Home Products Support Community News


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

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