[PATCH v2] KVM MMU: check reserved bits only when CR4.PSE=1 or CR4.PAE=1

[PATCH v2] KVM MMU: check reserved bits only when CR4.PSE=1 or CR4.PAE=1

[Xiao Guangrong]

- The RSV bit is possibility set in error code when #PF occurred
  only if CR4.PSE=1 or CR4.PAE=1
  
- context->rsvd_bits_mask[1][0] is always 0

Changlog:
Move this operation to reset_rsvds_bits_mask() address Avi Kivity's suggestion

Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
---
 arch/x86/kvm/mmu.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index b137515..c49f8ec 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2288,18 +2288,26 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 
 	if (!is_nx(vcpu))
 		exb_bit_rsvd = rsvd_bits(63, 63);
+
+	context->rsvd_bits_mask[1][0] = 0;
 	switch (level) {
 	case PT32_ROOT_LEVEL:
 		/* no rsvd bits for 2 level 4K page table entries */
 		context->rsvd_bits_mask[0][1] = 0;
 		context->rsvd_bits_mask[0][0] = 0;
+
+		/* check rsvd bits only when CR4.PSE=1 or CR4.PAE=1 */
+		if (!is_pse(vcpu)) {
+			context->rsvd_bits_mask[1][1] = 0;
+			break;
+		}
+
 		if (is_cpuid_PSE36())
 			/* 36bits PSE 4MB page */
 			context->rsvd_bits_mask[1][1] = rsvd_bits(17, 21);
 		else
 			/* 32 bits PSE 4MB page */
 			context->rsvd_bits_mask[1][1] = rsvd_bits(13, 21);
-		context->rsvd_bits_mask[1][0] = context->rsvd_bits_mask[1][0];
 		break;
 	case PT32E_ROOT_LEVEL:
 		context->rsvd_bits_mask[0][2] =
@@ -2312,7 +2320,6 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 		context->rsvd_bits_mask[1][1] = exb_bit_rsvd |
 			rsvd_bits(maxphyaddr, 62) |
 			rsvd_bits(13, 20);		/* large page */
-		context->rsvd_bits_mask[1][0] = context->rsvd_bits_mask[1][0];
 		break;
 	case PT64_ROOT_LEVEL:
 		context->rsvd_bits_mask[0][3] = exb_bit_rsvd |
@@ -2330,7 +2337,6 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
 		context->rsvd_bits_mask[1][1] = exb_bit_rsvd |
 			rsvd_bits(maxphyaddr, 51) |
 			rsvd_bits(13, 20);		/* large page */
-		context->rsvd_bits_mask[1][0] = context->rsvd_bits_mask[1][0];
 		break;
 	}
 }
-- 
1.6.1.2

Re: [PATCH v2] KVM MMU: check reserved bits only when CR4.PSE=1 or CR4.PAE=1

[Marcelo Tosatti]

On Wed, Mar 17, 2010 at 11:43:06AM +0800, Xiao Guangrong wrote:
> - The RSV bit is possibility set in error code when #PF occurred
>   only if CR4.PSE=1 or CR4.PAE=1
>   
> - context->rsvd_bits_mask[1][0] is always 0
> 
> Changlog:
> Move this operation to reset_rsvds_bits_mask() address Avi Kivity's suggestion
> 
> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
> ---
>  arch/x86/kvm/mmu.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index b137515..c49f8ec 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2288,18 +2288,26 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
>  
>  	if (!is_nx(vcpu))
>  		exb_bit_rsvd = rsvd_bits(63, 63);
> +
> +	context->rsvd_bits_mask[1][0] = 0;

So if the guest enables PAT at PTE level you completly disable reserved
bit checking? You should only disable checking for [1][1] if !PSE.

Re: [PATCH v2] KVM MMU: check reserved bits only when CR4.PSE=1 or CR4.PAE=1

[Xiao Guangrong]

Hi Marcelo,

Thanks for your review.

Marcelo Tosatti wrote:
> On Wed, Mar 17, 2010 at 11:43:06AM +0800, Xiao Guangrong wrote:
>> - The RSV bit is possibility set in error code when #PF occurred
>>   only if CR4.PSE=1 or CR4.PAE=1
>>   
>> - context->rsvd_bits_mask[1][0] is always 0
>>
>> Changlog:
>> Move this operation to reset_rsvds_bits_mask() address Avi Kivity's suggestion
>>
>> Signed-off-by: Xiao Guangrong <xiaoguangrong@cn.fujitsu.com>
>> ---
>>  arch/x86/kvm/mmu.c |   12 +++++++++---
>>  1 files changed, 9 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
>> index b137515..c49f8ec 100644
>> --- a/arch/x86/kvm/mmu.c
>> +++ b/arch/x86/kvm/mmu.c
>> @@ -2288,18 +2288,26 @@ static void reset_rsvds_bits_mask(struct kvm_vcpu *vcpu, int level)
>>  
>>  	if (!is_nx(vcpu))
>>  		exb_bit_rsvd = rsvd_bits(63, 63);
>> +
>> +	context->rsvd_bits_mask[1][0] = 0;
> 
> So if the guest enables PAT at PTE level you completly disable reserved
> bit checking? You should only disable checking for [1][1] if !PSE.

Sorry, i make a mistake here because i see the current code is redundant:
	context->rsvd_bits_mask[1][0] = context->rsvd_bits_mask[1][0];
in every case.

This code is imported by commit fd2e987d, i think this is Avi Kivity's typo :-)
i think the correct way is:
	ontext->rsvd_bits_mask[1][0] = context->rsvd_bits_mask[0][0];

I'll send a new version patch to fix it if you not object. 

Xiao