[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v2 10/11] exec: reorganize address_space_map
From: |
Jan Kiszka |
Subject: |
Re: [Qemu-devel] [PATCH v2 10/11] exec: reorganize address_space_map |
Date: |
Mon, 01 Jul 2013 20:34:44 +0200 |
User-agent: |
Mozilla/5.0 (X11; U; Linux i686 (x86_64); de; rv:1.8.1.12) Gecko/20080226 SUSE/2.0.0.12-1.1 Thunderbird/2.0.0.12 Mnenhy/0.7.5.666 |
On 2013-06-28 18:58, Paolo Bonzini wrote:
> First of all, rename "todo" to "done".
>
> Second, clearly separate the case of done == 0 from the case of done != 0.
> This will help handling reference counting in the next patch.
>
> Third, this test:
>
> if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
>
> does not guarantee that the memory region is the same across two iterations
> of the while loop. For example, you could have two blocks:
>
> A) size 640 K, mapped at physical address 0, ram_addr_t 0
> B) size 64 K, mapped at physical address 0xa0000, ram_addr_t 0xa0000
>
> then mapping 1 M starting at physical address zero will erroneously treat
> B as the continuation of block A. qemu_ram_ptr_length ensures that no
> invalid memory is accessed, but it is still a pointless complication of
> the algorithm. The patch makes the logic clearer with an explicit test
> that the memory region is the same.
>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
> exec.c | 71
> +++++++++++++++++++++++++++++++++++-------------------------------
> 1 file changed, 38 insertions(+), 33 deletions(-)
>
> diff --git a/exec.c b/exec.c
> index a372963..ea79aea 100644
> --- a/exec.c
> +++ b/exec.c
> @@ -2073,47 +2073,52 @@ void *address_space_map(AddressSpace *as,
> bool is_write)
> {
> hwaddr len = *plen;
> - hwaddr todo = 0;
> - hwaddr l, xlat;
> - MemoryRegion *mr;
> - ram_addr_t raddr = RAM_ADDR_MAX;
> - ram_addr_t rlen;
> - void *ret;
> + hwaddr done = 0;
> + hwaddr l, xlat, base;
> + MemoryRegion *mr, *this_mr;
> + ram_addr_t raddr;
>
> - while (len > 0) {
> - l = len;
> - mr = address_space_translate(as, addr, &xlat, &l, is_write);
> -
> - if (!memory_access_is_direct(mr, is_write)) {
> - if (todo || bounce.buffer) {
> - break;
> - }
> - bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE,
> TARGET_PAGE_SIZE);
> - bounce.addr = addr;
> - bounce.len = l;
> - if (!is_write) {
> - address_space_read(as, addr, bounce.buffer, l);
> - }
> + if (len == 0) {
> + return NULL;
> + }
>
> - *plen = l;
> - return bounce.buffer;
> + l = len;
> + mr = address_space_translate(as, addr, &xlat, &l, is_write);
> + if (!memory_access_is_direct(mr, is_write)) {
> + if (bounce.buffer) {
> + return NULL;
> }
> - if (!todo) {
> - raddr = memory_region_get_ram_addr(mr) + xlat;
> - } else {
> - if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
> - break;
> - }
> + bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, TARGET_PAGE_SIZE);
> + bounce.addr = addr;
> + bounce.len = l;
> + if (!is_write) {
> + address_space_read(as, addr, bounce.buffer, l);
> }
>
> + *plen = l;
> + return bounce.buffer;
> + }
> +
> + base = xlat;
> + raddr = memory_region_get_ram_addr(mr);
> +
> + for (;;) {
> len -= l;
> addr += l;
> - todo += l;
> + done += l;
> + if (len == 0) {
> + break;
> + }
> +
> + l = len;
> + this_mr = address_space_translate(as, addr, &xlat, &l, is_write);
> + if (this_mr != mr || xlat != base + done) {
> + break;
> + }
> }
> - rlen = todo;
> - ret = qemu_ram_ptr_length(raddr, &rlen);
> - *plen = rlen;
> - return ret;
> +
> + *plen = done;
> + return qemu_ram_ptr_length(raddr + base, plen);
> }
>
> /* Unmaps a memory region previously mapped by address_space_map().
>
The transformation looks correct, but I'm currently not understanding in
which cases we still need the loop. address_space_translate should
return as much of the target mr as the region can contiguously provide, no?
Jan
--
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux
- Re: [Qemu-devel] [PATCH v2 10/11] exec: reorganize address_space_map,
Jan Kiszka <=