Re: [Qemu-devel] [PATCH 06/11] exec.c: refactor cpu_physical_memory_map

[Peter Maydell <peter.maydell@linaro.org>]

On 19 June 2011 04:39, Alexander Graf <agraf@suse.de> wrote:
> From: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>
> Introduce qemu_ram_ptr_length that takes an address and a size as
> parameters rather than just an address.
>
> Refactor cpu_physical_memory_map so that we call qemu_ram_ptr_length only
> once rather than calling qemu_get_ram_ptr one time per page.
> This is not only more efficient but also tries to simplify the logic of
> the function.

This change breaks cpu_physical_memory_map() in the case where
the passed in physical memory address corresponds to a RAM block
which has been mapped in at multiple physical addresses. Specifically,
for Versatile Express this means we abort() when framebuffer_update_display()
calls cpu_physical_memory_map() for an address which is in the second
mapped area.

> +/* 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)

qemu_get_ram_ptr() takes a ramaddr_t, not a target_phys_addr_t;
so should this, because we're just looking through the ram_list
without doing physaddr to ramaddr translation.

Conceptually size should also be a ram_addr_t*, although if you
do that you can't just pass the plen pointer through to it.

The old implementation of cpu_physical_memory_map() worked
because it created the address to pass to qemu_get_ram_ptr()
from the p->phys_offset it got from phys_page_find(). The
new implementation needs to do something similar, not just pass
a target_phys_addr_t to qemu_ram_ptr_length().

> +        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", (uint64_t)addr);
> +        abort();
> +
> +        *size = 0;
> +        return NULL;

There doesn't seem much point in having code following an abort().

-- PMM