lkml.org 
[lkml]   [2012]   [Jun]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2012-06-14 03:41    [W:0.089 / U:1.504 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site