[PATCH 0/2] KVM PDPTR reload fixes review

[PATCH 0/2] KVM PDPTR reload fixes review

[Avi Kivity]

The upcoming F11 release switches the default 32-bit kernel to PAE, and
also uses the fancy priority inherited futexes.  This combination exposed
bugs in KVM's handling of PDPTR reloads.

There's nothing complicated in there, but I'd like to submit it for
2.6.30-rc7 as it fixes an important guest.  So please review it carefully.

Avi Kivity (2):
  KVM: Make paravirt tlb flush also reload PAE PDPTEs
  KVM: Make CR4 reloads also reload CR3 if paging is enabled

 arch/x86/kvm/mmu.c |    3 +--
 arch/x86/kvm/x86.c |    6 +++++-
 2 files changed, 6 insertions(+), 3 deletions(-)

[PATCH 1/2] KVM: Make paravirt tlb flush also reload PAE PDPTEs

[Avi Kivity]

The paravirt tlb flush may be used not only to flush TLBs, but also
to reload the four page-directory-pointer-table entries, as it is used
as a replacement for reloading CR3.  Change the code to do the entire
CR3 reloading dance instead of simply flushing the TLB.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/mmu.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 6880a4c..7030b5f 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2927,8 +2927,7 @@ static int kvm_pv_mmu_write(struct kvm_vcpu *vcpu,
 
 static int kvm_pv_mmu_flush_tlb(struct kvm_vcpu *vcpu)
 {
-	kvm_x86_ops->tlb_flush(vcpu);
-	set_bit(KVM_REQ_MMU_SYNC, &vcpu->requests);
+	kvm_set_cr3(vcpu, vcpu->arch.cr3);
 	return 1;
 }
 
-- 
1.6.0.6

Re: [PATCH 1/2] KVM: Make paravirt tlb flush also reload PAE PDPTEs

[Marcelo Tosatti]

On Mon, May 25, 2009 at 11:47:23AM +0300, Avi Kivity wrote:
> The paravirt tlb flush may be used not only to flush TLBs, but also
> to reload the four page-directory-pointer-table entries, as it is used
> as a replacement for reloading CR3.  Change the code to do the entire
> CR3 reloading dance instead of simply flushing the TLB.

Ugh, my bad.

ACK.


> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/mmu.c |    3 +--
>  1 files changed, 1 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 6880a4c..7030b5f 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2927,8 +2927,7 @@ static int kvm_pv_mmu_write(struct kvm_vcpu *vcpu,
>  
>  static int kvm_pv_mmu_flush_tlb(struct kvm_vcpu *vcpu)
>  {
> -	kvm_x86_ops->tlb_flush(vcpu);
> -	set_bit(KVM_REQ_MMU_SYNC, &vcpu->requests);
> +	kvm_set_cr3(vcpu, vcpu->arch.cr3);
>  	return 1;
>  }
>  
> -- 
> 1.6.0.6

Re: [PATCH 1/2] KVM: Make paravirt tlb flush also reload PAE PDPTEs

[Avi Kivity]

Marcelo Tosatti wrote:
> On Mon, May 25, 2009 at 11:47:23AM +0300, Avi Kivity wrote:
>   
>> The paravirt tlb flush may be used not only to flush TLBs, but also
>> to reload the four page-directory-pointer-table entries, as it is used
>> as a replacement for reloading CR3.  Change the code to do the entire
>> CR3 reloading dance instead of simply flushing the TLB.
>>     
>
> Ugh, my bad.
>   

Not really -- the pvop is misleadingly named.

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

[PATCH 2/2] KVM: Make CR4 reloads also reload CR3 if paging is enabled

[Avi Kivity]

The processor is documented to reload the PDPTRs while in PAE mode if any
of the CR4 bits PSE, PGE, or PAE change.  Linux relies on this
behaviour when zapping the low mappings of PAE kernels during boot.

The code already handled changes to CR4.PAE; augment it to also notice changes
to PSE and PGE.

This triggered while booting an F11 PAE kernel; the futex initialization code
runs before any CR3 reloads and writes to a NULL pointer; the futex subsystem
ended up uninitialized, killing PI futexes and pulseaudio which uses them.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/x86.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 8b5d1e5..6d44dd5 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -341,6 +341,9 @@ EXPORT_SYMBOL_GPL(kvm_lmsw);
 
 void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 {
+	unsigned long old_cr4 = vcpu->arch.cr4;
+	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
+
 	if (cr4 & CR4_RESERVED_BITS) {
 		printk(KERN_DEBUG "set_cr4: #GP, reserved bits\n");
 		kvm_inject_gp(vcpu, 0);
@@ -354,7 +357,8 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
 			kvm_inject_gp(vcpu, 0);
 			return;
 		}
-	} else if (is_paging(vcpu) && !is_pae(vcpu) && (cr4 & X86_CR4_PAE)
+	} else if (is_paging(vcpu) && (cr4 & X86_CR4_PAE)
+		   && ((cr4 ^ old_cr4) & pdptr_bits)
 		   && !load_pdptrs(vcpu, vcpu->arch.cr3)) {
 		printk(KERN_DEBUG "set_cr4: #GP, pdptrs reserved bits\n");
 		kvm_inject_gp(vcpu, 0);
-- 
1.6.0.6

Re: [PATCH 2/2] KVM: Make CR4 reloads also reload CR3 if paging is enabled

[Marcelo Tosatti]

On Mon, May 25, 2009 at 11:47:24AM +0300, Avi Kivity wrote:
> The processor is documented to reload the PDPTRs while in PAE mode if any
> of the CR4 bits PSE, PGE, or PAE change.  Linux relies on this
> behaviour when zapping the low mappings of PAE kernels during boot.
> 
> The code already handled changes to CR4.PAE; augment it to also notice changes
> to PSE and PGE.
> 
> This triggered while booting an F11 PAE kernel; the futex initialization code
> runs before any CR3 reloads and writes to a NULL pointer; the futex subsystem
> ended up uninitialized, killing PI futexes and pulseaudio which uses them.

One comment regarding set_cr0. Section 8.1 of the TLB doc says:

* The processor does not maintain a PDP cache as described in
Section 4. The processor always caches information from the four
page-directory-pointer-table entries. These entries are not cached at
the time of address translation. Instead, they are always cached as part
of the execution of the following instructions:

* A MOV to CR0 that modifies CR0.PG and that occurs with IA32_EFER.LMA = 0
and CR4.PAE = 1.

However kvm_set_cr0 only caches the PDPTRs if CR0.PG changed from 0->1.
Can't see a problem there though.

Also, the checks in kvm_arch_vcpu_ioctl_set_sregs should probably be unified
with kvm_set_crX, to avoid future mistakes.

Otherwise, ACK.

> 
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/x86.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 8b5d1e5..6d44dd5 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -341,6 +341,9 @@ EXPORT_SYMBOL_GPL(kvm_lmsw);
>  
>  void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>  {
> +	unsigned long old_cr4 = vcpu->arch.cr4;
> +	unsigned long pdptr_bits = X86_CR4_PGE | X86_CR4_PSE | X86_CR4_PAE;
> +
>  	if (cr4 & CR4_RESERVED_BITS) {
>  		printk(KERN_DEBUG "set_cr4: #GP, reserved bits\n");
>  		kvm_inject_gp(vcpu, 0);
> @@ -354,7 +357,8 @@ void kvm_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>  			kvm_inject_gp(vcpu, 0);
>  			return;
>  		}
> -	} else if (is_paging(vcpu) && !is_pae(vcpu) && (cr4 & X86_CR4_PAE)
> +	} else if (is_paging(vcpu) && (cr4 & X86_CR4_PAE)
> +		   && ((cr4 ^ old_cr4) & pdptr_bits)
>  		   && !load_pdptrs(vcpu, vcpu->arch.cr3)) {
>  		printk(KERN_DEBUG "set_cr4: #GP, pdptrs reserved bits\n");
>  		kvm_inject_gp(vcpu, 0);
> -- 
> 1.6.0.6

Re: [PATCH 2/2] KVM: Make CR4 reloads also reload CR3 if paging is enabled

[Avi Kivity]

Marcelo Tosatti wrote:
> On Mon, May 25, 2009 at 11:47:24AM +0300, Avi Kivity wrote:
>   
>> The processor is documented to reload the PDPTRs while in PAE mode if any
>> of the CR4 bits PSE, PGE, or PAE change.  Linux relies on this
>> behaviour when zapping the low mappings of PAE kernels during boot.
>>
>> The code already handled changes to CR4.PAE; augment it to also notice changes
>> to PSE and PGE.
>>
>> This triggered while booting an F11 PAE kernel; the futex initialization code
>> runs before any CR3 reloads and writes to a NULL pointer; the futex subsystem
>> ended up uninitialized, killing PI futexes and pulseaudio which uses them.
>>     
>
> One comment regarding set_cr0. Section 8.1 of the TLB doc says:
>
> * The processor does not maintain a PDP cache as described in
> Section 4. The processor always caches information from the four
> page-directory-pointer-table entries. These entries are not cached at
> the time of address translation. Instead, they are always cached as part
> of the execution of the following instructions:
>
> * A MOV to CR0 that modifies CR0.PG and that occurs with IA32_EFER.LMA = 0
> and CR4.PAE = 1.
>
> However kvm_set_cr0 only caches the PDPTRs if CR0.PG changed from 0->1.
> Can't see a problem there though.
>   

Yes, if cr0.pg == 0, then the pdptrs are meaningless.

> Also, the checks in kvm_arch_vcpu_ioctl_set_sregs should probably be unified
> with kvm_set_crX, to avoid future mistakes.
>   

Yes please.  But it needs to be done very carefully, since the order of 
the checks matters here.

> Otherwise, ACK.
>   

Thanks for the review.

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