From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53183) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIqMf-0007EQ-N7 for qemu-devel@nongnu.org; Tue, 03 Feb 2015 22:10:55 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YIqMa-0001z0-NF for qemu-devel@nongnu.org; Tue, 03 Feb 2015 22:10:53 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33590) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YIqMa-0001yf-Fb for qemu-devel@nongnu.org; Tue, 03 Feb 2015 22:10:48 -0500 Date: Wed, 4 Feb 2015 11:10:36 +0800 From: Fam Zheng Message-ID: <20150204031036.GD12948@ad.nay.redhat.com> References: <1422967948-3261-1-git-send-email-pbonzini@redhat.com> <1422967948-3261-7-git-send-email-pbonzini@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1422967948-3261-7-git-send-email-pbonzini@redhat.com> Subject: Re: [Qemu-devel] [PATCH 6/9] cosmetic changes preparing for the following patches List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Paolo Bonzini Cc: Mike Day , peter.maydell@linaro.org, qemu-devel@nongnu.org, fred.konrad@greensocs.com On Tue, 02/03 13:52, Paolo Bonzini wrote: > From: Mike Day > > Signed-off-by: Mike Day > Signed-off-by: Paolo Bonzini > --- > arch_init.c | 5 +-- > exec.c | 84 +++++++++++++++++++++++++++++++++----------------- > include/exec/cpu-all.h | 1 + > 3 files changed, 57 insertions(+), 33 deletions(-) > > diff --git a/arch_init.c b/arch_init.c > index 89c8fa4..b13f74b 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -688,9 +688,9 @@ static int ram_find_and_save_block(QEMUFile *f, bool last_stage) > } > } > } > + > last_seen_block = block; > last_offset = offset; > - > return bytes_sent; > } > > @@ -1117,7 +1117,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > ret = -EINVAL; > break; > } > - > ch = qemu_get_byte(f); > ram_handle_compressed(host, ch, TARGET_PAGE_SIZE); > break; > @@ -1128,7 +1127,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > ret = -EINVAL; > break; > } > - > qemu_get_buffer(f, host, TARGET_PAGE_SIZE); > break; > case RAM_SAVE_FLAG_XBZRLE: > @@ -1138,7 +1136,6 @@ static int ram_load(QEMUFile *f, void *opaque, int version_id) > ret = -EINVAL; > break; > } > - > if (load_xbzrle(f, addr, host) < 0) { > error_report("Failed to decompress XBZRLE page at " > RAM_ADDR_FMT, addr); > diff --git a/exec.c b/exec.c > index 05c5b44..8239370 100644 > --- a/exec.c > +++ b/exec.c > @@ -1265,11 +1265,12 @@ static RAMBlock *find_ram_block(ram_addr_t addr) > return NULL; > } > > +/* Called with iothread lock held. */ > void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev) > { > - RAMBlock *new_block = find_ram_block(addr); > - RAMBlock *block; > + RAMBlock *new_block, *block; > > + new_block = find_ram_block(addr); > assert(new_block); > assert(!new_block->idstr[0]); > > @@ -1282,7 +1283,6 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev) > } > pstrcat(new_block->idstr, sizeof(new_block->idstr), name); > > - /* This assumes the iothread lock is taken here too. */ > qemu_mutex_lock_ramlist(); > QTAILQ_FOREACH(block, &ram_list.blocks, next) { > if (block != new_block && !strcmp(block->idstr, new_block->idstr)) { > @@ -1294,10 +1294,17 @@ void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev) > qemu_mutex_unlock_ramlist(); > } > > +/* Called with iothread lock held. */ > void qemu_ram_unset_idstr(ram_addr_t addr) > { > - RAMBlock *block = find_ram_block(addr); > + RAMBlock *block; > > + /* FIXME: arch_init.c assumes that this is not called throughout > + * migration. Ignore the problem since hot-unplug during migration > + * does not work anyway. > + */ > + > + block = find_ram_block(addr); > if (block) { > memset(block->idstr, 0, sizeof(block->idstr)); > } > @@ -1585,7 +1592,6 @@ void qemu_ram_free(ram_addr_t addr) > } > } > qemu_mutex_unlock_ramlist(); > - > } > > #ifndef _WIN32 > @@ -1633,7 +1639,6 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length) > memory_try_enable_merging(vaddr, length); > qemu_ram_setup_dump(vaddr, length); > } > - return; Other changes are equivalent, but not quite for this one. But I think it is still correct, so: Reviewed-by: Fam Zheng > } > } > } > @@ -1641,49 +1646,60 @@ void qemu_ram_remap(ram_addr_t addr, ram_addr_t length) > > int qemu_get_ram_fd(ram_addr_t addr) > { > - RAMBlock *block = qemu_get_ram_block(addr); > + RAMBlock *block; > + int fd; > > - return block->fd; > + block = qemu_get_ram_block(addr); > + fd = block->fd; > + return fd; > } > > void *qemu_get_ram_block_host_ptr(ram_addr_t addr) > { > - RAMBlock *block = qemu_get_ram_block(addr); > + RAMBlock *block; > + void *ptr; > > - return ramblock_ptr(block, 0); > + block = qemu_get_ram_block(addr); > + ptr = ramblock_ptr(block, 0); > + return ptr; > } > > /* Return a host pointer to ram allocated with qemu_ram_alloc. > - With the exception of the softmmu code in this file, this should > - only be used for local memory (e.g. video ram) that the device owns, > - and knows it isn't going to access beyond the end of the block. > - > - It should not be used for general purpose DMA. > - Use cpu_physical_memory_map/cpu_physical_memory_rw instead. > + * This should not be used for general purpose DMA. Use address_space_map > + * or address_space_rw instead. For local memory (e.g. video ram) that the > + * device owns, use memory_region_get_ram_ptr. > */ > void *qemu_get_ram_ptr(ram_addr_t addr) > { > - RAMBlock *block = qemu_get_ram_block(addr); > + RAMBlock *block; > + void *ptr; > > - if (xen_enabled()) { > + block = qemu_get_ram_block(addr); > + > + if (xen_enabled() && block->host == NULL) { > /* We need to check if the requested address is in the RAM > * because we don't want to map the entire memory in QEMU. > * In that case just map until the end of the page. > */ > if (block->offset == 0) { > - return xen_map_cache(addr, 0, 0); > - } else if (block->host == NULL) { > - block->host = > - xen_map_cache(block->offset, block->max_length, 1); > + ptr = xen_map_cache(addr, 0, 0); > + goto done; > } > + > + block->host = xen_map_cache(block->offset, block->max_length, 1); > } > - return ramblock_ptr(block, addr - block->offset); > + ptr = ramblock_ptr(block, addr - block->offset); > + > +done: > + return ptr; > } > > /* Return a host pointer to guest's ram. Similar to qemu_get_ram_ptr > - * but takes a size argument */ > + * but takes a size argument. > + */ > static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size) > { > + void *ptr; > if (*size == 0) { > return NULL; > } > @@ -1691,12 +1707,12 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size) > return xen_map_cache(addr, *size, 1); > } else { > RAMBlock *block; > - > QTAILQ_FOREACH(block, &ram_list.blocks, next) { > if (addr - block->offset < block->max_length) { > if (addr - block->offset + *size > block->max_length) > *size = block->max_length - addr + block->offset; > - return ramblock_ptr(block, addr - block->offset); > + ptr = ramblock_ptr(block, addr - block->offset); > + return ptr; > } > } > > @@ -1706,15 +1722,24 @@ static void *qemu_ram_ptr_length(ram_addr_t addr, hwaddr *size) > } > > /* Some of the softmmu routines need to translate from a host pointer > - (typically a TLB entry) back to a ram offset. */ > + * (typically a TLB entry) back to a ram offset. > + * > + * By the time this function returns, the returned pointer is not protected > + * by RCU anymore. If the caller is not within an RCU critical section and > + * does not hold the iothread lock, it must have other means of protecting the > + * pointer, such as a reference to the region that includes the incoming > + * ram_addr_t. > + */ > MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr) > { > RAMBlock *block; > uint8_t *host = ptr; > + MemoryRegion *mr; > > if (xen_enabled()) { > *ram_addr = xen_ram_addr_from_mapcache(ptr); > - return qemu_get_ram_block(*ram_addr)->mr; > + mr = qemu_get_ram_block(*ram_addr)->mr; > + return mr; > } > > block = ram_list.mru_block; > @@ -1736,7 +1761,8 @@ MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr) > > found: > *ram_addr = block->offset + (host - block->host); > - return block->mr; > + mr = block->mr; > + return mr; > } > > static void notdirty_mem_write(void *opaque, hwaddr ram_addr, > diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h > index b8781d1..f7a3625 100644 > --- a/include/exec/cpu-all.h > +++ b/include/exec/cpu-all.h > @@ -277,6 +277,7 @@ struct RAMBlock { > ram_addr_t max_length; > void (*resized)(const char*, uint64_t length, void *host); > uint32_t flags; > + /* Protected by iothread lock. */ > char idstr[256]; > /* Reads can take either the iothread or the ramlist lock. > * Writes must take both locks. > -- > 1.8.3.1 > >