From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [patch 1/4] KVM: MMU audit: update count_writable_mappings / count_rmaps Date: Mon, 08 Jun 2009 12:24:08 +0300 Message-ID: <4A2CD8B8.2050308@redhat.com> References: <20090602213655.640083007@localhost.localdomain> <20090602214226.820226306@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: sheng@linux.intel.com, kvm@vger.kernel.org To: Marcelo Tosatti Return-path: Received: from mx2.redhat.com ([66.187.237.31]:48069 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751993AbZFHJYJ (ORCPT ); Mon, 8 Jun 2009 05:24:09 -0400 In-Reply-To: <20090602214226.820226306@localhost.localdomain> Sender: kvm-owner@vger.kernel.org List-ID: 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 > > 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