| Message ID | 1372158807-19715-8-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
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 <pbonzini@redhat.com> > --- > 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? > @@ -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
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 <pbonzini@redhat.com> >> --- >> 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 >
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; @@ -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 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
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 <pbonzini@redhat.com> --- exec.c | 16 ++++++++++++++-- include/exec/memory.h | 3 ++- 2 files changed, 16 insertions(+), 3 deletions(-)