diff mbox

qemu_ram_ptr_length: take ram_addr_t as arguments

Message ID alpine.DEB.2.00.1106241756410.12963@kaball-desktop
State New
Headers show

Commit Message

Stefano Stabellini June 27, 2011, 1:34 p.m. UTC 
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.


---

Comments

Peter Maydell June 27, 2011, 5 p.m. UTC | #1 
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
Stefano Stabellini June 27, 2011, 5:19 p.m. UTC | #2 
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 :/
diff mbox

Patch

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;