[PATCH 0/2] Fix walk_addr() page fault error codes

[PATCH 0/2] Fix walk_addr() page fault error codes

[Avi Kivity]

This patchset fixes bugs in page fault error code generation.  With the patches
applied, the access.flat unit test now passes with no errors on shadow, npt,
and ept.

Avi Kivity (2):
  KVM: MMU: Only indicate a fetch fault in page fault error code if nx
    is enabled
  KVM: MMU: Keep going on permission error

 arch/x86/kvm/paging_tmpl.h |   52 +++++++++++++++++++++++++------------------
 1 files changed, 30 insertions(+), 22 deletions(-)

[PATCH 1/2] KVM: MMU: Only indicate a fetch fault in page fault error code if nx is enabled

[Avi Kivity]

Bit 4 of the page fault error code is set only if EFER.NX is set.

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

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index dfb2720..56c7f4f 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -245,7 +245,7 @@ err:
 		walker->error_code |= PFERR_WRITE_MASK;
 	if (user_fault)
 		walker->error_code |= PFERR_USER_MASK;
-	if (fetch_fault)
+	if (fetch_fault && is_nx(vcpu))
 		walker->error_code |= PFERR_FETCH_MASK;
 	if (rsvd_fault)
 		walker->error_code |= PFERR_RSVD_MASK;
-- 
1.7.1

[PATCH 2/2] KVM: MMU: Keep going on permission error

[Avi Kivity]

Real hardware disregards permission errors when computing page fault error
code bit 0 (page present).  Do the same.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 arch/x86/kvm/paging_tmpl.h |   50 +++++++++++++++++++++++++------------------
 1 files changed, 29 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 56c7f4f..1bbeffc 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -121,19 +121,23 @@ static int FNAME(walk_addr)(struct guest_walker *walker,
 	gfn_t table_gfn;
 	unsigned index, pt_access, pte_access;
 	gpa_t pte_gpa;
-	int rsvd_fault = 0;
+	bool eperm, present, rsvd_fault;
 
 	trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault,
 				     fetch_fault);
 walk:
+	present = true;
+	eperm = rsvd_fault = false;
 	walker->level = vcpu->arch.mmu.root_level;
 	pte = vcpu->arch.cr3;
 #if PTTYPE == 64
 	if (!is_long_mode(vcpu)) {
 		pte = kvm_pdptr_read(vcpu, (addr >> 30) & 3);
 		trace_kvm_mmu_paging_element(pte, walker->level);
-		if (!is_present_gpte(pte))
-			goto not_present;
+		if (!is_present_gpte(pte)) {
+			present = false;
+			goto error;
+		}
 		--walker->level;
 	}
 #endif
@@ -151,31 +155,36 @@ walk:
 		walker->table_gfn[walker->level - 1] = table_gfn;
 		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-		if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte)))
-			goto not_present;
+		if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte))) {
+			present = false;
+			break;
+		}
 
 		trace_kvm_mmu_paging_element(pte, walker->level);
 
-		if (!is_present_gpte(pte))
-			goto not_present;
+		if (!is_present_gpte(pte)) {
+			present = false;
+			break;
+		}
 
-		rsvd_fault = is_rsvd_bits_set(vcpu, pte, walker->level);
-		if (rsvd_fault)
-			goto access_error;
+		if (is_rsvd_bits_set(vcpu, pte, walker->level)) {
+			rsvd_fault = true;
+			break;
+		}
 
 		if (write_fault && !is_writable_pte(pte))
 			if (user_fault || is_write_protection(vcpu))
-				goto access_error;
+				eperm = true;
 
 		if (user_fault && !(pte & PT_USER_MASK))
-			goto access_error;
+			eperm = true;
 
 #if PTTYPE == 64
 		if (fetch_fault && (pte & PT64_NX_MASK))
-			goto access_error;
+			eperm = true;
 #endif
 
-		if (!(pte & PT_ACCESSED_MASK)) {
+		if (!eperm && !rsvd_fault && !(pte & PT_ACCESSED_MASK)) {
 			trace_kvm_mmu_set_accessed_bit(table_gfn, index,
 						       sizeof(pte));
 			if (FNAME(cmpxchg_gpte)(vcpu->kvm, table_gfn,
@@ -214,6 +223,9 @@ walk:
 		--walker->level;
 	}
 
+	if (!present || eperm || rsvd_fault)
+		goto error;
+
 	if (write_fault && !is_dirty_gpte(pte)) {
 		bool ret;
 
@@ -233,14 +245,10 @@ walk:
 		 __func__, (u64)pte, pte_access, pt_access);
 	return 1;
 
-not_present:
+error:
 	walker->error_code = 0;
-	goto err;
-
-access_error:
-	walker->error_code = PFERR_PRESENT_MASK;
-
-err:
+	if (present)
+		walker->error_code |= PFERR_PRESENT_MASK;
 	if (write_fault)
 		walker->error_code |= PFERR_WRITE_MASK;
 	if (user_fault)
-- 
1.7.1

Re: [PATCH 2/2] KVM: MMU: Keep going on permission error

[Jan Kiszka]

Avi Kivity wrote:
> Real hardware disregards permission errors when computing page fault error
> code bit 0 (page present).  Do the same.
> 

This generates (false positive) build warnings here:

  CC [M]  /data/kvm-kmod/x86/coalesced_mmio.o
/data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging32_walk_addr’:
/data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
uninitialized in this function
/data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging64_walk_addr’:
/data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
uninitialized in this function

> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  arch/x86/kvm/paging_tmpl.h |   50 +++++++++++++++++++++++++------------------
>  1 files changed, 29 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 56c7f4f..1bbeffc 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -121,19 +121,23 @@ static int FNAME(walk_addr)(struct guest_walker *walker,
>  	gfn_t table_gfn;
>  	unsigned index, pt_access, pte_access;
>  	gpa_t pte_gpa;
> -	int rsvd_fault = 0;
> +	bool eperm, present, rsvd_fault;
>  
>  	trace_kvm_mmu_pagetable_walk(addr, write_fault, user_fault,
>  				     fetch_fault);
>  walk:
> +	present = true;
> +	eperm = rsvd_fault = false;
>  	walker->level = vcpu->arch.mmu.root_level;
>  	pte = vcpu->arch.cr3;
>  #if PTTYPE == 64
>  	if (!is_long_mode(vcpu)) {
>  		pte = kvm_pdptr_read(vcpu, (addr >> 30) & 3);
>  		trace_kvm_mmu_paging_element(pte, walker->level);
> -		if (!is_present_gpte(pte))
> -			goto not_present;
> +		if (!is_present_gpte(pte)) {
> +			present = false;
> +			goto error;
> +		}
>  		--walker->level;
>  	}
>  #endif
> @@ -151,31 +155,36 @@ walk:
>  		walker->table_gfn[walker->level - 1] = table_gfn;
>  		walker->pte_gpa[walker->level - 1] = pte_gpa;
>  
> -		if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte)))
> -			goto not_present;
> +		if (kvm_read_guest(vcpu->kvm, pte_gpa, &pte, sizeof(pte))) {
> +			present = false;
> +			break;
> +		}
>  
>  		trace_kvm_mmu_paging_element(pte, walker->level);
>  
> -		if (!is_present_gpte(pte))
> -			goto not_present;
> +		if (!is_present_gpte(pte)) {
> +			present = false;
> +			break;

goto error?

> +		}
>  
> -		rsvd_fault = is_rsvd_bits_set(vcpu, pte, walker->level);
> -		if (rsvd_fault)
> -			goto access_error;
> +		if (is_rsvd_bits_set(vcpu, pte, walker->level)) {
> +			rsvd_fault = true;
> +			break;

goto error?

> +		}
>  
>  		if (write_fault && !is_writable_pte(pte))
>  			if (user_fault || is_write_protection(vcpu))
> -				goto access_error;
> +				eperm = true;
>  
>  		if (user_fault && !(pte & PT_USER_MASK))
> -			goto access_error;
> +			eperm = true;
>  
>  #if PTTYPE == 64
>  		if (fetch_fault && (pte & PT64_NX_MASK))
> -			goto access_error;
> +			eperm = true;
>  #endif
>  
> -		if (!(pte & PT_ACCESSED_MASK)) {
> +		if (!eperm && !rsvd_fault && !(pte & PT_ACCESSED_MASK)) {

We should never get here with rsvd_fault == true - redundant check?

>  			trace_kvm_mmu_set_accessed_bit(table_gfn, index,
>  						       sizeof(pte));
>  			if (FNAME(cmpxchg_gpte)(vcpu->kvm, table_gfn,
> @@ -214,6 +223,9 @@ walk:
>  		--walker->level;
>  	}
>  
> +	if (!present || eperm || rsvd_fault)
> +		goto error;
> +

We would only need to check for eperm here if we jumped above.

Jan

>  	if (write_fault && !is_dirty_gpte(pte)) {
>  		bool ret;
>  
> @@ -233,14 +245,10 @@ walk:
>  		 __func__, (u64)pte, pte_access, pt_access);
>  	return 1;
>  
> -not_present:
> +error:
>  	walker->error_code = 0;
> -	goto err;
> -
> -access_error:
> -	walker->error_code = PFERR_PRESENT_MASK;
> -
> -err:
> +	if (present)
> +		walker->error_code |= PFERR_PRESENT_MASK;
>  	if (write_fault)
>  		walker->error_code |= PFERR_WRITE_MASK;
>  	if (user_fault)

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] KVM: MMU: Keep going on permission error

[Avi Kivity]

On 07/07/2010 03:33 PM, Jan Kiszka wrote:
> Avi Kivity wrote:
>    
>> Real hardware disregards permission errors when computing page fault error
>> code bit 0 (page present).  Do the same.
>>
>>      
> This generates (false positive) build warnings here:
>
>    CC [M]  /data/kvm-kmod/x86/coalesced_mmio.o
> /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging32_walk_addr’:
> /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
> uninitialized in this function
> /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging64_walk_addr’:
> /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
> uninitialized in this function
>    

Yes, I saw.  I'll clear it up.


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

Re: [PATCH 2/2] KVM: MMU: Keep going on permission error

[Jan Kiszka]

Avi Kivity wrote:
> On 07/07/2010 03:33 PM, Jan Kiszka wrote:
>> Avi Kivity wrote:
>>    
>>> Real hardware disregards permission errors when computing page fault error
>>> code bit 0 (page present).  Do the same.
>>>
>>>      
>> This generates (false positive) build warnings here:
>>
>>    CC [M]  /data/kvm-kmod/x86/coalesced_mmio.o
>> /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging32_walk_addr’:
>> /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
>> uninitialized in this function
>> /data/kvm-kmod/x86/paging_tmpl.h: In function ‘paging64_walk_addr’:
>> /data/kvm-kmod/x86/paging_tmpl.h:122: warning: ‘pte_access’ may be used
>> uninitialized in this function
>>    
> 
> Yes, I saw.  I'll clear it up.
> 

You already pushed some update, don't you? Then I'm afraid I have to
report that the problem persists.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH 2/2] KVM: MMU: Keep going on permission error

[Avi Kivity]

On 07/08/2010 02:14 PM, Jan Kiszka wrote:
>
>> Yes, I saw.  I'll clear it up.
>>
>>      
> You already pushed some update, don't you? Then I'm afraid I have to
> report that the problem persists.
>    

Yes, I did.  I can't believe I didn't even compile test it.

Sorry, will fix for real now.

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

Re: [PATCH 0/2] Fix walk_addr() page fault error codes

[Marcelo Tosatti]

On Tue, Jul 06, 2010 at 04:49:34PM +0300, Avi Kivity wrote:
> This patchset fixes bugs in page fault error code generation.  With the patches
> applied, the access.flat unit test now passes with no errors on shadow, npt,
> and ept.
> 
> Avi Kivity (2):
>   KVM: MMU: Only indicate a fetch fault in page fault error code if nx
>     is enabled
>   KVM: MMU: Keep going on permission error
> 
>  arch/x86/kvm/paging_tmpl.h |   52 +++++++++++++++++++++++++------------------
>  1 files changed, 30 insertions(+), 22 deletions(-)

Looks good to me.

Re: [PATCH 0/2] Fix walk_addr() page fault error codes

[Xiao Guangrong]

Avi Kivity wrote:
> This patchset fixes bugs in page fault error code generation.  With the patches
> applied, the access.flat unit test now passes with no errors on shadow, npt,
> and ept.
> 
> Avi Kivity (2):
>   KVM: MMU: Only indicate a fetch fault in page fault error code if nx
>     is enabled
>   KVM: MMU: Keep going on permission error
> 

Reviewed-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>