On 06/14/2012 09:13 AM, Takuya Yoshikawa wrote: > On Wed, 13 Jun 2012 18:39:05 -0300 > Marcelo Tosatti <[email protected]> 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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
