| Message ID | 20090609213312.838419569@amt.cnet (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Marcelo Tosatti wrote: > Required by EPT misconfiguration handler. > > 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 > @@ -3013,6 +3013,26 @@ out: > return r; > } > > +void kvm_mmu_shadow_walk(struct kvm_vcpu *vcpu, u64 addr, > + struct mmu_shadow_walk *walk) > +{ > + struct kvm_shadow_walk_iterator iterator; > + > + spin_lock(&vcpu->kvm->mmu_lock); > + for_each_shadow_entry(vcpu, addr, iterator) { > + int err; > + > + err = walk->fn(vcpu, iterator.sptep, iterator.level, walk); > + if (err) > + break; > + > + if (!is_shadow_present_pte(*iterator.sptep)) > + break; > + } > + spin_unlock(&vcpu->kvm->mmu_lock); > +} > +EXPORT_SYMBOL(kvm_mmu_shadow_walk); > + > Isn't it simpler to invoke for_each_shadow_entry(), instead of defining a callback and calling it? We had those callbacks once, then switched to for_each.
On Wed, Jun 10, 2009 at 12:17:09PM +0300, Avi Kivity wrote: > Marcelo Tosatti wrote: >> Required by EPT misconfiguration handler. >> >> 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 >> @@ -3013,6 +3013,26 @@ out: >> return r; >> } >> +void kvm_mmu_shadow_walk(struct kvm_vcpu *vcpu, u64 addr, >> + struct mmu_shadow_walk *walk) >> +{ >> + struct kvm_shadow_walk_iterator iterator; >> + >> + spin_lock(&vcpu->kvm->mmu_lock); >> + for_each_shadow_entry(vcpu, addr, iterator) { >> + int err; >> + >> + err = walk->fn(vcpu, iterator.sptep, iterator.level, walk); >> + if (err) >> + break; >> + >> + if (!is_shadow_present_pte(*iterator.sptep)) >> + break; >> + } >> + spin_unlock(&vcpu->kvm->mmu_lock); >> +} >> +EXPORT_SYMBOL(kvm_mmu_shadow_walk); >> + >> > > Isn't it simpler to invoke for_each_shadow_entry(), instead of defining > a callback and calling it? > > We had those callbacks once, then switched to for_each. The point is its exported to use in a external module (kvm-intel.ko), so you hide the details (such as locking) in the kvm_mmu_shadow_walk helper. Let me know how do you prefer this to be. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti wrote: >> Isn't it simpler to invoke for_each_shadow_entry(), instead of defining >> a callback and calling it? >> >> We had those callbacks once, then switched to for_each. >> > > The point is its exported to use in a external module (kvm-intel.ko), > so you hide the details (such as locking) in the kvm_mmu_shadow_walk > helper. Let me know how do you prefer this to be. > Ah, you're right. I don't think it's worthwhile to add all this just for debugging. You can add a function that dumps the spte chain as well as the features MSR, and we can decode it by hand when we see it. Disadvantage is more work for us when we hit the bug, but at least that function is reusable in other contexts.
On Wed, Jun 10, 2009 at 03:23:47PM +0300, Avi Kivity wrote: > Marcelo Tosatti wrote: >>> Isn't it simpler to invoke for_each_shadow_entry(), instead of >>> defining a callback and calling it? >>> >>> We had those callbacks once, then switched to for_each. >>> >> >> The point is its exported to use in a external module (kvm-intel.ko), >> so you hide the details (such as locking) in the kvm_mmu_shadow_walk >> helper. Let me know how do you prefer this to be. >> > > Ah, you're right. > > I don't think it's worthwhile to add all this just for debugging. You > can add a function that dumps the spte chain as well as the features > MSR, and we can decode it by hand when we see it. Disadvantage is more > work for us when we hit the bug, but at least that function is reusable > in other contexts. The problem is if someone hits the bug who has no possibility of reporting the printks. Nice thing about the WARN_ON's there is you can look up kerneloops.org, match the line number to the kernel version, and narrow down what bits are wrong (you still need reporter to send contents of dmesg for full spte value). Also the bit-by-bit validity checks (the inspect_spte function in vmx.c) can be used in the mmu audit code (thats the reason for print=0 in the callback parameters), so it is reusable in other contexes. What you dislike is hardcoding the bits checked in C code? Don't worry about the level stuff, will be handled next. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Marcelo Tosatti wrote: >> Ah, you're right. >> >> I don't think it's worthwhile to add all this just for debugging. You >> can add a function that dumps the spte chain as well as the features >> MSR, and we can decode it by hand when we see it. Disadvantage is more >> work for us when we hit the bug, but at least that function is reusable >> in other contexts. >> > > The problem is if someone hits the bug who has no possibility of > reporting the printks. Nice thing about the WARN_ON's there is you can > look up kerneloops.org, match the line number to the kernel version, > and narrow down what bits are wrong (you still need reporter to send > contents of dmesg for full spte value). > Well, we can KERN_WARNING instead of KERN_DEBUG in that printout. > Also the bit-by-bit validity checks (the inspect_spte function in vmx.c) > can be used in the mmu audit code (thats the reason for print=0 in the > callback parameters), so it is reusable in other contexes. > > What you dislike is hardcoding the bits checked in C code? Don't worry > about the level stuff, will be handled next. > I just don't want to introduce yet another function-callback pair for such a small thing. No objection to the code itself. Printing out the spte hierarchy seems a good idea in other bug contexts, so at least that function is reusable.
On 06/10/2009 06:24 PM, Avi Kivity wrote: > > I just don't want to introduce yet another function-callback pair for > such a small thing. No objection to the code itself. > > Printing out the spte hierarchy seems a good idea in other bug > contexts, so at least that function is reusable. > Here's one that preserves all information but is simple to use: kvm_mmu_get_spte_hierarchy(vcpu, gva, int *nr_sptes, u64 sptes[4]) So you can later do detailed checks on the sptes, but it doesn't expose yet another generic walker.
Addressing comments.
Index: kvm/arch/x86/kvm/mmu.c =================================================================== --- kvm.orig/arch/x86/kvm/mmu.c +++ kvm/arch/x86/kvm/mmu.c @@ -3013,6 +3013,26 @@ out: return r; } +void kvm_mmu_shadow_walk(struct kvm_vcpu *vcpu, u64 addr, + struct mmu_shadow_walk *walk) +{ + struct kvm_shadow_walk_iterator iterator; + + spin_lock(&vcpu->kvm->mmu_lock); + for_each_shadow_entry(vcpu, addr, iterator) { + int err; + + err = walk->fn(vcpu, iterator.sptep, iterator.level, walk); + if (err) + break; + + if (!is_shadow_present_pte(*iterator.sptep)) + break; + } + spin_unlock(&vcpu->kvm->mmu_lock); +} +EXPORT_SYMBOL(kvm_mmu_shadow_walk); + #ifdef AUDIT static const char *audit_msg; Index: kvm/arch/x86/kvm/mmu.h =================================================================== --- kvm.orig/arch/x86/kvm/mmu.h +++ kvm/arch/x86/kvm/mmu.h @@ -37,6 +37,14 @@ #define PT32_ROOT_LEVEL 2 #define PT32E_ROOT_LEVEL 3 +struct mmu_shadow_walk { + int (*fn) (struct kvm_vcpu *vcpu, u64 *sptep, int level, + struct mmu_shadow_walk *walk); +}; + +void kvm_mmu_shadow_walk(struct kvm_vcpu *vcpu, u64 addr, + struct mmu_shadow_walk *walk); + static inline void kvm_mmu_free_some_pages(struct kvm_vcpu *vcpu) { if (unlikely(vcpu->kvm->arch.n_free_mmu_pages < KVM_MIN_FREE_MMU_PAGES))
Required by EPT misconfiguration handler. Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com> -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html