Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Marcelo Tosatti]

On Fri, Sep 19, 2008 at 05:21:09PM -0700, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>> Since the sync page path can collapse flushes.
>>
>> Also only flush if the spte was writable before.
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> @@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
>>  		}
>>  	}
>>  	if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
>> -		      dirty, largepage, gfn, pfn, speculative))
>> +		      dirty, largepage, gfn, pfn, speculative)) {
>>  		if (write_fault)
>>  			*ptwrite = 1;
>> +		if (was_writeble)
>> +			kvm_x86_ops->tlb_flush(vcpu);
>> +	}
>>    
>
> I think we had cases where the spte.pfn contents changed, for example  
> when a large page was replaced by a normal page,

True. And the TLB is not flushed now for large->normal replace, in case
the pte thats faulting is read-only. The local (and remote) TLB's must
be flushed on large->normal replace.

(BTW the largepage patch is wrong, will reply to that soon).

> and also:
>
>        } else if (pfn != spte_to_pfn(*shadow_pte)) {

That one is likely to crash the guest anyway, so I don't see the need
for a flush there:

> > Did you find out what's causing the errors in the first place (if
> > zap is not used)?  It worries me greatly.
> Yes, the problem is that the rmap code does not handle the qemu
> process
> mappings from vanishing while there is a present rmap. If that
> happens,
> and there is a fault for a gfn whose qemu mapping has been removed, a
> different physical zero page will be allocated:
> 
>      rmap a -> gfn 0 -> physical host page 0
>      mapping for gfn 0 gets removed
>      guest faults in gfn 0 through the same pte "chain"
>      rmap a -> gfn 0 -> physical host page 1
> 
> When instantiating the shadow mapping for the second time, the
> "is_rmap_pte" check succeeds, so we release the reference grabbed by
> gfn_to_page() at mmu_set_spte(). We now have a shadow mapping
> pointing
> to a physical page without having an additional reference on that
> page.
> 
> The following makes the host not crash under such a condition, but
> the condition itself is invalid leading to inconsistent state on the
> guest.
> So IMHO it shouldnt be allowed to happen in the first place.

Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Avi Kivity]

Marcelo Tosatti wrote:

   

>> I think we had cases where the spte.pfn contents changed, for example  
>> when a large page was replaced by a normal page,
>>     
>
> True. And the TLB is not flushed now for large->normal replace, in case
> the pte thats faulting is read-only. The local (and remote) TLB's must
> be flushed on large->normal replace.
>
>   

Can you prepare a patch for that, for -stable?

>> and also:
>>
>>        } else if (pfn != spte_to_pfn(*shadow_pte)) {
>>     
>
> That one is likely to crash the guest anyway, so I don't see the need
> for a flush there:
>
>   
>>> Did you find out what's causing the errors in the first place (if
>>> zap is not used)?  It worries me greatly.
>>>       
>> Yes, the problem is that the rmap code does not handle the qemu
>> process
>> mappings from vanishing while there is a present rmap. If that
>> happens,
>> and there is a fault for a gfn whose qemu mapping has been removed, a
>> different physical zero page will be allocated:
>>
>>      rmap a -> gfn 0 -> physical host page 0
>>      mapping for gfn 0 gets removed
>>      guest faults in gfn 0 through the same pte "chain"
>>      rmap a -> gfn 0 -> physical host page 1
>>
>> When instantiating the shadow mapping for the second time, the
>> "is_rmap_pte" check succeeds, so we release the reference grabbed by
>> gfn_to_page() at mmu_set_spte(). We now have a shadow mapping
>> pointing
>> to a physical page without having an additional reference on that
>> page.
>>
>> The following makes the host not crash under such a condition, but
>> the condition itself is invalid leading to inconsistent state on the
>> guest.
>> So IMHO it shouldnt be allowed to happen in the first place.
>>     

And it isn't, with mmu notifiers.

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

Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Marcelo Tosatti]

On Mon, Sep 22, 2008 at 11:26:36PM +0300, Avi Kivity wrote:
> Marcelo Tosatti wrote:
>
>   
>
>>> I think we had cases where the spte.pfn contents changed, for example 
>>>  when a large page was replaced by a normal page,
>>>     
>>
>> True. And the TLB is not flushed now for large->normal replace, in case
>> the pte thats faulting is read-only. The local (and remote) TLB's must
>> be flushed on large->normal replace.
>>
>>   
>
> Can you prepare a patch for that, for -stable?

First one in the v3 series.

Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Avi Kivity]

Marcelo Tosatti wrote:

   

>> I think we had cases where the spte.pfn contents changed, for example  
>> when a large page was replaced by a normal page,
>>     
>
> True. And the TLB is not flushed now for large->normal replace, in case
> the pte thats faulting is read-only. The local (and remote) TLB's must
> be flushed on large->normal replace.
>
>   

Can you prepare a patch for that, for -stable?

>> and also:
>>
>>        } else if (pfn != spte_to_pfn(*shadow_pte)) {
>>     
>
> That one is likely to crash the guest anyway, so I don't see the need
> for a flush there:
>
>   
>>> Did you find out what's causing the errors in the first place (if
>>> zap is not used)?  It worries me greatly.
>>>       
>> Yes, the problem is that the rmap code does not handle the qemu
>> process
>> mappings from vanishing while there is a present rmap. If that
>> happens,
>> and there is a fault for a gfn whose qemu mapping has been removed, a
>> different physical zero page will be allocated:
>>
>>      rmap a -> gfn 0 -> physical host page 0
>>      mapping for gfn 0 gets removed
>>      guest faults in gfn 0 through the same pte "chain"
>>      rmap a -> gfn 0 -> physical host page 1
>>
>> When instantiating the shadow mapping for the second time, the
>> "is_rmap_pte" check succeeds, so we release the reference grabbed by
>> gfn_to_page() at mmu_set_spte(). We now have a shadow mapping
>> pointing
>> to a physical page without having an additional reference on that
>> page.
>>
>> The following makes the host not crash under such a condition, but
>> the condition itself is invalid leading to inconsistent state on the
>> guest.
>> So IMHO it shouldnt be allowed to happen in the first place.
>>     

And it isn't, with mmu notifiers.

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

[patch 00/10] out of sync shadow v2

[Marcelo Tosatti]

Addressing earlier comments.

-- 

[patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Marcelo Tosatti]

[-- Attachment #1: mmu-set-spte-tlb-flush --]
[-- Type: text/plain, Size: 1069 bytes --]

Since the sync page path can collapse flushes.

Also only flush if the spte was writable before.

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
@@ -1189,10 +1189,8 @@ static int set_spte(struct kvm_vcpu *vcp
 				 __func__, gfn);
 			ret = 1;
 			pte_access &= ~ACC_WRITE_MASK;
-			if (is_writeble_pte(spte)) {
+			if (is_writeble_pte(spte))
 				spte &= ~PT_WRITABLE_MASK;
-				kvm_x86_ops->tlb_flush(vcpu);
-			}
 		}
 	}
 
@@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
 		}
 	}
 	if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
-		      dirty, largepage, gfn, pfn, speculative))
+		      dirty, largepage, gfn, pfn, speculative)) {
 		if (write_fault)
 			*ptwrite = 1;
+		if (was_writeble)
+			kvm_x86_ops->tlb_flush(vcpu);
+	}
 
 	pgprintk("%s: setting spte %llx\n", __func__, *shadow_pte);
 	pgprintk("instantiating %s PTE (%s) at %ld (%llx) addr %p\n",

-- 

Re: [patch 02/10] KVM: MMU: move local TLB flush to mmu_set_spte

[Avi Kivity]

Marcelo Tosatti wrote:
> Since the sync page path can collapse flushes.
>
> Also only flush if the spte was writable before.
>
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>
> @@ -1241,9 +1239,12 @@ static void mmu_set_spte(struct kvm_vcpu
>  		}
>  	}
>  	if (set_spte(vcpu, shadow_pte, pte_access, user_fault, write_fault,
> -		      dirty, largepage, gfn, pfn, speculative))
> +		      dirty, largepage, gfn, pfn, speculative)) {
>  		if (write_fault)
>  			*ptwrite = 1;
> +		if (was_writeble)
> +			kvm_x86_ops->tlb_flush(vcpu);
> +	}
>  
>   

I think we had cases where the spte.pfn contents changed, for example 
when a large page was replaced by a normal page, and also:

        } else if (pfn != spte_to_pfn(*shadow_pte)) {


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