Re: [PATCH] KVM: MMU: Replace role.glevels with role.cr4_pae

[Avi Kivity <avi@redhat.com>]

On 04/14/2010 09:29 PM, Marcelo Tosatti wrote:
> On Wed, Apr 14, 2010 at 07:32:12PM +0300, Avi Kivity wrote:
>    
>> On 04/14/2010 07:20 PM, Avi Kivity wrote:
>>      
>>> There is no real distinction between glevels=3 and glevels=4; both have
>>> exactly the same format and the code is treated exactly the same way.  Drop
>>> role.glevels and replace is with role.cr4_pae (which is meaningful).  This
>>> simplifies the code a bit.
>>>
>>> As a side effect, it allows sharing shadow page tables between pae and
>>> longmode guest page tables at the same guest page.
>>>        
>>      
>>>   static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
>>>   {
>>> -	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
>>> +	if (sp->role.cr4_pae != !!is_pae(vcpu)) {
>>>   		kvm_mmu_zap_page(vcpu->kvm, sp);
>>>   		return 1;
>>>   	}
>>>        
>> This bit confuses me a little.  Why is it needed?  It will never hit
>> from mmu_sync_children(), and as for kvm_mmu_get_page(), it will
>> simply zap unrelated pages?
>>      
> kvm_mmu_get_page is write protecting a gfn.

Took me a while to figure out why.

> If there's shadow for a
> differ	ent role, and its unsync, it needs to be synchronized.
>
>    

We could leave it unsync and write protected, though that destroys an 
invariant (sync==protected, unsync==unprotected), and all the calls to 
rmap_write_protect() become confused.

> Perhaps it could call the appropriate _sync_page version instead
> of zapping, similar to mmu_pte_write_new_pte.
>    

Probably better for nonpae.

>> Is it related to the restriction that we can only unsync if we have
>> just one shadow page for a gfn?  That's somewhat artificial (and
>> hurts nonpae guests, and guests with linear page tables).
>>      
> If gfn is shadowed at PMD or higher level, you can't unsync the PTE
> shadow.
>    

Yes.  Even if we could, invlpg is defined to drop all PDE caches (except 
large page PDEs), so we would have to resync all those pages on invlpg.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.