Messages in this thread | | | Date | Thu, 14 Jun 2012 10:13:28 +0900 | From | Takuya Yoshikawa <> | Subject | Re: [PATCH v6 5/9] KVM: MMU: introduce SPTE_MMU_WRITEABLE bit |
| |
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
Gaining a bit of lines by putting many things into one function will not help us IMO.
Thanks, Takuya
| |