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:41:58 +0800
From:	Xiao Guangrong <xiaoguangrong@...ux.vnet.ibm.com>
To:	Takuya Yoshikawa <yoshikawa.takuya@....ntt.co.jp>
CC:	Marcelo Tosatti <mtosatti@...hat.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 06/14/2012 09:13 AM, Takuya Yoshikawa wrote:

> 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
>


Okay.

I will add more comments and use drop_large_spte to cleanup it.

--
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/