[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 07/11] exec: add a reference to the region retur
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 07/11] exec: add a reference to the region returned by address_space_translate |
Date: |
Fri, 28 Jun 2013 17:23:58 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/20130514 Thunderbird/17.0.6 |
Il 28/06/2013 15:41, Jan Kiszka ha scritto:
> On 2013-06-25 13:13, Paolo Bonzini wrote:
>> Once address_space_translate will only be protected by RCU, the returned
>> MemoryRegion can disappear as soon as the RCU read-side critical
>> section ends. Avoid this by adding a reference to the region, and
>> dropping it in the caller of address_space_translate.
>>
>> Signed-off-by: Paolo Bonzini <address@hidden>
>> ---
>> exec.c | 16 ++++++++++++++--
>> include/exec/memory.h | 3 ++-
>> 2 files changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/exec.c b/exec.c
>> index 5454e4d..b7f032d 100644
>> --- a/exec.c
>> +++ b/exec.c
>> @@ -286,6 +286,7 @@ MemoryRegion *address_space_translate(AddressSpace *as,
>> hwaddr addr,
>>
>> *plen = len;
>> *xlat = addr;
>> + memory_region_ref(mr);
>> return mr;
>> }
>>
>> @@ -1960,6 +1961,7 @@ bool address_space_rw(AddressSpace *as, hwaddr addr,
>> uint8_t *buf,
>> memcpy(buf, ptr, l);
>> }
>> }
>> + memory_region_unref(mr);
>> len -= l;
>> buf += l;
>> addr += l;
>> @@ -2125,8 +2127,10 @@ void *address_space_map(AddressSpace *as,
>> raddr = memory_region_get_ram_addr(mr) + xlat;
>> } else {
>> if (memory_region_get_ram_addr(mr) + xlat != raddr + todo) {
>> + memory_region_unref(mr);
>> break;
>> }
>> + memory_region_unref(mr);
>> }
>>
>> len -= l;
>
> This stage of the implementation looks inconsistent for
> memory_region_get_ram_addr. Isn't the idea so far that the mr reference
> is dropped again before leaving the function?
memory_region_get_ram_addr is not taking a reference.
address_space_map's address_space_translate is matched by the unref in
address_space_unmap. You're getting the same memory region multiple
times, and you have to unref it every time except the first.
But there is a missing unref at the end of address_space_map (bug
introduced when separating this patch from patch 11). I'll resend the
series.
Paolo
>> @@ -2226,6 +2230,7 @@ static inline uint32_t ldl_phys_internal(hwaddr addr,
>> break;
>> }
>> }
>> + memory_region_unref(mr);
>> return val;
>> }
>>
>> @@ -2285,6 +2290,7 @@ static inline uint64_t ldq_phys_internal(hwaddr addr,
>> break;
>> }
>> }
>> + memory_region_unref(mr);
>> return val;
>> }
>>
>> @@ -2352,6 +2358,7 @@ static inline uint32_t lduw_phys_internal(hwaddr addr,
>> break;
>> }
>> }
>> + memory_region_unref(mr);
>> return val;
>> }
>>
>> @@ -2399,6 +2406,7 @@ void stl_phys_notdirty(hwaddr addr, uint32_t val)
>> }
>> }
>> }
>> + memory_region_unref(mr);
>> }
>>
>> /* warning: addr must be aligned */
>> @@ -2440,6 +2448,7 @@ static inline void stl_phys_internal(hwaddr addr,
>> uint32_t val,
>> }
>> invalidate_and_set_dirty(addr1, 4);
>> }
>> + memory_region_unref(mr);
>> }
>>
>> void stl_phys(hwaddr addr, uint32_t val)
>> @@ -2503,6 +2512,7 @@ static inline void stw_phys_internal(hwaddr addr,
>> uint32_t val,
>> }
>> invalidate_and_set_dirty(addr1, 2);
>> }
>> + memory_region_unref(mr);
>> }
>>
>> void stw_phys(hwaddr addr, uint32_t val)
>> @@ -2592,11 +2602,13 @@ bool cpu_physical_memory_is_io(hwaddr phys_addr)
>> {
>> MemoryRegion*mr;
>> hwaddr l = 1;
>> + bool res;
>>
>> mr = address_space_translate(&address_space_memory,
>> phys_addr, &phys_addr, &l, false);
>>
>> - return !(memory_region_is_ram(mr) ||
>> - memory_region_is_romd(mr));
>> + res = !(memory_region_is_ram(mr) || memory_region_is_romd(mr));
>> + memory_region_unref(mr);
>> + return res;
>> }
>> #endif
>
> Unless I'm missing something ATM, address_space_access_valid,
> cpu_physical_memory_write_rom and tb_invalidate_phys_addr also require
> unref calls.
>
>> diff --git a/include/exec/memory.h b/include/exec/memory.h
>> index c842d48..2a37b2c 100644
>> --- a/include/exec/memory.h
>> +++ b/include/exec/memory.h
>> @@ -1003,7 +1003,8 @@ bool address_space_write(AddressSpace *as, hwaddr addr,
>> bool address_space_read(AddressSpace *as, hwaddr addr, uint8_t *buf, int
>> len);
>>
>> /* address_space_translate: translate an address range into an address space
>> - * into a MemoryRegion and an address range into that section
>> + * into a MemoryRegion and an address range into that section. Add a
>> reference
>> + * to that region.
>> *
>> * @as: #AddressSpace to be accessed
>> * @addr: address within that address space
>>
>
> Jan
>
- [Qemu-devel] [PATCH 00/11] Memory patches, part 4: region ownership, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 02/11] memory: destroy phys_sections one by one, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 03/11] exec: simplify destruction of the phys map, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 05/11] memory: add ref/unref, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 04/11] memory: add getter for owner, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 06/11] memory: add ref/unref calls, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 07/11] exec: add a reference to the region returned by address_space_translate, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 08/11] exec: check MRU in qemu_ram_addr_from_host, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 09/11] exec: move qemu_ram_addr_from_host_nofail to cputlb.c, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 11/11] memory: ref/unref memory across address_space_map/unmap, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 10/11] memory: return MemoryRegion from qemu_ram_addr_from_host, Paolo Bonzini, 2013/06/25
- [Qemu-devel] [PATCH 01/11] memory: add owner argument to initialization functions, Paolo Bonzini, 2013/06/25
- Re: [Qemu-devel] [PATCH 00/11] Memory patches, part 4: region ownership, Jan Kiszka, 2013/06/28