Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps

From: Avi Kivity <avi@redhat.com>
To: Marcelo Tosatti <mtosatti@redhat.com>
Cc: sheng@linux.intel.com, kvm@vger.kernel.org
Subject: Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps
Date: Mon, 08 Jun 2009 12:24:08 +0300	[thread overview]
Message-ID: <4A2CD8B8.2050308@redhat.com> (raw)
In-Reply-To: <20090602214226.820226306@localhost.localdomain>

Marcelo Tosatti wrote:
> Under testing, count_writable_mappings returns a value that is 2 integers
> larger than what count_rmaps returns.
>
> Suspicion is that either of the two functions is counting a duplicate (either
> positively or negatively). 
>
> Modifying check_writable_mappings_rmap to check for rmap existance on
> all present MMU pages fails to trigger an error, which should keep Avi
> happy.
>
> Also introduce mmu_spte_walk to invoke a callback on all present sptes visible
> to the current vcpu, might be useful in the future.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> Index: kvm/arch/x86/kvm/mmu.c
> ===================================================================
> --- kvm.orig/arch/x86/kvm/mmu.c
> +++ kvm/arch/x86/kvm/mmu.c
> @@ -3017,6 +3017,55 @@ static gva_t canonicalize(gva_t gva)
>  	return gva;
>  }
>  
> +
> +typedef void (*inspect_spte_fn) (struct kvm *kvm, struct kvm_mmu_page *sp,
> +				 u64 *sptep);
> +
> +static void __mmu_spte_walk(struct kvm *kvm, struct kvm_mmu_page *sp,
> +			    inspect_spte_fn fn)
> +{
> +	int i;
> +
> +	for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
> +		u64 ent = sp->spt[i];
> +
> +		if (is_shadow_present_pte(ent)) {
> +			if (sp->role.level > 1) {
>   

I think this is broken wrt large pages.  We should recurse if role.level 
 > 1 or the G bit is set.

> +	if (*sptep & PT_WRITABLE_MASK) {
> +		rev_sp = page_header(__pa(sptep));
> +		gfn = rev_sp->gfns[sptep - rev_sp->spt];
> +
> +		if (!gfn_to_memslot(kvm, gfn)) {
> +			printk(KERN_ERR "%s: no memslot for gfn %ld\n",
> +					 audit_msg, gfn);
> +			printk(KERN_ERR "%s: index %ld of sp (gfn=%lx)\n",
> +					audit_msg, sptep - rev_sp->spt,
> +					rev_sp->gfn);
> +			dump_stack();
> +			return;
> +		}
> +
> +		rmapp = gfn_to_rmap(kvm, rev_sp->gfns[sptep - rev_sp->spt], 0);
> +		if (!*rmapp) {
> +			printk(KERN_ERR "%s: no rmap for writable spte %llx\n",
> +					 audit_msg, *sptep);
> +			dump_stack();
> +		}
> +	}
>   

Semi-related: we should set up a new exit code to halt the VM so it can 
be inspected, otherwise all those printks and dump_stack()s will quickly 
overwhelm the logging facilities.

-- 
error compiling committee.c: too many arguments to function