| Message ID | 1458911978-19430-1-git-send-email-guangrong.xiao@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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: 0x123400000000 > ------L4: 3e95007 > ------L3: 3e96007 > ------L2: 2000083 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; > } > -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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: 0x123400000000 >> ------L4: 3e95007 >> ------L3: 3e96007 >> ------L2: 2000083 > > 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()... -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; 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; }
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: 0x123400000000 ------L4: 3e95007 ------L3: 3e96007 ------L2: 2000083 It's caused by the reason that PFEC returned to guest is copied from the PFEC triggered by shadow page table This patch fixes it and makes the logic of updating errcode more clean Signed-off-by: Xiao Guangrong <guangrong.xiao@linux.intel.com> --- arch/x86/kvm/mmu.h | 8 ++++---- arch/x86/kvm/paging_tmpl.h | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-)