[PATCH 1/2] KVM: SVM: fix compiling warning

[PATCH 1/2] KVM: SVM: fix compiling warning

[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’

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

diff --git a/arch/x86/kvm/svm.c b/arch/x86/kvm/svm.c
index 65324a0..02ea5cf 100644
--- a/arch/x86/kvm/svm.c
+++ b/arch/x86/kvm/svm.c
@@ -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);
-- 
1.6.1.2

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

[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.

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

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index bfd8382..ccbc98b 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -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)) {
-- 
1.6.1.2

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

[Avi Kivity]

On 05/26/2010 05:46 AM, Xiao Guangrong wrote:
> 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.
>
> @@ -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.

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

Re: [PATCH 1/2] KVM: SVM: fix compiling warning

[Avi Kivity]

On 05/26/2010 05:44 AM, Xiao Guangrong wrote:
> 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.

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

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

[Xiao Guangrong]

Avi Kivity wrote:
> On 05/26/2010 05:46 AM, Xiao Guangrong wrote:
>> 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.
>>
>> @@ -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.

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

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

[Avi Kivity]

On 05/27/2010 02:00 PM, Xiao Guangrong wrote:
>
>> 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.
>>      
> 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:

> -       } 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;
> -               }

:(

> 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?


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

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

[Avi Kivity]

On 05/27/2010 02:18 PM, Avi Kivity wrote:
>
>> 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.

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