From mboxrd@z Thu Jan  1 00:00:00 1970
From: Anthony Liguori <anthony@codemonkey.ws>
Subject: Re: [PATCH 2/6] qemu-kvm: Modify and introduce wrapper functions
 to access phys_ram_dirty.
Date: Tue, 16 Mar 2010 09:50:05 -0500
Message-ID: <4B9F9A9D.6000302@codemonkey.ws>
References: <1268736839-27371-1-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <1268736839-27371-3-git-send-email-tamura.yoshiaki@lab.ntt.co.jp> <4B9F7D78.5090201@redhat.com> <4B9F8502.3070108@lab.ntt.co.jp> <4B9F87A9.3070509@redhat.com> <4B9F8CE2.7010104@codemonkey.ws> <4B9F8E4C.5070307@redhat.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1; format=flowed
Content-Transfer-Encoding: 7bit
Cc: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>,
	kvm@vger.kernel.org, qemu-devel@nongnu.org,
	ohmura.kei@lab.ntt.co.jp
To: Avi Kivity <avi@redhat.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-gx0-f217.google.com ([209.85.217.217]:61971 "EHLO
	mail-gx0-f217.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S966435Ab0CPOuI (ORCPT <rfc822;kvm@vger.kernel.org>);
	Tue, 16 Mar 2010 10:50:08 -0400
Received: by gxk9 with SMTP id 9so2894gxk.8
        for <kvm@vger.kernel.org>; Tue, 16 Mar 2010 07:50:07 -0700 (PDT)
In-Reply-To: <4B9F8E4C.5070307@redhat.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

On 03/16/2010 08:57 AM, Avi Kivity wrote:
> On 03/16/2010 03:51 PM, Anthony Liguori wrote:
>> On 03/16/2010 08:29 AM, Avi Kivity wrote:
>>> On 03/16/2010 03:17 PM, Yoshiaki Tamura wrote:
>>>> Avi Kivity wrote:
>>>>> On 03/16/2010 12:53 PM, Yoshiaki Tamura wrote:
>>>>>> Modifies wrapper functions for byte-based phys_ram_dirty bitmap to
>>>>>> bit-based phys_ram_dirty bitmap, and adds more wrapper functions to
>>>>>> prevent
>>>>>> direct access to the phys_ram_dirty bitmap.
>>>>>
>>>>>> +
>>>>>> +static inline int cpu_physical_memory_get_dirty_flags(ram_addr_t 
>>>>>> addr)
>>>>>> +{
>>>>>> + unsigned long mask;
>>>>>> + int index = (addr>> TARGET_PAGE_BITS) / HOST_LONG_BITS;
>>>>>> + int offset = (addr>> TARGET_PAGE_BITS)& (HOST_LONG_BITS - 1);
>>>>>> + int ret = 0;
>>>>>> +
>>>>>> + mask = 1UL<< offset;
>>>>>> + if (phys_ram_vga_dirty[index]& mask)
>>>>>> + ret |= VGA_DIRTY_FLAG;
>>>>>> + if (phys_ram_code_dirty[index]& mask)
>>>>>> + ret |= CODE_DIRTY_FLAG;
>>>>>> + if (phys_ram_migration_dirty[index]& mask)
>>>>>> + ret |= MIGRATION_DIRTY_FLAG;
>>>>>> +
>>>>>> + return ret;
>>>>>> }
>>>>>>
>>>>>> 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_flags(addr)& dirty_flags;
>>>>>> }
>>>>>
>>>>> This turns one cacheline access into three. If the dirty bitmaps 
>>>>> were in
>>>>> an array, you could do
>>>>>
>>>>> return dirty_bitmaps[dirty_index][addr >> (TARGET_PAGE_BITS +
>>>>> BITS_IN_LONG)] & mask;
>>>>>
>>>>> with one cacheline access.
>>>>
>>>> If I'm understanding the existing code correctly,
>>>> int dirty_flags can be combined, like VGA + MIGRATION.
>>>> If we only have to worry about a single dirty flag, I agree with 
>>>> your idea.
>>>
>>> From a quick grep it seems flags are not combined, except for 
>>> something strange with CODE_DIRTY_FLAG:
>>>
>>>> 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];
>>>>     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];
>>>> #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;
>>>>     /* we remove the notdirty callback only if the code has been
>>>>        flushed */
>>>>     if (dirty_flags == 0xff)
>>>>         tlb_set_dirty(cpu_single_env, cpu_single_env->mem_io_vaddr);
>>>> }
>>>
>>> I can't say I understand what it does.
>>
>> The semantics of CODE_DIRTY_FLAG are a little counter intuitive.  
>> CODE_DIRTY_FLAG means that we know that something isn't code so 
>> writes do not need checking for self modifying code.
>
> So the hardware equivalent is, when the Instruction TLB loads a page 
> address, clear CODE_DIRTY_FLAG?

Yes, and is what tlb_protect_code() does and it's called from 
tb_alloc_page() which is what's code when a TB is created.

Regards,

Anthony Liguori