[PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[Wanpeng Li]

EPT misconfig handler in kvm will check which reason lead to EPT 
misconfiguration after vmexit. One of the reasons is that an EPT 
paging-structure entry is configured with settings reserved for 
future functionality. However, the handler can't identify if 
paging-structure entry of reserved bits for 1-GByte page are 
configured, since PDPTE which point to 1-GByte page will reserve 
bits 29:12 instead of bits 7:3 which are reserved for PDPTE that 
references an EPT Page Directory. This patch fix it by reserve 
bits 29:12 for 1-GByte page. 

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
v1 -> v2:
 * same "if" statement cover both 2MB and 1GB pages
 * return 0xf8 for level == 4
 * get the level by checking the return value of ept_rsvd_mask 

 arch/x86/kvm/vmx.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index cad37d5..2763f37 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -5521,17 +5521,12 @@ static u64 ept_rsvd_mask(u64 spte, int level)
 	for (i = 51; i > boot_cpu_data.x86_phys_bits; i--)
 		mask |= (1ULL << i);
 
-	if (level > 2)
-		/* bits 7:3 reserved */
-		mask |= 0xf8;
-	else if (level == 2) {
-		if (spte & (1ULL << 7))
-			/* 2MB ref, bits 20:12 reserved */
-			mask |= 0x1ff000;
-		else
-			/* bits 6:3 reserved */
-			mask |= 0x78;
-	}
+	if (spte & (1ULL << 7))
+		/* 1GB/2MB page, bits 29:12 or 20:12 reserved respectively */
+		mask |= (PAGE_SIZE << ((level - 1) * 9)) - PAGE_SIZE;
+	else
+		/* bits 6:3 reserved */
+		mask |= 0x78;
 
 	return mask;
 }
@@ -5561,7 +5556,7 @@ static void ept_misconfig_inspect_spte(struct kvm_vcpu *vcpu, u64 spte,
 			WARN_ON(1);
 		}
 
-		if (level == 1 || (level == 2 && (spte & (1ULL << 7)))) {
+		if (level == 1 || (rsvd_bits & 0x38)) {
 			u64 ept_mem_type = (spte & 0x38) >> 3;
 
 			if (ept_mem_type == 2 || ept_mem_type == 3 ||
-- 
1.9.1

[PATCH v2 2/3] KVM: x86: fix check legal type of Variable Range MTRRs

[Wanpeng Li]

The first entry in each pair(IA32_MTRR_PHYSBASEn) defines the base 
address and memory type for the range; the second entry(IA32_MTRR_PHYSMASKn)
contains a mask used to determine the address range. The legal values 
for the type field of IA32_MTRR_PHYSBASEn are 0,1,4,5, and 6. However,
IA32_MTRR_PHYSMASKn don't have type field. This patch avoid check if 
the type field is legal for IA32_MTRR_PHYSMASKn.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
v1 -> v2:
 * WARN_ON if not fall in variable Range MTRRs 
 * the base/mask can be separated just with an "&"

 arch/x86/kvm/x86.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 5f5edb6..fb3ea7a 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1747,7 +1747,13 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	}
 
 	/* variable MTRRs */
-	return valid_mtrr_type(data & 0xff);
+	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
+
+	if ((msr & 1) == 0)
+		/* MTRR base */
+		return valid_mtrr_type(data & 0xff);
+	/* MTRR mask */
+	return true;
 }
 
 static int set_msr_mtrr(struct kvm_vcpu *vcpu, u32 msr, u64 data)
-- 
1.9.1

[PATCH v2 3/3] KVM: x86: #GP when attempts to write reserved bits of Variable Range MTRRs

[Wanpeng Li]

Section 11.11.2.3 of the SDM mentions "All other bits in the IA32_MTRR_PHYSBASEn 
and IA32_MTRR_PHYSMASKn registers are reserved; the processor generates a 
general-protection exception(#GP) if software attempts to write to them". This 
patch do it in kvm.

Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
---
 arch/x86/kvm/x86.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index fb3ea7a..b85da5f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1726,6 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
 static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
 	int i;
+	u64 mask = 0;
 
 	if (!msr_mtrr_valid(msr))
 		return false;
@@ -1749,10 +1750,21 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	/* variable MTRRs */
 	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
 
-	if ((msr & 1) == 0)
+	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
+		mask |= (1ULL << i);
+	if ((msr & 1) == 0) {
 		/* MTRR base */
-		return valid_mtrr_type(data & 0xff);
-	/* MTRR mask */
+		if (!valid_mtrr_type(data & 0xff))
+			return false;
+		 mask |= 0xf00;
+	} else
+		/* MTRR mask */
+		mask |= 0x7ff;
+	if (data & mask) {
+		kvm_inject_gp(vcpu, 0);
+		return false;
+	}
+
 	return true;
 }
 
-- 
1.9.1

Re: [PATCH v2 3/3] KVM: x86: #GP when attempts to write reserved bits of Variable Range MTRRs

[Paolo Bonzini]

Il 19/08/2014 11:04, Wanpeng Li ha scritto:
> Section 11.11.2.3 of the SDM mentions "All other bits in the IA32_MTRR_PHYSBASEn 
> and IA32_MTRR_PHYSMASKn registers are reserved; the processor generates a 
> general-protection exception(#GP) if software attempts to write to them". This 
> patch do it in kvm.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
>  arch/x86/kvm/x86.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb3ea7a..b85da5f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1726,6 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
>  static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	int i;
> +	u64 mask = 0;
>  
>  	if (!msr_mtrr_valid(msr))
>  		return false;
> @@ -1749,10 +1750,21 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  	/* variable MTRRs */
>  	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
>  
> -	if ((msr & 1) == 0)
> +	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
> +		mask |= (1ULL << i);
> +	if ((msr & 1) == 0) {
>  		/* MTRR base */
> -		return valid_mtrr_type(data & 0xff);
> -	/* MTRR mask */
> +		if (!valid_mtrr_type(data & 0xff))
> +			return false;
> +		 mask |= 0xf00;
> +	} else
> +		/* MTRR mask */
> +		mask |= 0x7ff;
> +	if (data & mask) {
> +		kvm_inject_gp(vcpu, 0);
> +		return false;
> +	}
> +
>  	return true;
>  }
>  
> 

Thanks, these two patches look good.  Please write a testcase for
kvm-unit-tests (x86/msr.c), too.

Paolo

Re: [PATCH v2 3/3] KVM: x86: #GP when attempts to write reserved bits of Variable Range MTRRs

[Paolo Bonzini]

Il 19/08/2014 11:04, Wanpeng Li ha scritto:
> Section 11.11.2.3 of the SDM mentions "All other bits in the IA32_MTRR_PHYSBASEn 
> and IA32_MTRR_PHYSMASKn registers are reserved; the processor generates a 
> general-protection exception(#GP) if software attempts to write to them". This 
> patch do it in kvm.
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>

This breaks if the guest maxphyaddr is higher than the host's (which 
sometimes happens depending on your hardware and how QEMU is 
configured).

You need to use cpuid_maxphyaddr, like this

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a375dfc42f6a..916e89515210 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1726,7 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
 static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 {
 	int i;
-	u64 mask = 0;
+	u64 mask;
 
 	if (!msr_mtrr_valid(msr))
 		return false;
@@ -1750,8 +1750,7 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
 	/* variable MTRRs */
 	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
 
-	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
-		mask |= (1ULL << i);
+	mask = (~0ULL) << cpuid_maxphyaddr(vcpu);
 	if ((msr & 1) == 0) {
 		/* MTRR base */
 		if (!valid_mtrr_type(data & 0xff))


Jan, can you see if this patch fixes the SeaBIOS triple fault you reported?

Paolo

> ---
>  arch/x86/kvm/x86.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index fb3ea7a..b85da5f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1726,6 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
>  static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	int i;
> +	u64 mask = 0;
>  
>  	if (!msr_mtrr_valid(msr))
>  		return false;
> @@ -1749,10 +1750,21 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  	/* variable MTRRs */
>  	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
>  
> -	if ((msr & 1) == 0)
> +	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
> +		mask |= (1ULL << i);
> +	if ((msr & 1) == 0) {
>  		/* MTRR base */
> -		return valid_mtrr_type(data & 0xff);
> -	/* MTRR mask */
> +		if (!valid_mtrr_type(data & 0xff))
> +			return false;
> +		 mask |= 0xf00;
> +	} else
> +		/* MTRR mask */
> +		mask |= 0x7ff;
> +	if (data & mask) {
> +		kvm_inject_gp(vcpu, 0);
> +		return false;
> +	}
> +
>  	return true;
>  }
>  
> 

Re: [PATCH v2 3/3] KVM: x86: #GP when attempts to write reserved bits of Variable Range MTRRs

[Jan Kiszka]

On 2014-08-29 18:47, Paolo Bonzini wrote:
> Il 19/08/2014 11:04, Wanpeng Li ha scritto:
>> Section 11.11.2.3 of the SDM mentions "All other bits in the IA32_MTRR_PHYSBASEn 
>> and IA32_MTRR_PHYSMASKn registers are reserved; the processor generates a 
>> general-protection exception(#GP) if software attempts to write to them". This 
>> patch do it in kvm.
>>
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> 
> This breaks if the guest maxphyaddr is higher than the host's (which 
> sometimes happens depending on your hardware and how QEMU is 
> configured).
> 
> You need to use cpuid_maxphyaddr, like this
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a375dfc42f6a..916e89515210 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -1726,7 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
>  static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  {
>  	int i;
> -	u64 mask = 0;
> +	u64 mask;
>  
>  	if (!msr_mtrr_valid(msr))
>  		return false;
> @@ -1750,8 +1750,7 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>  	/* variable MTRRs */
>  	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
>  
> -	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
> -		mask |= (1ULL << i);
> +	mask = (~0ULL) << cpuid_maxphyaddr(vcpu);
>  	if ((msr & 1) == 0) {
>  		/* MTRR base */
>  		if (!valid_mtrr_type(data & 0xff))
> 
> 
> Jan, can you see if this patch fixes the SeaBIOS triple fault you reported?

Yep, it does.

Jan

-- 
Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 3/3] KVM: x86: #GP when attempts to write reserved bits of Variable Range MTRRs

[Wanpeng Li]

On Fri, Aug 29, 2014 at 06:47:54PM +0200, Paolo Bonzini wrote:
>Il 19/08/2014 11:04, Wanpeng Li ha scritto:
>> Section 11.11.2.3 of the SDM mentions "All other bits in the IA32_MTRR_PHYSBASEn 
>> and IA32_MTRR_PHYSMASKn registers are reserved; the processor generates a 
>> general-protection exception(#GP) if software attempts to write to them". This 
>> patch do it in kvm.
>> 
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>
>This breaks if the guest maxphyaddr is higher than the host's (which 
>sometimes happens depending on your hardware and how QEMU is 
>configured).
>
>You need to use cpuid_maxphyaddr, like this
>
>diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>index a375dfc42f6a..916e89515210 100644
>--- a/arch/x86/kvm/x86.c
>+++ b/arch/x86/kvm/x86.c
>@@ -1726,7 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
> static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> {
> 	int i;
>-	u64 mask = 0;
>+	u64 mask;
> 
> 	if (!msr_mtrr_valid(msr))
> 		return false;
>@@ -1750,8 +1750,7 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> 	/* variable MTRRs */
> 	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
> 
>-	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
>-		mask |= (1ULL << i);
>+	mask = (~0ULL) << cpuid_maxphyaddr(vcpu);
> 	if ((msr & 1) == 0) {
> 		/* MTRR base */
> 		if (!valid_mtrr_type(data & 0xff))
>
>

Got it, thanks Paolo and Jan. 

Regards,
Wanpeng Li 

>Jan, can you see if this patch fixes the SeaBIOS triple fault you reported?
>
>Paolo
>
>> ---
>>  arch/x86/kvm/x86.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index fb3ea7a..b85da5f 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -1726,6 +1726,7 @@ static bool valid_mtrr_type(unsigned t)
>>  static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>  {
>>  	int i;
>> +	u64 mask = 0;
>>  
>>  	if (!msr_mtrr_valid(msr))
>>  		return false;
>> @@ -1749,10 +1750,21 @@ static bool mtrr_valid(struct kvm_vcpu *vcpu, u32 msr, u64 data)
>>  	/* variable MTRRs */
>>  	WARN_ON(!(msr >= 0x200 && msr < 0x200 + 2 * KVM_NR_VAR_MTRR));
>>  
>> -	if ((msr & 1) == 0)
>> +	for (i = 63; i > boot_cpu_data.x86_phys_bits; i--)
>> +		mask |= (1ULL << i);
>> +	if ((msr & 1) == 0) {
>>  		/* MTRR base */
>> -		return valid_mtrr_type(data & 0xff);
>> -	/* MTRR mask */
>> +		if (!valid_mtrr_type(data & 0xff))
>> +			return false;
>> +		 mask |= 0xf00;
>> +	} else
>> +		/* MTRR mask */
>> +		mask |= 0x7ff;
>> +	if (data & mask) {
>> +		kvm_inject_gp(vcpu, 0);
>> +		return false;
>> +	}
>> +
>>  	return true;
>>  }
>>  
>> 

Re: [PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[Paolo Bonzini]

Il 19/08/2014 11:04, Wanpeng Li ha scritto:
> EPT misconfig handler in kvm will check which reason lead to EPT 
> misconfiguration after vmexit. One of the reasons is that an EPT 
> paging-structure entry is configured with settings reserved for 
> future functionality. However, the handler can't identify if 
> paging-structure entry of reserved bits for 1-GByte page are 
> configured, since PDPTE which point to 1-GByte page will reserve 
> bits 29:12 instead of bits 7:3 which are reserved for PDPTE that 
> references an EPT Page Directory. This patch fix it by reserve 
> bits 29:12 for 1-GByte page. 
> 
> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
> ---
> v1 -> v2:
>  * same "if" statement cover both 2MB and 1GB pages
>  * return 0xf8 for level == 4

I think you dropped this check by mistake.

>  * get the level by checking the return value of ept_rsvd_mask 
> 
>  arch/x86/kvm/vmx.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index cad37d5..2763f37 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -5521,17 +5521,12 @@ static u64 ept_rsvd_mask(u64 spte, int level)
>  	for (i = 51; i > boot_cpu_data.x86_phys_bits; i--)
>  		mask |= (1ULL << i);
>  
> -	if (level > 2)
> -		/* bits 7:3 reserved */
> -		mask |= 0xf8;
> -	else if (level == 2) {
> -		if (spte & (1ULL << 7))
> -			/* 2MB ref, bits 20:12 reserved */
> -			mask |= 0x1ff000;
> -		else
> -			/* bits 6:3 reserved */
> -			mask |= 0x78;
> -	}
> +	if (spte & (1ULL << 7))

You need to go this way if level == 1 too.  Otherwise, you would report
bits 6:3 reserved if the hypervisor is using the ignored bit 7 (Table
28-6, Format of an EPT Page-Table Entry).

> +		/* 1GB/2MB page, bits 29:12 or 20:12 reserved respectively */
> +		mask |= (PAGE_SIZE << ((level - 1) * 9)) - PAGE_SIZE;
> +	else
> +		/* bits 6:3 reserved */
> +		mask |= 0x78;
>  
>  	return mask;
>  }
> @@ -5561,7 +5556,7 @@ static void ept_misconfig_inspect_spte(struct kvm_vcpu *vcpu, u64 spte,
>  			WARN_ON(1);
>  		}
>  
> -		if (level == 1 || (level == 2 && (spte & (1ULL << 7)))) {
> +		if (level == 1 || (rsvd_bits & 0x38)) {

- rsvd_bits will always be zero here.  You need to check the return
value of ept_rsvd_mask().  Let's call it rsvd_mask in the rest of this
email.

- the test is inverted, you need to check that bits 5:3 are _not_
reserved, hence (rsvd_mask & 0x38) == 0.

- once you do this, the test also covers level 1.

I suggest that you write a testcase for kvm-unit-tests.

Paolo

Re: [PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[Wanpeng Li]

On Tue, Aug 19, 2014 at 11:09:49AM +0200, Paolo Bonzini wrote:
>Il 19/08/2014 11:04, Wanpeng Li ha scritto:
>> EPT misconfig handler in kvm will check which reason lead to EPT 
>> misconfiguration after vmexit. One of the reasons is that an EPT 
>> paging-structure entry is configured with settings reserved for 
>> future functionality. However, the handler can't identify if 
>> paging-structure entry of reserved bits for 1-GByte page are 
>> configured, since PDPTE which point to 1-GByte page will reserve 
>> bits 29:12 instead of bits 7:3 which are reserved for PDPTE that 
>> references an EPT Page Directory. This patch fix it by reserve 
>> bits 29:12 for 1-GByte page. 
>> 
>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>> ---
>> v1 -> v2:
>>  * same "if" statement cover both 2MB and 1GB pages
>>  * return 0xf8 for level == 4
>
>I think you dropped this check by mistake.

Indeed. I will do it in next version.

>
>>  * get the level by checking the return value of ept_rsvd_mask 
>> 
>>  arch/x86/kvm/vmx.c | 19 +++++++------------
>>  1 file changed, 7 insertions(+), 12 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index cad37d5..2763f37 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -5521,17 +5521,12 @@ static u64 ept_rsvd_mask(u64 spte, int level)
>>  	for (i = 51; i > boot_cpu_data.x86_phys_bits; i--)
>>  		mask |= (1ULL << i);
>>  
>> -	if (level > 2)
>> -		/* bits 7:3 reserved */
>> -		mask |= 0xf8;
>> -	else if (level == 2) {
>> -		if (spte & (1ULL << 7))
>> -			/* 2MB ref, bits 20:12 reserved */
>> -			mask |= 0x1ff000;
>> -		else
>> -			/* bits 6:3 reserved */
>> -			mask |= 0x78;
>> -	}
>> +	if (spte & (1ULL << 7))
>
>You need to go this way if level == 1 too.  Otherwise, you would report
>bits 6:3 reserved if the hypervisor is using the ignored bit 7 (Table
>28-6, Format of an EPT Page-Table Entry).
>

Agreed. What still need to do here is to update the comments in order to  
include level == 1, right?

>> +		/* 1GB/2MB page, bits 29:12 or 20:12 reserved respectively */
>> +		mask |= (PAGE_SIZE << ((level - 1) * 9)) - PAGE_SIZE;
>> +	else
>> +		/* bits 6:3 reserved */
>> +		mask |= 0x78;
>>  
>>  	return mask;
>>  }
>> @@ -5561,7 +5556,7 @@ static void ept_misconfig_inspect_spte(struct kvm_vcpu *vcpu, u64 spte,
>>  			WARN_ON(1);
>>  		}
>>  
>> -		if (level == 1 || (level == 2 && (spte & (1ULL << 7)))) {
>> +		if (level == 1 || (rsvd_bits & 0x38)) {
>
>- rsvd_bits will always be zero here.  You need to check the return
>value of ept_rsvd_mask().  Let's call it rsvd_mask in the rest of this
>email.
>
>- the test is inverted, you need to check that bits 5:3 are _not_
>reserved, hence (rsvd_mask & 0x38) == 0.
>
>- once you do this, the test also covers level 1.

Agreed.

>
>I suggest that you write a testcase for kvm-unit-tests.
>

Ok. 

Regards,
Wanpeng Li 

>Paolo

Re: [PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[Paolo Bonzini]

Il 19/08/2014 13:16, Wanpeng Li ha scritto:
> On Tue, Aug 19, 2014 at 11:09:49AM +0200, Paolo Bonzini wrote:
>> Il 19/08/2014 11:04, Wanpeng Li ha scritto:
>>> EPT misconfig handler in kvm will check which reason lead to EPT 
>>> misconfiguration after vmexit. One of the reasons is that an EPT 
>>> paging-structure entry is configured with settings reserved for 
>>> future functionality. However, the handler can't identify if 
>>> paging-structure entry of reserved bits for 1-GByte page are 
>>> configured, since PDPTE which point to 1-GByte page will reserve 
>>> bits 29:12 instead of bits 7:3 which are reserved for PDPTE that 
>>> references an EPT Page Directory. This patch fix it by reserve 
>>> bits 29:12 for 1-GByte page. 
>>>
>>> Signed-off-by: Wanpeng Li <wanpeng.li@linux.intel.com>
>>> ---
>>> v1 -> v2:
>>>  * same "if" statement cover both 2MB and 1GB pages
>>>  * return 0xf8 for level == 4
>>
>> I think you dropped this check by mistake.
> 
> Indeed. I will do it in next version.
> 
>>
>>>  * get the level by checking the return value of ept_rsvd_mask 
>>>
>>>  arch/x86/kvm/vmx.c | 19 +++++++------------
>>>  1 file changed, 7 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index cad37d5..2763f37 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -5521,17 +5521,12 @@ static u64 ept_rsvd_mask(u64 spte, int level)
>>>  	for (i = 51; i > boot_cpu_data.x86_phys_bits; i--)
>>>  		mask |= (1ULL << i);
>>>  
>>> -	if (level > 2)
>>> -		/* bits 7:3 reserved */
>>> -		mask |= 0xf8;
>>> -	else if (level == 2) {
>>> -		if (spte & (1ULL << 7))
>>> -			/* 2MB ref, bits 20:12 reserved */
>>> -			mask |= 0x1ff000;
>>> -		else
>>> -			/* bits 6:3 reserved */
>>> -			mask |= 0x78;
>>> -	}
>>> +	if (spte & (1ULL << 7))
>>
>> You need to go this way if level == 1 too.  Otherwise, you would report
>> bits 6:3 reserved if the hypervisor is using the ignored bit 7 (Table
>> 28-6, Format of an EPT Page-Table Entry).
>>
> 
> Agreed. What still need to do here is to update the comments in order to  
> include level == 1, right?

Yes.

>>> +		/* 1GB/2MB page, bits 29:12 or 20:12 reserved respectively */
>>> +		mask |= (PAGE_SIZE << ((level - 1) * 9)) - PAGE_SIZE;
>>> +	else
>>> +		/* bits 6:3 reserved */
>>> +		mask |= 0x78;
>>>  
>>>  	return mask;
>>>  }
>>> @@ -5561,7 +5556,7 @@ static void ept_misconfig_inspect_spte(struct kvm_vcpu *vcpu, u64 spte,
>>>  			WARN_ON(1);
>>>  		}
>>>  
>>> -		if (level == 1 || (level == 2 && (spte & (1ULL << 7)))) {
>>> +		if (level == 1 || (rsvd_bits & 0x38)) {
>>
>> - rsvd_bits will always be zero here.  You need to check the return
>> value of ept_rsvd_mask().  Let's call it rsvd_mask in the rest of this
>> email.
>>
>> - the test is inverted, you need to check that bits 5:3 are _not_
>> reserved, hence (rsvd_mask & 0x38) == 0.
>>
>> - once you do this, the test also covers level 1.
> 
> Agreed.

Thanks,

Paolo

Re: [PATCH v2 1/3] KVM: vmx: fix ept reserved bits for 1-GByte page

[Wanpeng Li]

Hi Paolo,
On Tue, Aug 19, 2014 at 11:09:49AM +0200, Paolo Bonzini wrote:
[...]
>I suggest that you write a testcase for kvm-unit-tests.
>

Just send out v3. The testcase will be written later since I'm not familiar
with kvm-unit-tests before and time is still needed.

Regards,
Wanpeng Li 

>Paolo