Re: [patch 09/10] KVM: MMU: out of sync shadow core v2

[Avi Kivity <avi@redhat.com>]

Marcelo Tosatti wrote:
>>> +	while (parent->unsync_children) {
>>> +		for (i = 0; i < PT64_ENT_PER_PAGE; ++i) {
>>> +			u64 ent = sp->spt[i];
>>> +
>>> +			if (is_shadow_present_pte(ent)) {
>>> +				struct kvm_mmu_page *child;
>>> +				child = page_header(ent & PT64_BASE_ADDR_MASK);
>>>       
>> What does this do?
>>     
>
> Walks all children of given page with no efficiency. Its replaced later
> by the bitmap version.
>
>   

I don't understand how the variables sp, child, and parent interact. You 
either need recursion or an explicit stack?

>>> +static int kvm_sync_page(struct kvm_vcpu *vcpu, struct kvm_mmu_page *sp)
>>> +{
>>> +	if (sp->role.glevels != vcpu->arch.mmu.root_level) {
>>> +		kvm_mmu_zap_page(vcpu->kvm, sp);
>>> +		return 1;
>>> +	}
>>>   
>>>       
>> Suppose we switch to real mode, touch a pte, switch back.  Is this handled?
>>     
>
> The shadow page will go unsync on pte touch and resynced as soon as its
> visible (after return to paging).
>
> Or, while still in real mode, it might be zapped by
> kvm_mmu_get_page->kvm_sync_page.
>
> Am I missing something?
>
>   

I guess I was. Ok.

>
>>>  static int kvm_mmu_zap_page(struct kvm *kvm, struct kvm_mmu_page *sp)
>>> -	return 0;
>>> +	return ret;
>>>  }
>>>   
>>>       
>> Why does the caller care if zap also zapped some other random pages?  To  
>> restart walking the list?
>>     
>
> Yes. The next element for_each_entry_safe saved could have been zapped.
>
>   

Ouch. Ouch.

I hate doing this. Can see no alternative though.

>>> +	/* don't unsync if pagetable is shadowed with multiple roles */
>>> +	hlist_for_each_entry_safe(s, node, n, bucket, hash_link) {
>>> +		if (s->gfn != sp->gfn || s->role.metaphysical)
>>> +			continue;
>>> +		if (s->role.word != sp->role.word)
>>> +			return 1;
>>> +	}
>>>   
>>>       
>> This will happen for nonpae paging.  But why not allow it?  Zap all  
>> unsynced pages on mode switch.
>>
>> Oh, if a page is both a page directory and page table, yes.  
>>     
>
> Yes. 
>
>   
>> So to allow nonpae oos, check the level instead.
>>     
>
> Windows 2008 64-bit has all sorts of sharing a pagetable at multiple
> levels too.
>
>   

We still want to allow oos for the two quadrants of a nonpae shadow page.

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