linux-kernel - Re: [PATCH v6 5/9] KVM: MMU: introduce SPTE_MMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 14 Jun 2012 10:13:28 +0900
From:	Takuya Yoshikawa <yoshikawa.takuya@....ntt.co.jp>
To:	Marcelo Tosatti <mtosatti@...hat.com>
Cc:	Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>,
	Avi Kivity <avi@...hat.com>,
	LKML <linux-kernel@...r.kernel.org>, KVM <kvm@...r.kernel.org>
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@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/