On Fri, 03/25 07:58, Paolo Bonzini wrote:
>
>
> ----- Original Message -----
> > From: "Fam Zheng" <f...@redhat.com>
> > To: "Paolo Bonzini" <pbonz...@redhat.com>
> > Cc: qemu-devel@nongnu.org, "arei gonglei" <arei.gong...@huawei.com>,
> > m...@redhat.com
> > Sent: Friday, March 25, 2016 7:20:38 AM
> > Subject: Re: [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr
> > users
> >
> > On Thu, 03/24 12:03, Paolo Bonzini wrote:
> > > Let users of qemu_get_ram_ptr and qemu_ram_ptr_length pass in an
> > > address that is relative to the MemoryRegion. This basically means
> > > what address_space_translate returns.
> > >
> > > invalidate_and_set_dirty has to add back mr->ram_addr, but reads do
> > > not need it at all.
> > >
> > > Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
> > > ---
> > > exec.c | 40
> > > +++++++++++++++-------------------------
> > > include/exec/memory.h | 1 -
> > > memory.c | 4 ++--
> > > scripts/dump-guest-memory.py | 19 +++----------------
> > > 4 files changed, 20 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/exec.c b/exec.c
> > > index 001b669..ca9e3b6 100644
> > > --- a/exec.c
> > > +++ b/exec.c
> > > @@ -1876,6 +1876,7 @@ void *qemu_get_ram_ptr(RAMBlock *ram_block,
> > > ram_addr_t addr)
> >
> > Shall we rename the parameter to "offset" then? I don't know, but that
> > seems
> > easier to read for me.
>
> Good question. I'm not sure about that because of the block == NULL case,
> where the address is absolute.
>
> > > @@ -1924,7 +1924,7 @@ static void *qemu_ram_ptr_length(RAMBlock
> > > *ram_block,
> > > ram_addr_t addr,
> > > block->host = xen_map_cache(block->offset, block->max_length, 1);
> > > }
> > >
> > > - return ramblock_ptr(block, offset_inside_block);
> > > + return ramblock_ptr(block, addr);
> > > }
> > >
> > > /*
> > > @@ -2504,6 +2504,8 @@ static void invalidate_and_set_dirty(MemoryRegion
> > > *mr, hwaddr addr,
> > > hwaddr length)
> > > {
> > > uint8_t dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> > > + addr += memory_region_get_ram_addr(mr);
> > > +
> >
> > If called by address_space_unmap, is this addition still correct?
>
> No, thanks for the careful review! That's another opportunity
> for cleanup actually, splitting the (few) users of qemu_ram_addr_from_host
> that really need a ram_addr_t and those (the majority) that need a
> MemoryRegion and offset. They can use two different functions. I'll
> defer this to 2.7 and post the patches to do so later.
Good idea. The above "block == NULL" qemu_get_ram_ptr callers could use a
separate function, too - frankly I don't like that function interface too much.
What do you think?
Fam
