[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram
From: |
Paolo Bonzini |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] memory: hide mr->ram_addr from qemu_get_ram_ptr users |
Date: |
Fri, 25 Mar 2016 07:58:02 -0400 (EDT) |
----- Original Message -----
> From: "Fam Zheng" <address@hidden>
> To: "Paolo Bonzini" <address@hidden>
> Cc: address@hidden, "arei gonglei" <address@hidden>, address@hidden
> 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 <address@hidden>
> > ---
> > 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.
> > @@ -3382,13 +3374,13 @@ void address_space_stl_notdirty(AddressSpace *as,
> > hwaddr addr, uint32_t val,
> >
> > r = memory_region_dispatch_write(mr, addr1, val, 4, attrs);
> > } else {
> > - addr1 += memory_region_get_ram_addr(mr);
> > ptr = qemu_get_ram_ptr(mr->ram_block, addr1);
> > stl_p(ptr, val);
> >
> > dirty_log_mask = memory_region_get_dirty_log_mask(mr);
> > dirty_log_mask &= ~(1 << DIRTY_MEMORY_CODE);
> > - cpu_physical_memory_set_dirty_range(addr1, 4, dirty_log_mask);
> > + cpu_physical_memory_set_dirty_range(memory_region_get_ram_addr(mr)
> > + addr,
>
> Is this line too long?
It's 82 characters
Paolo