Re: [Qemu-devel] [PATCH v4 41/47] qemu_ram_block_from_host

["Dr. David Alan Gilbert" <dgilbert@redhat.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" <dgilbert@redhat.com>
> > 
> > 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 <dgilbert@redhat.com>
> > ---
> >  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