[Xen-devel] Re: [Qemu-devel] [PATCH v2 4/5] exec.c: refactor cpu_physical_memory_map

[Alexander Graf <agraf@xxxxxxx>]

On 22.07.2011, at 07:42, Liu Yu-B13201 wrote:

> 
> 
>> -----Original Message-----
>> From: qemu-devel-bounces+yu.liu=freescale.com@xxxxxxxxxx 
>> [mailto:qemu-devel-bounces+yu.liu=freescale.com@xxxxxxxxxx] 
>> On Behalf Of stefano.stabellini@xxxxxxxxxxxxx
>> Sent: Friday, May 20, 2011 1:36 AM
>> To: qemu-devel@xxxxxxxxxx
>> Cc: xen-devel@xxxxxxxxxxxxxxxxxxx; agraf@xxxxxxx; Stefano Stabellini
>> Subject: [Qemu-devel] [PATCH v2 4/5] exec.c: refactor 
>> cpu_physical_memory_map
>> 
>> From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>> 
>> 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.
>> Currently we are relying on the fact that all the pages are mapped
>> contiguously in qemu's address space: we have a check to make 
>> sure that
>> the virtual address returned by qemu_get_ram_ptr from the 
>> second call on
>> is consecutive. Now we are making this more explicit replacing all the
>> calls to qemu_get_ram_ptr with a single call to qemu_ram_ptr_length
>> passing a size argument.
>> 
>> Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
>> CC: agraf@xxxxxxx
>> CC: anthony@xxxxxxxxxxxxx
>> ---
>> cpu-common.h |    1 +
>> exec.c       |   51 
>> ++++++++++++++++++++++++++++++++++-----------------
>> 2 files changed, 35 insertions(+), 17 deletions(-)
>> 
>> diff --git a/cpu-common.h b/cpu-common.h
>> index 151c32c..085aacb 100644
>> --- a/cpu-common.h
>> +++ b/cpu-common.h
>> @@ -64,6 +64,7 @@ void qemu_ram_free(ram_addr_t addr);
>> void qemu_ram_remap(ram_addr_t addr, ram_addr_t length);
>> /* This should only be used for ram local to a device.  */
>> void *qemu_get_ram_ptr(ram_addr_t addr);
>> +void *qemu_ram_ptr_length(target_phys_addr_t addr, 
>> target_phys_addr_t *size);
>> /* Same but slower, to use for migration, where the order of
>>  * RAMBlocks must not change. */
>> void *qemu_safe_ram_ptr(ram_addr_t addr);
>> diff --git a/exec.c b/exec.c
>> index 21f21f0..ff9c174 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -3111,6 +3111,31 @@ void *qemu_safe_ram_ptr(ram_addr_t addr)
>>     return NULL;
>> }
>> 
>> +/* 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)
>> +{
>> +    if (xen_mapcache_enabled())
>> +        return qemu_map_cache(addr, *size, 1);
>> +    else {
>> +        RAMBlock *block;
>> +
>> +        QLIST_FOREACH(block, &ram_list.blocks, next) {
>> +            if (addr - block->offset < block->length) {
>> +                if (addr - block->offset + *size > block->length)
>> +                    *size = block->length - addr + block->offset;
>> +                return block->host + (addr - block->offset);
>> +            }
>> +        }
>> +
>> +        fprintf(stderr, "Bad ram offset %" PRIx64 "\n", 
>> (uint64_t)addr);
>> +        abort();
>> +
>> +        *size = 0;
>> +        return NULL;
>> +    }
>> +}
>> +
>> void qemu_put_ram_ptr(void *addr)
>> {
>>     trace_qemu_put_ram_ptr(addr);
>> @@ -3972,14 +3997,12 @@ void 
>> *cpu_physical_memory_map(target_phys_addr_t addr,
>>                               int is_write)
>> {
>>     target_phys_addr_t len = *plen;
>> -    target_phys_addr_t done = 0;
>> +    target_phys_addr_t todo = 0;
>>     int l;
>> -    uint8_t *ret = NULL;
>> -    uint8_t *ptr;
>>     target_phys_addr_t page;
>>     unsigned long pd;
>>     PhysPageDesc *p;
>> -    unsigned long addr1;
>> +    target_phys_addr_t addr1 = addr;
>> 
>>     while (len > 0) {
>>         page = addr & TARGET_PAGE_MASK;
>> @@ -3994,7 +4017,7 @@ void 
>> *cpu_physical_memory_map(target_phys_addr_t addr,
>>         }
>> 
>>         if ((pd & ~TARGET_PAGE_MASK) != IO_MEM_RAM) {
>> -            if (done || bounce.buffer) {
>> +            if (todo || bounce.buffer) {
>>                 break;
>>             }
>>             bounce.buffer = qemu_memalign(TARGET_PAGE_SIZE, 
>> TARGET_PAGE_SIZE);
>> @@ -4003,23 +4026,17 @@ void 
>> *cpu_physical_memory_map(target_phys_addr_t addr,
>>             if (!is_write) {
>>                 cpu_physical_memory_read(addr, bounce.buffer, l);
>>             }
>> -            ptr = bounce.buffer;
>> -        } else {
>> -            addr1 = (pd & TARGET_PAGE_MASK) + (addr & 
>> ~TARGET_PAGE_MASK);
>> -            ptr = qemu_get_ram_ptr(addr1);
>> -        }
>> -        if (!done) {
>> -            ret = ptr;
>> -        } else if (ret + done != ptr) {
>> -            break;
>> +
>> +            *plen = l;
>> +            return bounce.buffer;
>>         }
>> 
>>         len -= l;
>>         addr += l;
>> -        done += l;
>> +        todo += l;
>>     }
>> -    *plen = done;
>> -    return ret;
>> +    *plen = todo;
>> +    return qemu_ram_ptr_length(addr1, plen);
>> }
>> 
>> /* Unmaps a memory region previously mapped by 
>> cpu_physical_memory_map().
>> -- 
>> 1.7.2.3
> 
> Hello Stefano,
> 
> This commit breaks the case that guest memory doesn't start from 0.
> 
> In previous code
>       addr1 = (pd & TARGET_PAGE_MASK) + (addr & ~TARGET_PAGE_MASK);
> This transfer guest physical addr to qemu ram_addr, and so that it can pass 
> the ram range checking.
> 
> But current code
>       addr1 = addr
> this make it fail to pass the ram range checking.

Are you sure it's still broken with commit 
8ab934f93b5ad3d0af4ad419d2531235a75d672c? If so, mind to pinpoint where exactly?


Alex


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