From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yoshiaki Tamura Subject: Re: [RFC PATCH 01/20] Modify DIRTY_FLAG value and introduce DIRTY_IDX to use as indexes of bit-based phys_ram_dirty. Date: Fri, 23 Apr 2010 11:09:37 +0900 Message-ID: <4BD10161.8070904@lab.ntt.co.jp> References: <1271829445-5328-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <1271829445-5328-2-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <4BD0A2F7.4050207@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm@vger.kernel.org, qemu-devel@nongnu.org, avi@redhat.com, Anthony Liguori , mtosatti@redhat.com, ohmura.kei@lab.ntt.co.jp, yoshikawa.takuya@oss.ntt.co.jp To: Anthony Liguori Return-path: Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:60683 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752367Ab0DWCKA (ORCPT ); Thu, 22 Apr 2010 22:10:00 -0400 In-Reply-To: <4BD0A2F7.4050207@linux.vnet.ibm.com> Sender: kvm-owner@vger.kernel.org List-ID: Anthony Liguori wrote: > Hi, > > On 04/21/2010 12:57 AM, Yoshiaki Tamura wrote: >> Replaces byte-based phys_ram_dirty bitmap with four (MASTER, VGA, >> CODE, MIGRATION) bit-based phys_ram_dirty bitmap. On allocation, it >> sets all bits in the bitmap. It uses ffs() to convert DIRTY_FLAG to >> DIRTY_IDX. >> >> Modifies wrapper functions for byte-based phys_ram_dirty bitmap to >> bit-based phys_ram_dirty bitmap. MASTER works as a buffer, and upon >> get_diry() or get_dirty_flags(), it calls >> cpu_physical_memory_sync_master() to update VGA and MIGRATION. > > Why use an additional bitmap for MASTER instead of just updating the > VGA, CODE, and MIGRATION bitmaps together? This way we don't have to think whether we should update VGA or MIGRATION. IIRC, we had this discussion on upstream before with Avi? http://www.mail-archive.com/kvm@vger.kernel.org/msg30728.html BTW, I also have the following TODO list regarding dirty bitmap. 1. Allocate vga and migration dirty bitmap dynamically. 2. Separate CODE and the other dirty bitmaps functions. > > Regards, > > Anthony Liguori > >> Replaces direct phys_ram_dirty access with wrapper functions to >> prevent direct access to the phys_ram_dirty bitmap. >> >> Signed-off-by: Yoshiaki Tamura >> Signed-off-by: OHMURA Kei >> --- >> cpu-all.h | 130 >> +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---- >> exec.c | 60 ++++++++++++++-------------- >> 2 files changed, 152 insertions(+), 38 deletions(-) >> >> diff --git a/cpu-all.h b/cpu-all.h >> index 51effc0..3f8762d 100644 >> --- a/cpu-all.h >> +++ b/cpu-all.h >> @@ -37,6 +37,9 @@ >> >> #include "softfloat.h" >> >> +/* to use ffs in flag_to_idx() */ >> +#include >> + >> #if defined(HOST_WORDS_BIGENDIAN) != defined(TARGET_WORDS_BIGENDIAN) >> #define BSWAP_NEEDED >> #endif >> @@ -846,7 +849,6 @@ int cpu_str_to_log_mask(const char *str); >> /* memory API */ >> >> extern int phys_ram_fd; >> -extern uint8_t *phys_ram_dirty; >> extern ram_addr_t ram_size; >> extern ram_addr_t last_ram_offset; >> extern uint8_t *bios_mem; >> @@ -869,28 +871,140 @@ extern uint8_t *bios_mem; >> /* Set if TLB entry is an IO callback. */ >> #define TLB_MMIO (1<< 5) >> >> +/* Use DIRTY_IDX as indexes of bit-based phys_ram_dirty. */ >> +#define MASTER_DIRTY_IDX 0 >> +#define VGA_DIRTY_IDX 1 >> +#define CODE_DIRTY_IDX 2 >> +#define MIGRATION_DIRTY_IDX 3 >> +#define NUM_DIRTY_IDX 4 >> + >> +#define MASTER_DIRTY_FLAG (1<< MASTER_DIRTY_IDX) >> +#define VGA_DIRTY_FLAG (1<< VGA_DIRTY_IDX) >> +#define CODE_DIRTY_FLAG (1<< CODE_DIRTY_IDX) >> +#define MIGRATION_DIRTY_FLAG (1<< MIGRATION_DIRTY_IDX) >> + >> +extern unsigned long *phys_ram_dirty[NUM_DIRTY_IDX]; >> + >> +static inline int dirty_flag_to_idx(int flag) >> +{ >> + return ffs(flag) - 1; >> +} >> + >> +static inline int dirty_idx_to_flag(int idx) >> +{ >> + return 1<< idx; >> +} >> + >> int cpu_memory_rw_debug(CPUState *env, target_ulong addr, >> uint8_t *buf, int len, int is_write); >> >> -#define VGA_DIRTY_FLAG 0x01 >> -#define CODE_DIRTY_FLAG 0x02 >> -#define MIGRATION_DIRTY_FLAG 0x08 >> - >> /* read dirty bit (return 0 or 1) */ >> static inline int cpu_physical_memory_is_dirty(ram_addr_t addr) >> { >> - return phys_ram_dirty[addr>> TARGET_PAGE_BITS] == 0xff; >> + unsigned long mask; >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + >> + mask = 1UL<< offset; >> + return (phys_ram_dirty[MASTER_DIRTY_IDX][index]& mask) == mask; >> +} >> + >> +static inline void cpu_physical_memory_sync_master(ram_addr_t index) >> +{ >> + if (phys_ram_dirty[MASTER_DIRTY_IDX][index]) { >> + phys_ram_dirty[VGA_DIRTY_IDX][index] >> + |= phys_ram_dirty[MASTER_DIRTY_IDX][index]; >> + phys_ram_dirty[MIGRATION_DIRTY_IDX][index] >> + |= phys_ram_dirty[MASTER_DIRTY_IDX][index]; >> + phys_ram_dirty[MASTER_DIRTY_IDX][index] = 0UL; >> + } >> +} >> + >> +static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t addr) >> +{ >> + unsigned long mask; >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + int ret = 0, i; >> + >> + mask = 1UL<< offset; >> + cpu_physical_memory_sync_master(index); >> + >> + for (i = VGA_DIRTY_IDX; i<= MIGRATION_DIRTY_IDX; i++) { >> + if (phys_ram_dirty[i][index]& mask) { >> + ret |= dirty_idx_to_flag(i); >> + } >> + } >> + >> + return ret; >> +} >> + >> +static inline int cpu_physical_memory_get_dirty_idx(ram_addr_t addr, >> + int dirty_idx) >> +{ >> + unsigned long mask; >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + >> + mask = 1UL<< offset; >> + cpu_physical_memory_sync_master(index); >> + return (phys_ram_dirty[dirty_idx][index]& mask) == mask; >> } >> >> static inline int cpu_physical_memory_get_dirty(ram_addr_t addr, >> int dirty_flags) >> { >> - return phys_ram_dirty[addr>> TARGET_PAGE_BITS]& dirty_flags; >> + return cpu_physical_memory_get_dirty_idx(addr, >> + dirty_flag_to_idx(dirty_flags)); >> } >> >> static inline void cpu_physical_memory_set_dirty(ram_addr_t addr) >> { >> - phys_ram_dirty[addr>> TARGET_PAGE_BITS] = 0xff; >> + unsigned long mask; >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + >> + mask = 1UL<< offset; >> + phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask; >> +} >> + >> +static inline void cpu_physical_memory_set_dirty_range(ram_addr_t addr, >> + unsigned long mask) >> +{ >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + >> + phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask; >> +} >> + >> +static inline void cpu_physical_memory_set_dirty_flags(ram_addr_t addr, >> + int dirty_flags) >> +{ >> + unsigned long mask; >> + ram_addr_t index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + >> + mask = 1UL<< offset; >> + phys_ram_dirty[MASTER_DIRTY_IDX][index] |= mask; >> + >> + if (dirty_flags& CODE_DIRTY_FLAG) { >> + phys_ram_dirty[CODE_DIRTY_IDX][index] |= mask; >> + } >> +} >> + >> +static inline void cpu_physical_memory_mask_dirty_range(ram_addr_t >> start, >> + unsigned long length, >> + int dirty_flags) >> +{ >> + ram_addr_t addr = start, index; >> + unsigned long mask; >> + int offset, i; >> + >> + for (i = 0; i< length; i += TARGET_PAGE_SIZE) { >> + index = ((addr + i)>> TARGET_PAGE_BITS) / HOST_LONG_BITS; >> + offset = ((addr + i)>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1); >> + mask = ~(1UL<< offset); >> + phys_ram_dirty[dirty_flag_to_idx(dirty_flags)][index]&= mask; >> + } >> } >> >> void cpu_physical_memory_reset_dirty(ram_addr_t start, ram_addr_t end, >> diff --git a/exec.c b/exec.c >> index b647512..bf8d703 100644 >> --- a/exec.c >> +++ b/exec.c >> @@ -119,7 +119,7 @@ uint8_t *code_gen_ptr; >> >> #if !defined(CONFIG_USER_ONLY) >> int phys_ram_fd; >> -uint8_t *phys_ram_dirty; >> +unsigned long *phys_ram_dirty[NUM_DIRTY_IDX]; >> uint8_t *bios_mem; >> static int in_migration; >> >> @@ -1947,7 +1947,7 @@ static void tlb_protect_code(ram_addr_t ram_addr) >> static void tlb_unprotect_code_phys(CPUState *env, ram_addr_t ram_addr, >> target_ulong vaddr) >> { >> - phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS] |= CODE_DIRTY_FLAG; >> + cpu_physical_memory_set_dirty_flags(ram_addr, CODE_DIRTY_FLAG); >> } >> >> static inline void tlb_reset_dirty_range(CPUTLBEntry *tlb_entry, >> @@ -1968,8 +1968,7 @@ void cpu_physical_memory_reset_dirty(ram_addr_t >> start, ram_addr_t end, >> { >> CPUState *env; >> unsigned long length, start1; >> - int i, mask, len; >> - uint8_t *p; >> + int i; >> >> start&= TARGET_PAGE_MASK; >> end = TARGET_PAGE_ALIGN(end); >> @@ -1977,11 +1976,7 @@ void cpu_physical_memory_reset_dirty(ram_addr_t >> start, ram_addr_t end, >> length = end - start; >> if (length == 0) >> return; >> - len = length>> TARGET_PAGE_BITS; >> - mask = ~dirty_flags; >> - p = phys_ram_dirty + (start>> TARGET_PAGE_BITS); >> - for(i = 0; i< len; i++) >> - p[i]&= mask; >> + cpu_physical_memory_mask_dirty_range(start, length, dirty_flags); >> >> /* we modify the TLB cache so that the dirty bit will be set again >> when accessing the range */ >> @@ -2643,6 +2638,7 @@ extern const char *mem_path; >> ram_addr_t qemu_ram_alloc(ram_addr_t size) >> { >> RAMBlock *new_block; >> + int i; >> >> size = TARGET_PAGE_ALIGN(size); >> new_block = qemu_malloc(sizeof(*new_block)); >> @@ -2667,10 +2663,14 @@ ram_addr_t qemu_ram_alloc(ram_addr_t size) >> new_block->next = ram_blocks; >> ram_blocks = new_block; >> >> - phys_ram_dirty = qemu_realloc(phys_ram_dirty, >> - (last_ram_offset + size)>> TARGET_PAGE_BITS); >> - memset(phys_ram_dirty + (last_ram_offset>> TARGET_PAGE_BITS), >> - 0xff, size>> TARGET_PAGE_BITS); >> + for (i = MASTER_DIRTY_IDX; i< NUM_DIRTY_IDX; i++) { >> + phys_ram_dirty[i] >> + = qemu_realloc(phys_ram_dirty[i], >> + BITMAP_SIZE(last_ram_offset + size)); >> + memset((uint8_t *)phys_ram_dirty[i] + BITMAP_SIZE(last_ram_offset), >> + 0xff, BITMAP_SIZE(last_ram_offset + size) >> + - BITMAP_SIZE(last_ram_offset)); >> + } >> >> last_ram_offset += size; >> >> @@ -2833,16 +2833,16 @@ static void notdirty_mem_writeb(void *opaque, >> target_phys_addr_t ram_addr, >> uint32_t val) >> { >> int dirty_flags; >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> if (!(dirty_flags& CODE_DIRTY_FLAG)) { >> #if !defined(CONFIG_USER_ONLY) >> tb_invalidate_phys_page_fast(ram_addr, 1); >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> #endif >> } >> stb_p(qemu_get_ram_ptr(ram_addr), val); >> dirty_flags |= (0xff& ~CODE_DIRTY_FLAG); >> - phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS] = dirty_flags; >> + cpu_physical_memory_set_dirty_flags(ram_addr, dirty_flags); >> /* we remove the notdirty callback only if the code has been >> flushed */ >> if (dirty_flags == 0xff) >> @@ -2853,16 +2853,16 @@ static void notdirty_mem_writew(void *opaque, >> target_phys_addr_t ram_addr, >> uint32_t val) >> { >> int dirty_flags; >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> if (!(dirty_flags& CODE_DIRTY_FLAG)) { >> #if !defined(CONFIG_USER_ONLY) >> tb_invalidate_phys_page_fast(ram_addr, 2); >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> #endif >> } >> stw_p(qemu_get_ram_ptr(ram_addr), val); >> dirty_flags |= (0xff& ~CODE_DIRTY_FLAG); >> - phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS] = dirty_flags; >> + cpu_physical_memory_set_dirty_flags(ram_addr, dirty_flags); >> /* we remove the notdirty callback only if the code has been >> flushed */ >> if (dirty_flags == 0xff) >> @@ -2873,16 +2873,16 @@ static void notdirty_mem_writel(void *opaque, >> target_phys_addr_t ram_addr, >> uint32_t val) >> { >> int dirty_flags; >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> if (!(dirty_flags& CODE_DIRTY_FLAG)) { >> #if !defined(CONFIG_USER_ONLY) >> tb_invalidate_phys_page_fast(ram_addr, 4); >> - dirty_flags = phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS]; >> + dirty_flags = cpu_physical_memory_get_dirty_flags(ram_addr); >> #endif >> } >> stl_p(qemu_get_ram_ptr(ram_addr), val); >> dirty_flags |= (0xff& ~CODE_DIRTY_FLAG); >> - phys_ram_dirty[ram_addr>> TARGET_PAGE_BITS] = dirty_flags; >> + cpu_physical_memory_set_dirty_flags(ram_addr, dirty_flags); >> /* we remove the notdirty callback only if the code has been >> flushed */ >> if (dirty_flags == 0xff) >> @@ -3334,8 +3334,8 @@ void cpu_physical_memory_rw(target_phys_addr_t >> addr, uint8_t *buf, >> /* invalidate code */ >> tb_invalidate_phys_page_range(addr1, addr1 + l, 0); >> /* set dirty bit */ >> - phys_ram_dirty[addr1>> TARGET_PAGE_BITS] |= >> - (0xff& ~CODE_DIRTY_FLAG); >> + cpu_physical_memory_set_dirty_flags( >> + addr1, (0xff& ~CODE_DIRTY_FLAG)); >> } >> /* qemu doesn't execute guest code directly, but kvm does >> therefore flush instruction caches */ >> @@ -3548,8 +3548,8 @@ void cpu_physical_memory_unmap(void *buffer, >> target_phys_addr_t len, >> /* invalidate code */ >> tb_invalidate_phys_page_range(addr1, addr1 + l, 0); >> /* set dirty bit */ >> - phys_ram_dirty[addr1>> TARGET_PAGE_BITS] |= >> - (0xff& ~CODE_DIRTY_FLAG); >> + cpu_physical_memory_set_dirty_flags( >> + addr1, (0xff& ~CODE_DIRTY_FLAG)); >> } >> addr1 += l; >> access_len -= l; >> @@ -3685,8 +3685,8 @@ void stl_phys_notdirty(target_phys_addr_t addr, >> uint32_t val) >> /* invalidate code */ >> tb_invalidate_phys_page_range(addr1, addr1 + 4, 0); >> /* set dirty bit */ >> - phys_ram_dirty[addr1>> TARGET_PAGE_BITS] |= >> - (0xff& ~CODE_DIRTY_FLAG); >> + cpu_physical_memory_set_dirty_flags( >> + addr1, (0xff& ~CODE_DIRTY_FLAG)); >> } >> } >> } >> @@ -3754,8 +3754,8 @@ void stl_phys(target_phys_addr_t addr, uint32_t >> val) >> /* invalidate code */ >> tb_invalidate_phys_page_range(addr1, addr1 + 4, 0); >> /* set dirty bit */ >> - phys_ram_dirty[addr1>> TARGET_PAGE_BITS] |= >> - (0xff& ~CODE_DIRTY_FLAG); >> + cpu_physical_memory_set_dirty_flags(addr1, >> + (0xff& ~CODE_DIRTY_FLAG)); >> } >> } >> } > > > >