Re: [PATCH] KVM: MMU: Segregate mmu pages created with different cr4.pge settings

From: Avi Kivity <avi@redhat.com>
To: Alexander Graf <agraf@suse.de>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	joerg.roedel@amd.com, Marcelo Tosatti <mtosatti@redhat.com>
Subject: Re: [PATCH] KVM: MMU: Segregate mmu pages created with different cr4.pge	settings
Date: Tue, 06 Jan 2009 12:41:40 +0200	[thread overview]
Message-ID: <49633564.7070403@redhat.com> (raw)
In-Reply-To: <49621FA9.5080903@suse.de>

Alexander Graf wrote:
> Avi Kivity wrote:
>   
>> From: Avi Kivity <avi@redhat.com>
>>
>> Don't allow a vcpu with cr4.pge cleared to use a shadow page created with
>> cr4.pge set; this might cause a cr3 switch not to sync ptes that have the
>> global bit set (the global bit has no effect if !cr4.pge).
>>
>> This can only occur on smp with different cr4.pge settings for different
>> vcpus (since a cr4 change will resync the shadow ptes), but there's no
>> cost to being correct here.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index f49bfd0..ab8ef1d 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -183,6 +183,7 @@ union kvm_mmu_page_role {
>>  		unsigned metaphysical:1;
>>  		unsigned access:3;
>>  		unsigned invalid:1;
>> +		unsigned cr4_pge:1;
>>  	};
>>  };
>>  
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index c4da7fb..aa4575c 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -363,6 +363,7 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>>  	}
>>  	kvm_x86_ops->set_cr4(vcpu, cr4);
>>  	vcpu->arch.cr4 = cr4;
>> +	vcpu->arch.mmu.base_role.cr4_pge = !!(cr4 & X86_CR4_PGE);
>>   
>>     
>
> This line broke VMware ESX bootup using NPT on one VCPU for me. If I
> comment it out and apply my patches to actually make ESX run, it boots
> again.
>   

I think this might be the problem:

> static int mmu_need_write_protect(struct kvm_vcpu *vcpu, gfn_t gfn,
>                   bool can_unsync)
> {
>     struct kvm_mmu_page *shadow;
>
>     shadow = kvm_mmu_lookup_page(vcpu->kvm, gfn);
>     if (shadow) {
>         if (shadow->role.level != PT_PAGE_TABLE_LEVEL)
>             return 1;
>         if (shadow->unsync)
>             return 0;
>         if (can_unsync && oos_shadow)
>             return kvm_unsync_page(vcpu, shadow);
>         return 1;
>     }
>     return 0;
> }
>

lookup_page() is not deterministic if there are multiple shadows for a 
page, and the patch increases multiple shadows.

Marcelo, shouldn't there be a for_each in there?

-- 
error compiling committee.c: too many arguments to function