Re: KVM: MMU: always invalidate and flush on spte page size change

[Avi Kivity <avi@redhat.com>]

On 05/30/2010 06:19 PM, Marcelo Tosatti wrote:
> On Sun, May 30, 2010 at 01:28:19PM +0300, Avi Kivity wrote:
>    
>> On 05/28/2010 03:44 PM, Marcelo Tosatti wrote:
>>      
>>> Always invalidate spte and flush TLBs when changing page size, to make
>>> sure different sized translations for the same address are never cached
>>> in a CPU's TLB.
>>>
>>> The first case where this occurs is when a non-leaf spte pointer is
>>> overwritten by a leaf, large spte entry. This can happen after dirty
>>> logging is disabled on a memslot, for example.
>>>
>>> The second case is a leaf, large spte entry is overwritten with a
>>> non-leaf spte pointer, in __direct_map. Note this cannot happen now
>>> because the only potential source of such overwrite is dirty logging
>>> being enabled, which zaps all MMU pages. But this might change
>>> in the future, so better be robust against it.
>>>
>>> Noticed by Andrea.
>>>
>>> KVM-Stable-Tag
>>> 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
>>> @@ -1952,6 +1952,8 @@ static void mmu_set_spte(struct kvm_vcpu
>>>
>>>   			child = page_header(pte&   PT64_BASE_ADDR_MASK);
>>>   			mmu_page_remove_parent_pte(child, sptep);
>>> +			__set_spte(sptep, shadow_trap_nonpresent_pte);
>>> +			kvm_flush_remote_tlbs(vcpu->kvm);
>>>   		} else if (pfn != spte_to_pfn(*sptep)) {
>>>   			pgprintk("hfn old %lx new %lx\n",
>>>   				 spte_to_pfn(*sptep), pfn);
>>>        
>> Applied this bit.
>>
>>      
>>> @@ -2015,6 +2017,16 @@ static int __direct_map(struct kvm_vcpu
>>>   			break;
>>>   		}
>>>
>>> +		if (is_shadow_present_pte(*iterator.sptep)&&
>>> +		    !is_large_pte(*iterator.sptep))
>>> +			continue;
>>> +
>>> +		if (is_large_pte(*iterator.sptep)) {
>>> +			rmap_remove(vcpu->kvm, iterator.sptep);
>>> +			__set_spte(iterator.sptep, shadow_trap_nonpresent_pte);
>>> +			kvm_flush_remote_tlbs(vcpu->kvm);
>>> +		}
>>> +
>>>        
>> Don't we have exactly the same issue in FNAME(fetch)()?
>>      
> Yes and its already handled there:
>
>                  if (is_shadow_present_pte(*sptep)&&  !is_large_pte(*sptep))
>                          continue;
>
>                  if (is_large_pte(*sptep)) {
>                          rmap_remove(vcpu->kvm, sptep);
>                          __set_spte(sptep, shadow_trap_nonpresent_pte);
>                          kvm_flush_remote_tlbs(vcpu->kvm);
>                  }
>
>    

Well that bit of code wants deduplication under a good name.


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