Re: [patch 3/5] KVM: MMU: add kvm_mmu_shadow_walk helper

[Marcelo Tosatti <mtosatti@redhat.com>]

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.