Re: [PATCH 2/2] kvm: change the dirty page tracking to work with dirty bity

From: Marcelo Tosatti <mtosatti@redhat.com>
To: Izik Eidus <ieidus@redhat.com>
Cc: kvm@vger.kernel.org, avi@redhat.com
Subject: Re: [PATCH 2/2] kvm: change the dirty page tracking to work with dirty bity
Date: Wed, 10 Jun 2009 22:04:13 -0300	[thread overview]
Message-ID: <20090611010413.GB15350@amt.cnet> (raw)
In-Reply-To: <4A30484E.1040708@redhat.com>

On Thu, Jun 11, 2009 at 02:57:02AM +0300, Izik Eidus wrote:
>> You can avoid the mark_page_dirty if SPTE_DONT_DIRTY? (which is a good idea,
>> gfn_to_memslot_unaliased and friends show up high in profiling).
>>   
>
> This code shouldnt run on anything but EPT, shadow_dirty_mask should be  
> set to zero only on EPT that doesnt have dirty bit tracking, so we still  
> need to mark the page dirty from this context...

Right, i misunderstood you were not skipping it for
kvm_x86_ops->have_dirty_bit() case.

>>> --- a/virt/kvm/kvm_main.c
>>> +++ b/virt/kvm/kvm_main.c
>>> @@ -1135,8 +1135,11 @@ int __kvm_set_memory_region(struct kvm *kvm,
>>>  	}
>>>   	/* Free page dirty bitmap if unneeded */
>>> -	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES))
>>> +	if (!(new.flags & KVM_MEM_LOG_DIRTY_PAGES)) {
>>>  		new.dirty_bitmap = NULL;
>>> +		if (old.flags & KVM_MEM_LOG_DIRTY_PAGES)
>>> +			kvm_arch_flush_shadow(kvm);
>>> +	}
>>>     
>>
>> Whats this for?
>>   
>
> We have added all this SPTE_DONT_DIRTY..., when we stop dirty bit  
> tracking, we want to continue setting the dirty bit for the spte inside  
> set_spte(), so writing to the page would be faster....

Right... so instead of using kvm_arch_flush_shadow make a
"kvm_arch_stop_dirty_logging(kvm, slot)", and clear the SPTE_DONT_DIRTY
bits?

>
>> The idea of making dirty bit accessible (also can use it to map host
>> ptes read-only, when guest fault is read-only, for KSM (*)) is good. But
>> better first introduce infrastructure to handle dirty bit (making sure
>> the bit is transferred properly), then have logging make use of it.
>>   
>
> ???, I dont understand it much, it mean you want to continue trying  
> walking in that direction of the patch? or you want some other way?

What i'm saying is with shadow and NPT (i believe) you can mark a spte
writable but not dirty, which gives you the ability to know whether
certain pages have been dirtied.

Should be ok todo that with shadow, as long as the gpte is already
dirty, right? So:

- guest create writable pte
- guest write fault on that pte
- vmexit
- write dirty bit to gpte
- create writable spte
-> 

from this point on, you can create a writable spte which is not dirtied, 
and use the dirty bit to know whether a page has been written to?

And unless i'm mistaken NPT gives you that for free since the hw updates
the spte with accessed/dirty bits.

Maybe this is a stupid idea. 

>> * EPT violations do no transfer fault information to the page fault
>> handler, but its available (there's a vm-exit field).
>>   
>
> EPT in this patch, run as before, (using page faults with  
> mark_page_dirty() inside set_spte())

Right, i was talking about gup(write=0) when guest fault is read-only to
relief KSM of wrprotecting host ptes, but that is quite different.

>>   
>>>  	r = -ENOMEM;
>>>  @@ -1279,6 +1282,7 @@ int kvm_get_dirty_log(struct kvm *kvm,
>>>  	if (!memslot->dirty_bitmap)
>>>  		goto out;
>>>  +	kvm_arch_get_dirty_log(kvm, memslot);
>>>  	n = ALIGN(memslot->npages, BITS_PER_LONG) / 8;
>>>   	for (i = 0; !any && i < n/sizeof(long); ++i)
>>> -- 
>>> 1.5.6.5
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe kvm" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>     
>
>
> Thanks :).