From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48182) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtLHf-0001ju-Pq for qemu-devel@nongnu.org; Tue, 25 Nov 2014 13:56:28 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XtLHa-0004zt-Gu for qemu-devel@nongnu.org; Tue, 25 Nov 2014 13:56:19 -0500 Received: from mx1.redhat.com ([209.132.183.28]:40752) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XtLHa-0004zV-9N for qemu-devel@nongnu.org; Tue, 25 Nov 2014 13:56:14 -0500 Date: Tue, 25 Nov 2014 18:55:55 +0000 From: "Dr. David Alan Gilbert" Message-ID: <20141125185554.GC11112@work-vm> References: <1412358473-31398-1-git-send-email-dgilbert@redhat.com> <1412358473-31398-42-git-send-email-dgilbert@redhat.com> <20141113025921.GC7291@voom.fritz.box> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20141113025921.GC7291@voom.fritz.box> Subject: Re: [Qemu-devel] [PATCH v4 41/47] qemu_ram_block_from_host List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: David Gibson Cc: aarcange@redhat.com, yamahata@private.email.ne.jp, quintela@redhat.com, cristian.klein@cs.umu.se, qemu-devel@nongnu.org, amit.shah@redhat.com, yanghy@cn.fujitsu.com * David Gibson (david@gibson.dropbear.id.au) wrote: > On Fri, Oct 03, 2014 at 06:47:47PM +0100, Dr. David Alan Gilbert (git) wrote: > > From: "Dr. David Alan Gilbert" > > > > Postcopy sends RAMBlock names and offsets over the wire (since it can't > > rely on the order of ramaddr being the same), and it starts out with > > HVA fault addresses from the kernel. > > > > qemu_ram_block_from_host translates a HVA into a RAMBlock, an offset > > in the RAMBlock, the global ram_addr_t value and it's bitmap position. > > s/it's/its/ fixed. > I find most of the passing around of bitmap positions confusing in > this patch series. Would it make things simpler if you broke up the > bitmap into (aligned) per-ramblock chunks. Then the offset would > determine the bitmap position, which is easier to understand since it > has an "inherent" meaning outside of the secondary data structure used > to track things. Yes it does get very confusing; there are two halves to the question though, source and destination. source: I didn't really want to change the way the existing migration structures work here; and my 'sent' bitmap is very similar to the migration bitmap. I think the reason that this is a single bitmap on the source is to make it easy/fast to search the bitmap for dirty pages; I don't know if there's any more detailed history behind it. destination: It might be possible to make that change; although I need to think about it; I'm going to need to keep a mapping similar to the RAMBlock list to get my data structure, or tack an individual PMI data onto each RAMBlock. Let me add that to a list to think about. > > Rewrite qemu_ram_addr_from_host to use qemu_ram_block_from_host. > > > > Provide qemu_ram_get_idstr since it's the actual name text sent on the > > wire. > > > > Signed-off-by: Dr. David Alan Gilbert > > --- > > exec.c | 56 ++++++++++++++++++++++++++++++++++++++++++----- > > include/exec/cpu-common.h | 4 ++++ > > 2 files changed, 55 insertions(+), 5 deletions(-) > > > > diff --git a/exec.c b/exec.c > > index 65ee612..07722b3 100644 > > --- a/exec.c > > +++ b/exec.c > > @@ -1246,6 +1246,11 @@ static RAMBlock *find_ram_block(ram_addr_t addr) > > return NULL; > > } > > > > +const char *qemu_ram_get_idstr(RAMBlock *rb) > > +{ > > + return rb->idstr; > > +} > > + > > void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev) > > { > > RAMBlock *new_block = find_ram_block(addr); > > @@ -1603,16 +1608,35 @@ 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. */ > > -MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr) > > +/* > > + * Translates a host ptr back to a RAMBlock, a ram_addr and an offset > > + * in that RAMBlock. > > + * > > + * ptr: Host pointer to look up > > + * round_offset: If true round the result offset down to a page boundary > > + * *ram_addr: set to result ram_addr > > + * *offset: set to result offset within the RAMBlock > > + * *bm_index: bitmap index (i.e. scaled ram_addr for use where the scale > > + * isn't available) > > + * > > + * Returns: RAMBlock (or NULL if not found) > > + */ > > +RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > > + ram_addr_t *ram_addr, > > + ram_addr_t *offset, > > + unsigned long *bm_index) > > { > > RAMBlock *block; > > uint8_t *host = ptr; > > > > if (xen_enabled()) { > > *ram_addr = xen_ram_addr_from_mapcache(ptr); > > - return qemu_get_ram_block(*ram_addr)->mr; > > + block = qemu_get_ram_block(*ram_addr); > > + if (!block) { > > + return NULL; > > + } > > + *offset = (host - block->host); > > + return block; > > } > > > > block = ram_list.mru_block; > > @@ -1633,7 +1657,29 @@ MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr) > > return NULL; > > > > found: > > - *ram_addr = block->offset + (host - block->host); > > + *offset = (host - block->host); > > + if (round_offset) { > > + *offset &= TARGET_PAGE_MASK; > > + } > > This seems clumsy. Surely the caller can apply the mask itself it it > wants that. That's true for what gets returned, but we're about to use that value again; although does that ever matter; I need to think about it. > > + *ram_addr = block->offset + *offset; > > + *bm_index = *ram_addr >> TARGET_PAGE_BITS; > > + return block; > > +} > > + > > +/* Some of the softmmu routines need to translate from a host pointer > > + (typically a TLB entry) back to a ram offset. */ > > +MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr) > > +{ > > + RAMBlock *block; > > + ram_addr_t offset; /* Not used */ > > + unsigned long index; /* Not used */ > > + > > + block = qemu_ram_block_from_host(ptr, false, ram_addr, &offset, &index); > > + > > + if (!block) { > > + return NULL; > > + } > > + > > return block->mr; > > } > > > > diff --git a/include/exec/cpu-common.h b/include/exec/cpu-common.h > > index 8042f50..ae25407 100644 > > --- a/include/exec/cpu-common.h > > +++ b/include/exec/cpu-common.h > > @@ -55,8 +55,12 @@ typedef uint32_t CPUReadMemoryFunc(void *opaque, hwaddr addr); > > void qemu_ram_remap(ram_addr_t addr, ram_addr_t length); > > /* This should not be used by devices. */ > > MemoryRegion *qemu_ram_addr_from_host(void *ptr, ram_addr_t *ram_addr); > > +RAMBlock *qemu_ram_block_from_host(void *ptr, bool round_offset, > > + ram_addr_t *ram_addr, ram_addr_t *offset, > > + unsigned long *bm_index); > > void qemu_ram_set_idstr(ram_addr_t addr, const char *name, DeviceState *dev); > > void qemu_ram_unset_idstr(ram_addr_t addr); > > +const char *qemu_ram_get_idstr(RAMBlock *rb); > > > > void cpu_physical_memory_rw(hwaddr addr, uint8_t *buf, > > int len, int is_write); > > -- > David Gibson | I'll have my music baroque, and my code > david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ > | _way_ _around_! > http://www.ozlabs.org/~dgibson -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK