Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 04/06/2016 04:56 PM, Paolo Bonzini wrote: On 25/03/2016 14:19, Xiao Guangrong wrote: @@ -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)); One more tweak is needed in the line above; pfec - 1 must become pfec & ~1, because you've removed the pfec |= PFERR_PRESENT_MASK; line. Applied with this change. Yes, indeed. Thanks for your fix.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 04/06/2016 04:56 PM, Paolo Bonzini wrote: On 25/03/2016 14:19, Xiao Guangrong wrote: @@ -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)); One more tweak is needed in the line above; pfec - 1 must become pfec & ~1, because you've removed the pfec |= PFERR_PRESENT_MASK; line. Applied with this change. Yes, indeed. Thanks for your fix.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 25/03/2016 14:19, Xiao Guangrong wrote: > @@ -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)); One more tweak is needed in the line above; pfec - 1 must become pfec & ~1, because you've removed the pfec |= PFERR_PRESENT_MASK; line. Applied with this change. Paolo > > 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);
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 25/03/2016 14:19, Xiao Guangrong wrote: > @@ -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)); One more tweak is needed in the line above; pfec - 1 must become pfec & ~1, because you've removed the pfec |= PFERR_PRESENT_MASK; line. Applied with this change. Paolo > > 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);
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 06/04/2016 05:27, Xiao Guangrong wrote: > > I tested it and it is failed: > > test pte.p pte.user pde.p pde.user pde.a pde.pse pkru.wd pkey=1 user > write efer.nx cr4.pke: FAIL: error code 27 expected 7 > Dump mapping: address: 0x1234 > --L4: 2ebe007 > --L3: 2ebf007 > --L2: 80002a5 > > So PFEC.PKEY is set even if the ordinary check failed (caused by pde.rw > = 0), the kvm code is > right. :) Cool, thanks very much. I'll fix both QEMU and kvm-unit-tests. Paolo
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 06/04/2016 05:27, Xiao Guangrong wrote: > > I tested it and it is failed: > > test pte.p pte.user pde.p pde.user pde.a pde.pse pkru.wd pkey=1 user > write efer.nx cr4.pke: FAIL: error code 27 expected 7 > Dump mapping: address: 0x1234 > --L4: 2ebe007 > --L3: 2ebf007 > --L2: 80002a5 > > So PFEC.PKEY is set even if the ordinary check failed (caused by pde.rw > = 0), the kvm code is > right. :) Cool, thanks very much. I'll fix both QEMU and kvm-unit-tests. Paolo
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 03/30/2016 02:39 PM, Xiao Guangrong wrote: 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. I tested it and it is failed: test pte.p pte.user pde.p pde.user pde.a pde.pse pkru.wd pkey=1 user write efer.nx cr4.pke: FAIL: error code 27 expected 7 Dump mapping: address: 0x1234 --L4: 2ebe007 --L3: 2ebf007 --L2: 80002a5 So PFEC.PKEY is set even if the ordinary check failed (caused by pde.rw = 0), the kvm code is right. :)
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
On 03/30/2016 02:39 PM, Xiao Guangrong wrote: 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. I tested it and it is failed: test pte.p pte.user pde.p pde.user pde.a pde.pse pkru.wd pkey=1 user write efer.nx cr4.pke: FAIL: error code 27 expected 7 Dump mapping: address: 0x1234 --L4: 2ebe007 --L3: 2ebf007 --L2: 80002a5 So PFEC.PKEY is set even if the ordinary check failed (caused by pde.rw = 0), the kvm code is right. :)
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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. ;)
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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. ;)
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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.
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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: 0x1234
--L4: 3e95007
--L3: 3e96007
--L2: 283
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()...
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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: 0x1234
--L4: 3e95007
--L3: 3e96007
--L2: 283
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()...
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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: 0x1234
> --L4: 3e95007
> --L3: 3e96007
> --L2: 283
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;
> }
>
Re: [PATCH 1/4] KVM: MMU: fix permission_fault()
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: 0x1234
> --L4: 3e95007
> --L3: 3e96007
> --L2: 283
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;
> }
>
