Skip to contentSkip to conversation

KVM: SVM: fix compiling warning

PATCH In review Xiao Guangrong 2 participants 7 messages May 26, 2010 – May 27, 2010

Subsystems (3)
x86 KVM KVM x86
lore
Share & tools
lore

Xiao Guangrong

author
#0 lore raw

PATCH 1/2 KVM: SVM: fix compiling warning

fix:

arch/x86/kvm/svm.c: In function ‘is_erratum_383’:
arch/x86/kvm/svm.c:1459: warning: integer constant is too large for ‘long’ type arch/x86/kvm/svm.c: In function ‘svm_handle_mce’:
arch/x86/kvm/svm.c:1490: warning: unused variable ‘kvm_run’

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/svm.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)
arch/x86/kvm/svm.c +1 -3
index 65324a0..02ea5cf 100644
@@ -1456,7 +1456,7 @@ static bool is_erratum_383(void)
 	/* Bit 62 may or may not be set for this mce */
 	value &= ~(1ULL << 62);
 
-	if (value != 0xb600000000010015)
+	if (value != 0xb600000000010015ULL)
 		return false;
 
 	/* Clear MCi_STATUS registers */

@@ -1487,8 +1487,6 @@ static void svm_handle_mce(struct vcpu_svm *svm)
 		 * Erratum 383 triggered. Guest state is corrupt so kill the
 		 * guest.
 		 */
-		struct kvm_run *kvm_run = svm->vcpu.run;
-
 		pr_err("KVM: Guest triggered AMD Erratum 383\n");
 
 		set_bit(KVM_REQ_TRIPLE_FAULT, &svm->vcpu.requests);
Signature · 1 line
1.6.1.2
author
#1 lore raw

PATCH 2/2 KVM: MMU: fix relaxing permission

There is a relaxing permission operation in set_spte():

if guest's CR0.WP is not set and R/W #PF occurs in supervisor-level, the mapping path might set to writable, then user can allow to write.

Signed-off-by Xiao Guangrong <[email protected]>
---
 arch/x86/kvm/mmu.c |    3 +--
 1 files changed, 1 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu.c +1 -2
index bfd8382..ccbc98b 100644
@@ -1859,8 +1859,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 
 	spte |= (u64)pfn << PAGE_SHIFT;
 
-	if ((pte_access & ACC_WRITE_MASK)
-	    || (write_fault && !is_write_protection(vcpu) && !user_fault)) {
+	if (pte_access & ACC_WRITE_MASK) {
 
 		if (level > PT_PAGE_TABLE_LEVEL &&
 		    has_wrprotected_page(vcpu->kvm, gfn, level)) {
Signature · 1 line
1.6.1.2
#2 lore raw

Re: [PATCH 2/2] KVM: MMU: fix relaxing permission

Quoted 6 lines · Xiao Guangrong

There is a relaxing permission operation in set_spte():

if guest's CR0.WP is not set and R/W #PF occurs in supervisor-level, the mapping path might set to writable, then user can allow to write.

Review static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep, · line 1864

Quoted 8 lines
(quoted diff) +1 -2
@@ -1859,8 +1859,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64 *sptep,
 
  	spte |= (u64)pfn<<  PAGE_SHIFT;
 
-	if ((pte_access&  ACC_WRITE_MASK)
-	    || (write_fault&&  !is_write_protection(vcpu)&&  !user_fault)) {
+	if (pte_access&  ACC_WRITE_MASK) {

The host always sets cr0.wp (in shadow mode) so we can write protect page tables. So when the guest clears cr0.wp, we emulate a gpte with gpte.w=0 and gpte.u=1 in two ways:

- spte.w=1, spte.u=0: this will allow the guest kernel to write but trap on guest user access
- spte.w=0, spte.u=1: allows guest user access but traps on guest kernel writes

If the guest attempts an access that is currently disallowed, we switch to the other spte encoding.

Signature · 1 line
error compiling committee.c: too many arguments to function
author
#3 lore raw

Re: [PATCH 2/2] KVM: MMU: fix relaxing permission

Quoted 21 lines · Avi Kivity
Quoted 6 lines · Xiao Guangrong

There is a relaxing permission operation in set_spte():

if guest's CR0.WP is not set and R/W #PF occurs in supervisor-level, the mapping path might set to writable, then user can allow to write.

Review static int set_spte(struct kvm_vcpu *vcpu, u64 · line 1865

Quoted 9 lines
(quoted diff) +1 -2
@@ -1859,8 +1859,7 @@ static int set_spte(struct kvm_vcpu *vcpu, u64
 *sptep,
 
      spte |= (u64)pfn<<  PAGE_SHIFT;
 
-    if ((pte_access&  ACC_WRITE_MASK)
-        || (write_fault&&  !is_write_protection(vcpu)&&  !user_fault)) {
+    if (pte_access&  ACC_WRITE_MASK) {

The host always sets cr0.wp (in shadow mode) so we can write protect page tables. So when the guest clears cr0.wp, we emulate a gpte with gpte.w=0 and gpte.u=1 in two ways:

Quoted 7 lines

- spte.w=1, spte.u=0: this will allow the guest kernel to write but trap on guest user access
- spte.w=0, spte.u=1: allows guest user access but traps on guest kernel writes

If the guest attempts an access that is currently disallowed, we switch to the other spte encoding.

Avi,

Thanks for your explanation, but i not see where to implement what you say, could you please point it out for me? :-(

And, i think use 'spte.w=1, spte.u=0' to emulate 'guest cr0.wp=0 and gpte.w=0' is not a good way since it can completely stop user process access, but in this case, user process is usually read and kernel lazily to write, just like vdso, it will generate a lots of #PF

#4 lore raw

Re: [PATCH 2/2] KVM: MMU: fix relaxing permission

Quoted 10 lines · Xiao Guangrong
Quoted 10 lines

The host always sets cr0.wp (in shadow mode) so we can write protect page tables. So when the guest clears cr0.wp, we emulate a gpte with gpte.w=0 and gpte.u=1 in two ways:

- spte.w=1, spte.u=0: this will allow the guest kernel to write but trap on guest user access
- spte.w=0, spte.u=1: allows guest user access but traps on guest kernel writes

Quoted 8 lines
Quoted 3 lines

If the guest attempts an access that is currently disallowed, we switch to the other spte encoding.

Avi,

Thanks for your explanation, but i not see where to implement what you say, could you please point it out for me? :-(

b70ccb0b3fd removed it accidentally:

Review

Quoted 10 lines
(quoted diff) +0 -10
-       } else
-               /*
-                * Kernel mode access.  Fail if it's a read-only page and
-                * supervisor write protection is enabled.
-                */
-               if (!writable_shadow) {
-                       if (is_write_protection(vcpu))
-                               return 0;
-                       *shadow_ent &= ~PT_USER_MASK;
-               }

:(

Quoted 5 lines

And, i think use 'spte.w=1, spte.u=0' to emulate 'guest cr0.wp=0 and gpte.w=0' is not a good way since it can completely stop user process access, but in this case, user process is usually read and kernel lazily to write, just like vdso, it will generate a lots of #PF

As soon as the guest kernel stops writing we switch back to gpte.w=gpte.u=1 and the guest can access it completely. For the case where both the kernel and userspace use interleaved access, you are right, but I don't see a better way, do you?

Signature · 1 line
error compiling committee.c: too many arguments to function
#5 lore raw

Re: [PATCH 2/2] KVM: MMU: fix relaxing permission

Quoted 7 lines · Avi Kivity
Quoted 5 lines

And, i think use 'spte.w=1, spte.u=0' to emulate 'guest cr0.wp=0 and gpte.w=0' is not a good way since it can completely stop user process access, but in this case, user process is usually read and kernel lazily to write, just like vdso, it will generate a lots of #PF

As soon as the guest kernel stops writing we switch back to gpte.w=gpte.u=1 and the guest can access it completely. For the case where both the kernel and userspace use interleaved access, you are right, but I don't see a better way, do you?

To expand, we only set spte.w=1 on write faults. So if the guest only reads the page, we'll instantiate an spte with u=1 and w=0.

Signature · 1 line
error compiling committee.c: too many arguments to function
#6 lore raw
In reply to Xiao Guangrong
Quoted 7 lines · Xiao Guangrong

fix:

arch/x86/kvm/svm.c: In function ‘is_erratum_383’:
arch/x86/kvm/svm.c:1459: warning: integer constant is too large for ‘long’ type arch/x86/kvm/svm.c: In function ‘svm_handle_mce’:
arch/x86/kvm/svm.c:1490: warning: unused variable ‘kvm_run’

Applied (partially, got the other part from Jan already), thanks.

Signature · 1 line
error compiling committee.c: too many arguments to function