[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v4 41/47] qemu_ram_block_from_host
From: |
Dr. David Alan Gilbert |
Subject: |
Re: [Qemu-devel] [PATCH v4 41/47] qemu_ram_block_from_host |
Date: |
Tue, 25 Nov 2014 18:55:55 +0000 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
* David Gibson (address@hidden) wrote:
> On Fri, Oct 03, 2014 at 06:47:47PM +0100, Dr. David Alan Gilbert (git) wrote:
> > From: "Dr. David Alan Gilbert" <address@hidden>
> >
> > 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 <address@hidden>
> > ---
> > 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 / address@hidden / Manchester, UK