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

[Takuya Yoshikawa]

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

Gaining a bit of lines by putting many things into one function will
not help us IMO.

Thanks,
	Takuya
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/