Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1364664 > unrolled thread

[PATCH 1/4] KVM: MMU: fix permission_fault()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/4] KVM: MMU: fix permission_fault() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 14:30 +0100
    [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 14:30 +0100
      Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 14:50 +0100
        Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 15:00 +0100
          Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 15:10 +0100
            Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 15:30 +0100
        Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 15:00 +0100
    Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 14:40 +0100
      Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 14:50 +0100
        Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 15:00 +0100
    [PATCH 2/4] KVM: MMU: simplify the logic of __mmu_unsync_walk() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-25 14:40 +0100
    Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Paolo Bonzini <pbonzini@redhat.com> - 2016-03-25 15:30 +0100
      Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-29 19:50 +0200
        Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Paolo Bonzini <pbonzini@redhat.com> - 2016-03-29 22:20 +0200
          Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-30 04:00 +0200
            Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Paolo Bonzini <pbonzini@redhat.com> - 2016-03-30 08:40 +0200
              Re: [PATCH 1/4] KVM: MMU: fix permission_fault() Xiao Guangrong <guangrong.xiao@linux.intel.com> - 2016-03-30 08:50 +0200

#1364664 — [PATCH 1/4] KVM: MMU: fix permission_fault()

kvm-unit-tests complained about the PFEC is not set properly, e.g,:
test pte.rw pte.d pte.nx pde.p pde.rw pde.pse user fetch: FAIL: error code 15 
expected 5
Dump mapping: address: 0x123400000000
------L4: 3e95007
------L3: 3e96007
------L2: 2000083

It's caused by the reason that PFEC returned to guest is copied from the
PFEC triggered by shadow page table

This patch fixes it and makes the logic of updating errcode more clean

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 arch/x86/kvm/mmu.h         | 8 ++++----
 arch/x86/kvm/paging_tmpl.h | 2 +-
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index b70df72..81bffd1 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -154,7 +154,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 				  unsigned pfec)
 {
 	int cpl = kvm_x86_ops->get_cpl(vcpu);
-	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
+	unsigned long errcode, rflags = kvm_x86_ops->get_rflags(vcpu);
 
 	/*
 	 * If CPL < 3, SMAP prevention are disabled if EFLAGS.AC = 1.
@@ -175,7 +175,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	bool fault = (mmu->permissions[index] >> pte_access) & 1;
 
 	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
-	pfec |= PFERR_PRESENT_MASK;
+	errcode = PFERR_PRESENT_MASK;
 
 	if (unlikely(mmu->pkru_mask)) {
 		u32 pkru_bits, offset;
@@ -193,11 +193,11 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
 
 		pkru_bits &= mmu->pkru_mask >> offset;
-		pfec |= -pkru_bits & PFERR_PK_MASK;
+		errcode |= -pkru_bits & PFERR_PK_MASK;
 		fault |= (pkru_bits != 0);
 	}
 
-	return -(uint32_t)fault & pfec;
+	return -(uint32_t)fault & errcode;
 }
 
 void kvm_mmu_invalidate_zap_all_pages(struct kvm *kvm);
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 1d971c7..bc019f7 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -360,7 +360,7 @@ retry_walk:
 			goto error;
 
 		if (unlikely(is_rsvd_bits_set(mmu, pte, walker->level))) {
-			errcode |= PFERR_RSVD_MASK | PFERR_PRESENT_MASK;
+			errcode = PFERR_RSVD_MASK | PFERR_PRESENT_MASK;
 			goto error;
 		}
 
-- 
1.8.3.1

[toc] | [next] | [standalone]

#1364666 — [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

Currently only PT64_ROOT_LEVEL - 1 levels are used, one additional entry
in .parent[] is used as a sentinel, the additional entry in .idx[] is
purely wasted

This patch reduces its size and sets the sentinel on the upper level of
the place where we start from

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 arch/x86/kvm/mmu.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index e273144..c396e8b 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1984,12 +1984,12 @@ static bool kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn,
 }
 
 struct mmu_page_path {
-	struct kvm_mmu_page *parent[PT64_ROOT_LEVEL];
-	unsigned int idx[PT64_ROOT_LEVEL];
+	struct kvm_mmu_page *parent[PT64_ROOT_LEVEL - 1];
+	unsigned int idx[PT64_ROOT_LEVEL - 1];
 };
 
 #define for_each_sp(pvec, sp, parents, i)			\
-		for (i = mmu_pages_first(&pvec, &parents);	\
+		for (i =  mmu_pages_next(&pvec, &parents, -1);	\
 			i < pvec.nr && ({ sp = pvec.page[i].sp; 1;});	\
 			i = mmu_pages_next(&pvec, &parents, i))
 
@@ -2016,25 +2016,15 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec,
 	return n;
 }
 
-static int mmu_pages_first(struct kvm_mmu_pages *pvec,
-			   struct mmu_page_path *parents)
+static void
+mmu_pages_init(struct mmu_page_path *parents, struct kvm_mmu_page *parent)
 {
-	struct kvm_mmu_page *sp;
-	int level;
-
-	if (pvec->nr == 0)
-		return 0;
-
-	sp = pvec->page[0].sp;
-	level = sp->role.level;
-	WARN_ON(level == PT_PAGE_TABLE_LEVEL);
-
 	/*
-	 * Also set up a sentinel. Further entries in pvec are all
-	 * children of sp, so this element is never overwritten.
+	 * set up a sentinel. Further entries in pvec are all children of
+	 * sp, so this element is never overwritten.
 	 */
-	parents->parent[level - 1] = NULL;
-	return mmu_pages_next(pvec, parents, -1);
+	if (parent->role.level < PT64_ROOT_LEVEL)
+		parents->parent[parent->role.level - 1] = NULL;
 }
 
 static void mmu_pages_clear_parents(struct mmu_page_path *parents)
@@ -2051,7 +2041,7 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents)
 		WARN_ON(idx == INVALID_INDEX);
 		clear_unsync_child_bit(sp, idx);
 		level++;
-	} while (!sp->unsync_children);
+	} while (!sp->unsync_children && (level < PT64_ROOT_LEVEL - 1));
 }
 
 static void mmu_sync_children(struct kvm_vcpu *vcpu,
@@ -2064,6 +2054,7 @@ static void mmu_sync_children(struct kvm_vcpu *vcpu,
 	LIST_HEAD(invalid_list);
 	bool flush = false;
 
+	mmu_pages_init(&parents, parent);
 	while (mmu_unsync_walk(parent, &pages)) {
 		bool protected = false;
 
@@ -2335,6 +2326,7 @@ static int mmu_zap_unsync_children(struct kvm *kvm,
 	if (parent->role.level == PT_PAGE_TABLE_LEVEL)
 		return 0;
 
+	mmu_pages_init(&parents, parent);
 	while (mmu_unsync_walk(parent, &pages)) {
 		struct kvm_mmu_page *sp;
 
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1364671 — Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

On 25/03/2016 14:19, Xiao Guangrong wrote:
> Currently only PT64_ROOT_LEVEL - 1 levels are used, one additional entry
> in .parent[] is used as a sentinel, the additional entry in .idx[] is
> purely wasted
> 
> This patch reduces its size and sets the sentinel on the upper level of
> the place where we start from

This patch and the previous one are basically redoing commit
0a47cd85833e ("KVM: MMU: Fix ubsan warnings", 2016-03-04).  While you
find your version easier to understand, I of course find mine easier.

Rather than getting stuck in a ko fight, the solution is to stick with
the code in KVM and add comments.  I'll give it a try...

Paolo

[toc] | [prev] | [next] | [standalone]

#1364673 — Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

On 25/03/2016 14:48, Xiao Guangrong wrote:
>>>
>>
>> This patch and the previous one are basically redoing commit
>> 0a47cd85833e ("KVM: MMU: Fix ubsan warnings", 2016-03-04).  While you
>> find your version easier to understand, I of course find mine easier.
>>
>> Rather than getting stuck in a ko fight, the solution is to stick with
>> the code in KVM and add comments.  I'll give it a try...
> 
> If you do not like this one, we can just make the .index is
> [PT64_ROOT_LEVEL - 1] and keep the sentinel in .parents[], that little
> change and nice code shape.

I suppose you'd have something like this then:

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 70e95d097ef1..15e1735a2e3a 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1980,7 +1980,7 @@ static bool kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn,
 
 struct mmu_page_path {
 	struct kvm_mmu_page *parent[PT64_ROOT_LEVEL];
-	unsigned int idx[PT64_ROOT_LEVEL];
+	unsigned int idx[PT64_ROOT_LEVEL-1];
 };
 
 #define for_each_sp(pvec, sp, parents, i)			\
@@ -2037,13 +2037,14 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents)
 {
 	struct kvm_mmu_page *sp;
 	unsigned int level = 0;
+	unsigned int idx;
 
 	do {
-		unsigned int idx = parents->idx[level];
 		sp = parents->parent[level];
-		if (!sp)
+		if (!sp || WARN_ON(level == PT64_ROOT_LEVEL-1))
 			return;
 
+		idx = parents->idx[level];
 		WARN_ON(idx == INVALID_INDEX);
 		clear_unsync_child_bit(sp, idx);
 		level++;

By making the arrays the same size, the effect of the sentinel seems
clearer to me.  It doesn't seem worth 4 bytes (and strictly speaking
those 4 bytes would be there anyway due to padding)...

Paolo

[toc] | [prev] | [next] | [standalone]

#1364679 — Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

On 03/25/2016 09:56 PM, Paolo Bonzini wrote:
>
>
> On 25/03/2016 14:48, Xiao Guangrong wrote:
>>>>
>>>
>>> This patch and the previous one are basically redoing commit
>>> 0a47cd85833e ("KVM: MMU: Fix ubsan warnings", 2016-03-04).  While you
>>> find your version easier to understand, I of course find mine easier.
>>>
>>> Rather than getting stuck in a ko fight, the solution is to stick with
>>> the code in KVM and add comments.  I'll give it a try...
>>
>> If you do not like this one, we can just make the .index is
>> [PT64_ROOT_LEVEL - 1] and keep the sentinel in .parents[], that little
>> change and nice code shape.
>
> I suppose you'd have something like this then:
>
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 70e95d097ef1..15e1735a2e3a 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -1980,7 +1980,7 @@ static bool kvm_sync_pages(struct kvm_vcpu *vcpu, gfn_t gfn,
>
>   struct mmu_page_path {
>   	struct kvm_mmu_page *parent[PT64_ROOT_LEVEL];
> -	unsigned int idx[PT64_ROOT_LEVEL];
> +	unsigned int idx[PT64_ROOT_LEVEL-1];
>   };
>
>   #define for_each_sp(pvec, sp, parents, i)			\
> @@ -2037,13 +2037,14 @@ static void mmu_pages_clear_parents(struct mmu_page_path *parents)
>   {
>   	struct kvm_mmu_page *sp;
>   	unsigned int level = 0;
> +	unsigned int idx;
>
>   	do {
> -		unsigned int idx = parents->idx[level];
>   		sp = parents->parent[level];
> -		if (!sp)
> +		if (!sp || WARN_ON(level == PT64_ROOT_LEVEL-1))
>   			return;
>
> +		idx = parents->idx[level];
>   		WARN_ON(idx == INVALID_INDEX);
>   		clear_unsync_child_bit(sp, idx);
>   		level++;
>

Yes, exactly.

[ actually, we can keep mmu_pages_clear_parents() unchanged ]

> By making the arrays the same size, the effect of the sentinel seems
> clearer to me.  It doesn't seem worth 4 bytes (and strictly speaking
> those 4 bytes would be there anyway due to padding)...

The sentinel is NULL forever so it can not go to the inner loop anyway...

Okay, i am not strong opinion on it, it is not a big deal. Let's
happily drop it if you really dislike it. :)

[toc] | [prev] | [next] | [standalone]

#1364686 — Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

On 25/03/2016 15:07, Xiao Guangrong wrote:
>>
>> @@ -2037,13 +2037,14 @@ static void mmu_pages_clear_parents(struct
>> mmu_page_path *parents)
>>   {
>>       struct kvm_mmu_page *sp;
>>       unsigned int level = 0;
>> +    unsigned int idx;
>>
>>       do {
>> -        unsigned int idx = parents->idx[level];
>>           sp = parents->parent[level];
>> -        if (!sp)
>> +        if (!sp || WARN_ON(level == PT64_ROOT_LEVEL-1))
>>               return;
>>
>> +        idx = parents->idx[level];
>>           WARN_ON(idx == INVALID_INDEX);
>>           clear_unsync_child_bit(sp, idx);
>>           level++;
>>
> 
> Yes, exactly.
> 
> [ actually, we can keep mmu_pages_clear_parents() unchanged ]

You cannot because ubsan would complain. :)

Paolo

[toc] | [prev] | [next] | [standalone]

#1364675 — Re: [PATCH 3/4] KVM: MMU: reduce the size of mmu_page_path

On 03/25/2016 09:45 PM, Paolo Bonzini wrote:
>
>
> On 25/03/2016 14:19, Xiao Guangrong wrote:
>> Currently only PT64_ROOT_LEVEL - 1 levels are used, one additional entry
>> in .parent[] is used as a sentinel, the additional entry in .idx[] is
>> purely wasted
>>
>> This patch reduces its size and sets the sentinel on the upper level of
>> the place where we start from
>
> This patch and the previous one are basically redoing commit
> 0a47cd85833e ("KVM: MMU: Fix ubsan warnings", 2016-03-04).  While you
> find your version easier to understand, I of course find mine easier.
>
> Rather than getting stuck in a ko fight, the solution is to stick with
> the code in KVM and add comments.  I'll give it a try...

If you do not like this one, we can just make the .index is
[PT64_ROOT_LEVEL - 1] and keep the sentinel in .parents[], that little
change and nice code shape.

[toc] | [prev] | [next] | [standalone]

#1364667

On 25/03/2016 14:19, Xiao Guangrong wrote:
> kvm-unit-tests complained about the PFEC is not set properly, e.g,:
> test pte.rw pte.d pte.nx pde.p pde.rw pde.pse user fetch: FAIL: error code 15 
> expected 5
> Dump mapping: address: 0x123400000000
> ------L4: 3e95007
> ------L3: 3e96007
> ------L2: 2000083

What's the command line for the reproducer?

> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
> index b70df72..81bffd1 100644
> --- a/arch/x86/kvm/mmu.h
> +++ b/arch/x86/kvm/mmu.h
> @@ -154,7 +154,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  				  unsigned pfec)
>  {
>  	int cpl = kvm_x86_ops->get_cpl(vcpu);
> -	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
> +	unsigned long errcode, rflags = kvm_x86_ops->get_rflags(vcpu);
>  
>  	/*
>  	 * If CPL < 3, SMAP prevention are disabled if EFLAGS.AC = 1.
> @@ -175,7 +175,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  	bool fault = (mmu->permissions[index] >> pte_access) & 1;
>  
>  	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
> -	pfec |= PFERR_PRESENT_MASK;
> +	errcode = PFERR_PRESENT_MASK;

So is this patch doing the same as "KVM: MMU: precompute page fault
error code"?  It was necessary after all. :)

Paolo

>  
>  	if (unlikely(mmu->pkru_mask)) {
>  		u32 pkru_bits, offset;
> @@ -193,11 +193,11 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
>  
>  		pkru_bits &= mmu->pkru_mask >> offset;
> -		pfec |= -pkru_bits & PFERR_PK_MASK;
> +		errcode |= -pkru_bits & PFERR_PK_MASK;
>  		fault |= (pkru_bits != 0);
>  	}
>  
> -	return -(uint32_t)fault & pfec;
> +	return -(uint32_t)fault & errcode;
>  }
>  
>  void kvm_mmu_invalidate_zap_all_pages(struct kvm *kvm);
> diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
> index 1d971c7..bc019f7 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -360,7 +360,7 @@ retry_walk:
>  			goto error;
>  
>  		if (unlikely(is_rsvd_bits_set(mmu, pte, walker->level))) {
> -			errcode |= PFERR_RSVD_MASK | PFERR_PRESENT_MASK;
> +			errcode = PFERR_RSVD_MASK | PFERR_PRESENT_MASK;
>  			goto error;
>  		}
>  

[toc] | [prev] | [next] | [standalone]

#1364670

On 03/25/2016 09:35 PM, Paolo Bonzini wrote:
>
>
> On 25/03/2016 14:19, Xiao Guangrong wrote:
>> kvm-unit-tests complained about the PFEC is not set properly, e.g,:
>> test pte.rw pte.d pte.nx pde.p pde.rw pde.pse user fetch: FAIL: error code 15
>> expected 5
>> Dump mapping: address: 0x123400000000
>> ------L4: 3e95007
>> ------L3: 3e96007
>> ------L2: 2000083
>
> What's the command line for the reproducer?

QEMU=/home/eric/qemu/x86_64-softmmu/qemu-system-x86_64 ./x86-run x86/access.flat

>
>> diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
>> index b70df72..81bffd1 100644
>> --- a/arch/x86/kvm/mmu.h
>> +++ b/arch/x86/kvm/mmu.h
>> @@ -154,7 +154,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>>   				  unsigned pfec)
>>   {
>>   	int cpl = kvm_x86_ops->get_cpl(vcpu);
>> -	unsigned long rflags = kvm_x86_ops->get_rflags(vcpu);
>> +	unsigned long errcode, rflags = kvm_x86_ops->get_rflags(vcpu);
>>
>>   	/*
>>   	 * If CPL < 3, SMAP prevention are disabled if EFLAGS.AC = 1.
>> @@ -175,7 +175,7 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>>   	bool fault = (mmu->permissions[index] >> pte_access) & 1;
>>
>>   	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
>> -	pfec |= PFERR_PRESENT_MASK;
>> +	errcode = PFERR_PRESENT_MASK;
>
> So is this patch doing the same as "KVM: MMU: precompute page fault
> error code"?  It was necessary after all. :)

Sorry for my mistake... I missed the logic you changed :(

I still prefer to calculating the error code on the fault path which is rare, or
think a way to encapsulate it to permission_fault()...

[toc] | [prev] | [next] | [standalone]

#1364676

On 25/03/2016 14:41, Xiao Guangrong wrote:
>>>
>>
>> So is this patch doing the same as "KVM: MMU: precompute page fault
>> error code"?  It was necessary after all. :)
> 
> Sorry for my mistake... I missed the logic you changed :(
> 
> I still prefer to calculating the error code on the fault path which is
> rare, or think a way to encapsulate it to permission_fault()...

Yes, I will apply your patch.

Paolo

[toc] | [prev] | [next] | [standalone]

#1364669 — [PATCH 2/4] KVM: MMU: simplify the logic of __mmu_unsync_walk()

Each time i looked into the logic of walking unsync shadow pages, it costs
lots of time to understand what it is doing. The trick of this logic is
that the item, sp and idx, saved to kvm_mmu_pages is the sp and the index
in the _parent_ level and it lacks any comment to explain this fact

This patch simplifies it by saving the sp and its index to kvm_mmu_pages,
then it is much easier to understand the operations on the its index

Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com>
---
 arch/x86/kvm/mmu.c | 40 ++++++++++++++++++++++------------------
 1 file changed, 22 insertions(+), 18 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 6bdfbc2..e273144 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -1830,6 +1830,8 @@ static inline void clear_unsync_child_bit(struct kvm_mmu_page *sp, int idx)
 	__clear_bit(idx, sp->unsync_child_bitmap);
 }
 
+#define INVALID_INDEX (-1)
+
 static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 			   struct kvm_mmu_pages *pvec)
 {
@@ -1846,10 +1848,10 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 
 		child = page_header(ent & PT64_BASE_ADDR_MASK);
 
-		if (child->unsync_children) {
-			if (mmu_pages_add(pvec, child, i))
-				return -ENOSPC;
+		if (mmu_pages_add(pvec, sp, i))
+			return -ENOSPC;
 
+		if (child->unsync_children) {
 			ret = __mmu_unsync_walk(child, pvec);
 			if (!ret) {
 				clear_unsync_child_bit(sp, i);
@@ -1860,7 +1862,13 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 				return ret;
 		} else if (child->unsync) {
 			nr_unsync_leaf++;
-			if (mmu_pages_add(pvec, child, i))
+
+			/*
+			 * the unsync is on the last level so its 'idx' is
+			 * useless, we set it to INVALID_INDEX to catch
+			 * potential bugs.
+			 */
+			if (mmu_pages_add(pvec, child, INVALID_INDEX))
 				return -ENOSPC;
 		} else
 			clear_unsync_child_bit(sp, i);
@@ -1869,8 +1877,6 @@ static int __mmu_unsync_walk(struct kvm_mmu_page *sp,
 	return nr_unsync_leaf;
 }
 
-#define INVALID_INDEX (-1)
-
 static int mmu_unsync_walk(struct kvm_mmu_page *sp,
 			   struct kvm_mmu_pages *pvec)
 {
@@ -1878,7 +1884,6 @@ static int mmu_unsync_walk(struct kvm_mmu_page *sp,
 	if (!sp->unsync_children)
 		return 0;
 
-	mmu_pages_add(pvec, sp, INVALID_INDEX);
 	return __mmu_unsync_walk(sp, pvec);
 }
 
@@ -1994,16 +1999,18 @@ static int mmu_pages_next(struct kvm_mmu_pages *pvec,
 {
 	int n;
 
-	for (n = i+1; n < pvec->nr; n++) {
+	for (n = i + 1; n < pvec->nr; n++) {
 		struct kvm_mmu_page *sp = pvec->page[n].sp;
 		unsigned idx = pvec->page[n].idx;
 		int level = sp->role.level;
 
-		parents->idx[level-1] = idx;
-		if (level == PT_PAGE_TABLE_LEVEL)
+		if (level == PT_PAGE_TABLE_LEVEL) {
+			WARN_ON(idx != INVALID_INDEX);
 			break;
+		}
 
-		parents->parent[level-2] = sp;
+		parents->idx[level - 2] = idx;
+		parents->parent[level - 2] = sp;
 	}
 
 	return n;
@@ -2018,19 +2025,16 @@ static int mmu_pages_first(struct kvm_mmu_pages *pvec,
 	if (pvec->nr == 0)
 		return 0;
 
-	WARN_ON(pvec->page[0].idx != INVALID_INDEX);
-
 	sp = pvec->page[0].sp;
 	level = sp->role.level;
 	WARN_ON(level == PT_PAGE_TABLE_LEVEL);
 
-	parents->parent[level-2] = sp;
-
-	/* Also set up a sentinel.  Further entries in pvec are all
+	/*
+	 * Also set up a sentinel. Further entries in pvec are all
 	 * children of sp, so this element is never overwritten.
 	 */
-	parents->parent[level-1] = NULL;
-	return mmu_pages_next(pvec, parents, 0);
+	parents->parent[level - 1] = NULL;
+	return mmu_pages_next(pvec, parents, -1);
 }
 
 static void mmu_pages_clear_parents(struct mmu_page_path *parents)
-- 
1.8.3.1

[toc] | [prev] | [next] | [standalone]

#1364688

On 25/03/2016 14:19, Xiao Guangrong wrote:
>  	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
> -	pfec |= PFERR_PRESENT_MASK;
> +	errcode = PFERR_PRESENT_MASK;
>  
>  	if (unlikely(mmu->pkru_mask)) {
>  		u32 pkru_bits, offset;
> @@ -193,11 +193,11 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>  			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
>  
>  		pkru_bits &= mmu->pkru_mask >> offset;
> -		pfec |= -pkru_bits & PFERR_PK_MASK;
> +		errcode |= -pkru_bits & PFERR_PK_MASK;
>  		fault |= (pkru_bits != 0);
>  	}
>  
> -	return -(uint32_t)fault & pfec;
> +	return -(uint32_t)fault & errcode;
>  }

I have another doubt here.

If you get a fault due to U=0, you would not get PFERR_PK_MASK.  This
is checked implicitly through the pte_user bit which we moved to
PFERR_RSVD_BIT.  However, if you get a fault due to W=0 _and_
PKRU.AD=1 or PKRU.WD=1 for the page's protection key, would the PK
bit be set in the error code?  If not, we would need something like
this:

diff --git a/arch/x86/kvm/mmu.h b/arch/x86/kvm/mmu.h
index 81bffd1524c4..6835a551a5c4 100644
--- a/arch/x86/kvm/mmu.h
+++ b/arch/x86/kvm/mmu.h
@@ -172,12 +172,11 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 	unsigned long smap = (cpl - 3) & (rflags & X86_EFLAGS_AC);
 	int index = (pfec >> 1) +
 		    (smap >> (X86_EFLAGS_AC_BIT - PFERR_RSVD_BIT + 1));
-	bool fault = (mmu->permissions[index] >> pte_access) & 1;
 
 	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
-	errcode = PFERR_PRESENT_MASK;
+	errcode = (mmu->permissions[index] >> pte_access) & PFERR_PRESENT_MASK;
 
-	if (unlikely(mmu->pkru_mask)) {
+	if (unlikely(-errcode & mmu->pkru_mask)) {
 		u32 pkru_bits, offset;
 
 		/*
@@ -188,11 +187,10 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
 			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
 
 		pkru_bits &= mmu->pkru_mask >> offset;
-		errcode |= -pkru_bits & PFERR_PK_MASK;
-		fault |= (pkru_bits != 0);
+		errcode |= pkru_bits ? PFERR_PK_MASK | PFERR_PRESENT_MASK : 0;
 	}
 
-	return -(uint32_t)fault & errcode;
+	return errcode;
 }
 
 void kvm_mmu_invalidate_zap_all_pages(struct kvm *kvm);


Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1366513

On 03/25/2016 10:21 PM, Paolo Bonzini wrote:
>
>
> On 25/03/2016 14:19, Xiao Guangrong wrote:
>>   	WARN_ON(pfec & (PFERR_PK_MASK | PFERR_RSVD_MASK));
>> -	pfec |= PFERR_PRESENT_MASK;
>> +	errcode = PFERR_PRESENT_MASK;
>>
>>   	if (unlikely(mmu->pkru_mask)) {
>>   		u32 pkru_bits, offset;
>> @@ -193,11 +193,11 @@ static inline u8 permission_fault(struct kvm_vcpu *vcpu, struct kvm_mmu *mmu,
>>   			((pte_access & PT_USER_MASK) << (PFERR_RSVD_BIT - PT_USER_SHIFT));
>>
>>   		pkru_bits &= mmu->pkru_mask >> offset;
>> -		pfec |= -pkru_bits & PFERR_PK_MASK;
>> +		errcode |= -pkru_bits & PFERR_PK_MASK;
>>   		fault |= (pkru_bits != 0);
>>   	}
>>
>> -	return -(uint32_t)fault & pfec;
>> +	return -(uint32_t)fault & errcode;
>>   }
>
> I have another doubt here.
>
> If you get a fault due to U=0, you would not get PFERR_PK_MASK.  This
> is checked implicitly through the pte_user bit which we moved to
> PFERR_RSVD_BIT.  However, if you get a fault due to W=0 _and_
> PKRU.AD=1 or PKRU.WD=1 for the page's protection key, would the PK
> bit be set in the error code?  If not, we would need something like
> this:

Based on the SDM:
PK flag (bit 5).
This flag is 1 if (1) IA32_EFER.LMA = CR4.PKE = 1; (2) the access causing the page-fault exception 
was a data access; (3) the linear address was a user-mode address with protection key i; and (5) the 
PKRU register (see Section 4.6.2) is such that either (a) ADi = 1; or (b) the following all hold: 
(i) WDi = 1; (ii) the access is a write access; and (iii) either CR0.WP = 1 or the access causing 
the page-fault exception was a user-mode access.

So I think PKEY check and ordinary check are independent, i.e, PFEC.PKEY may be
set even if the on permission on the page table is not adequate.

[toc] | [prev] | [next] | [standalone]

#1366632

On 29/03/2016 19:43, Xiao Guangrong wrote:
> Based on the SDM:
> PK flag (bit 5).
> This flag is 1 if (1) IA32_EFER.LMA = CR4.PKE = 1; (2) the access
> causing the page-fault exception was a data access; (3) the linear
> address was a user-mode address with protection key i; and (5) the PKRU
> register (see Section 4.6.2) is such that either (a) ADi = 1; or (b) the
> following all hold: (i) WDi = 1; (ii) the access is a write access; and
> (iii) either CR0.WP = 1 or the access causing the page-fault exception
> was a user-mode access.
> 
> So I think PKEY check and ordinary check are independent, i.e, PFEC.PKEY
> may be set even if the on permission on the page table is not adequate.

x86/access.flat is currently using the "other" definition, i.e., PFEC.PK
is only set if W=1 or CR0.WP=0 && PFEC.U=0 or PFEC.W=0.  Can you use it
(with ept=1 of course) to check what the processor is doing?

Paolo

[toc] | [prev] | [next] | [standalone]

#1366850

On 03/30/2016 04:09 AM, Paolo Bonzini wrote:
>
>
> On 29/03/2016 19:43, Xiao Guangrong wrote:
>> Based on the SDM:
>> PK flag (bit 5).
>> This flag is 1 if (1) IA32_EFER.LMA = CR4.PKE = 1; (2) the access
>> causing the page-fault exception was a data access; (3) the linear
>> address was a user-mode address with protection key i; and (5) the PKRU
>> register (see Section 4.6.2) is such that either (a) ADi = 1; or (b) the
>> following all hold: (i) WDi = 1; (ii) the access is a write access; and
>> (iii) either CR0.WP = 1 or the access causing the page-fault exception
>> was a user-mode access.
>>
>> So I think PKEY check and ordinary check are independent, i.e, PFEC.PKEY
>> may be set even if the on permission on the page table is not adequate.
>
> x86/access.flat is currently using the "other" definition, i.e., PFEC.PK
> is only set if W=1 or CR0.WP=0 && PFEC.U=0 or PFEC.W=0.  Can you use it
> (with ept=1 of course) to check what the processor is doing?
>

Sure.

And ept=1 is hard to trigger MMU issue, i am enabling PKEY on shadow
MMU, let's see what will happen. ;)

[toc] | [prev] | [next] | [standalone]

#1366911

On 30/03/2016 03:56, Xiao Guangrong wrote:
>> x86/access.flat is currently using the "other" definition, i.e., PFEC.PK
>> is only set if W=1 or CR0.WP=0 && PFEC.U=0 or PFEC.W=0.  Can you use it
>> (with ept=1 of course) to check what the processor is doing?
> 
> Sure.
> 
> And ept=1 is hard to trigger MMU issue, i am enabling PKEY on shadow
> MMU, let's see what will happen. ;)

No, don't do that!

ept=1 lets you test what the processor does.  It means you cannot test
permission_fault(), but what we want here is just reverse engineering
the microcode.  ept=1 lets you do exactly that.

Paolo

[toc] | [prev] | [next] | [standalone]

#1366920

On 03/30/2016 02:36 PM, Paolo Bonzini wrote:
>
>
> On 30/03/2016 03:56, Xiao Guangrong wrote:
>>> x86/access.flat is currently using the "other" definition, i.e., PFEC.PK
>>> is only set if W=1 or CR0.WP=0 && PFEC.U=0 or PFEC.W=0.  Can you use it
>>> (with ept=1 of course) to check what the processor is doing?
>>
>> Sure.
>>
>> And ept=1 is hard to trigger MMU issue, i am enabling PKEY on shadow
>> MMU, let's see what will happen. ;)
>
> No, don't do that!
>
> ept=1 lets you test what the processor does.  It means you cannot test
> permission_fault(), but what we want here is just reverse engineering
> the microcode.  ept=1 lets you do exactly that.

Yes, i got this point. Huaitong will do the test once the machine gets
free.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web