On 06/14/2012 09:13 AM, Takuya Yoshikawa wrote:
> On Wed, 13 Jun 2012 18:39:05 -0300
> Marcelo Tosatti <mtosatti@xxxxxxxxxx> 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.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
