Re: [PATCH v6 5/9] KVM: MMU: introduce SPTE_MMU_WRITEABLE bit

[Xiao Guangrong <xiaoguangrong@linux.vnet.ibm.com>]

On 06/14/2012 09:13 AM, Takuya Yoshikawa wrote:

> On Wed, 13 Jun 2012 18:39:05 -0300
> Marcelo Tosatti <mtosatti@redhat.com> wrote:
> 
>>>  /* Return true if the spte is dropped. */
>>> -static bool spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush)
>>> +static bool
>>> +spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush, bool pt_protect)
>>>  {
>>>  	u64 spte = *sptep;
>>>
>>> -	if (!is_writable_pte(spte))
>>> +	if (!is_writable_pte(spte) &&
>>> +	      !(pt_protect && spte_can_be_writable(spte)))
>>>  		return false;
>>>
>>>  	rmap_printk("rmap_write_protect: spte %p %llx\n", sptep, *sptep);
>>>
>>> -	*flush |= true;
>>>  	if (is_large_pte(spte)) {
>>>  		WARN_ON(page_header(__pa(sptep))->role.level ==
>>>  		       PT_PAGE_TABLE_LEVEL);
>>> +
>>> +		*flush |= true;
>>>  		drop_spte(kvm, sptep);
>>>  		--kvm->stat.lpages;
>>>  		return true;
>>>  	}
>>>
>>> +	if (pt_protect)
>>> +		spte &= ~SPTE_MMU_WRITEABLE;
>>>  	spte = spte & ~PT_WRITABLE_MASK;
>>> -	mmu_spte_update(sptep, spte);
>>> +
>>> +	*flush = mmu_spte_update(sptep, spte);
>>
>> This clears previous flush value when looping over multiple sptes in 
>> a single rmapp.
>>
> 
> I'm sorry but I have to say that this function is hard to understand.
> 
> /* Return true if the spte is dropped. */
> static bool
> spte_write_protect(struct kvm *kvm, u64 *sptep, bool *flush, bool pt_protect)
> 
> Even with the comment above, can we guess what this function will do
> for us without reading the body?
> 
> My feeling is that separate roles have been put into this one without
> explaining each parameter.
> 
> I think there are two solutions:
> 	1. separate this into a few functions
> 	2. explain each parameter/role properly in the comment
>


Okay.

I will add more comments and use drop_large_spte to cleanup it.