| Message ID | 20180809004524.129883-1-junaids@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: mmu: Don't read PDPTEs when paging is not enabled | expand |
On Wed, Aug 08, 2018 at 05:45:24PM -0700, Junaid Shahid wrote: > kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and > CR4.PAE = 1. > > Signed-off-by: Junaid Shahid <junaids@google.com> > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3c83711c0ebe..a726af7d31b6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -627,7 +627,7 @@ bool pdptrs_changed(struct kvm_vcpu *vcpu) > gfn_t gfn; > int r; > > - if (is_long_mode(vcpu) || !is_pae(vcpu)) > + if (is_long_mode(vcpu) || !is_pae(vcpu) || !is_paging(vcpu)) > return false; > > if (!test_bit(VCPU_EXREG_PDPTR, > @@ -8123,7 +8123,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > kvm_update_cpuid(vcpu); > > idx = srcu_read_lock(&vcpu->kvm->srcu); > - if (!is_long_mode(vcpu) && is_pae(vcpu)) { > + if (!is_long_mode(vcpu) && is_pae(vcpu) && is_paging(vcpu)) { > load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu)); > mmu_reset_needed = 1; > } > -- > 2.18.0.345.g5c9ce644c3-goog >
On 09/08/2018 02:45, Junaid Shahid wrote: > kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and > CR4.PAE = 1. > > Signed-off-by: Junaid Shahid <junaids@google.com> Do you have a testcase? Thanks, Paolo > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3c83711c0ebe..a726af7d31b6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -627,7 +627,7 @@ bool pdptrs_changed(struct kvm_vcpu *vcpu) > gfn_t gfn; > int r; > > - if (is_long_mode(vcpu) || !is_pae(vcpu)) > + if (is_long_mode(vcpu) || !is_pae(vcpu) || !is_paging(vcpu)) > return false; > > if (!test_bit(VCPU_EXREG_PDPTR, > @@ -8123,7 +8123,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > kvm_update_cpuid(vcpu); > > idx = srcu_read_lock(&vcpu->kvm->srcu); > - if (!is_long_mode(vcpu) && is_pae(vcpu)) { > + if (!is_long_mode(vcpu) && is_pae(vcpu) && is_paging(vcpu)) { > load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu)); > mmu_reset_needed = 1; > } >
On 08/09/2018 09:58 AM, Paolo Bonzini wrote: > On 09/08/2018 02:45, Junaid Shahid wrote: >> kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and >> CR4.PAE = 1. >> >> Signed-off-by: Junaid Shahid <junaids@google.com> > > Do you have a testcase? > I could write a test case to trigger this condition, but we would need to add some type of trace statement to this path so that the test can easily detect whether or not the extraneous reads have occurred. Thanks, Junaid
On 10/08/2018 05:01, Junaid Shahid wrote: > On 08/09/2018 09:58 AM, Paolo Bonzini wrote: >> On 09/08/2018 02:45, Junaid Shahid wrote: >>> kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and >>> CR4.PAE = 1. >>> >>> Signed-off-by: Junaid Shahid <junaids@google.com> >> Do you have a testcase? >> > I could write a test case to trigger this condition, but we would > need to add some type of trace statement to this path so that the > test can easily detect whether or not the extraneous reads have > occurred. Perhaps you can write a standalone test (not using kvm-unit-tests) you can place the PDPTEs in a place where the read will cause a vmentry failure? Paolo
On 08/10/2018 12:50 AM, Paolo Bonzini wrote: > > Perhaps you can write a standalone test (not using kvm-unit-tests) you > can place the PDPTEs in a place where the read will cause a vmentry failure? It wouldn't cause a vmentry failure because the paths that actually load the PDPTEs into the VMCS already have the is_paging() check. These two paths are just about kvm itself reading them unnecessarily (and then doing further work when it thinks that the PDPTEs have changed). Thanks, Junaid
On 10/08/2018 23:13, Junaid Shahid wrote: >> Perhaps you can write a standalone test (not using kvm-unit-tests) >> you can place the PDPTEs in a place where the read will cause a >> vmentry failure? > > It wouldn't cause a vmentry failure because the paths that actually > load the PDPTEs into the VMCS already have the is_paging() check. > These two paths are just about kvm itself reading them unnecessarily > (and then doing further work when it thinks that the PDPTEs have > changed). Then the patch is okay, thanks for the clarification! Paolo
On 09/08/2018 02:45, Junaid Shahid wrote: > kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and > CR4.PAE = 1. > > Signed-off-by: Junaid Shahid <junaids@google.com> > --- > arch/x86/kvm/x86.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 3c83711c0ebe..a726af7d31b6 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -627,7 +627,7 @@ bool pdptrs_changed(struct kvm_vcpu *vcpu) > gfn_t gfn; > int r; > > - if (is_long_mode(vcpu) || !is_pae(vcpu)) > + if (is_long_mode(vcpu) || !is_pae(vcpu) || !is_paging(vcpu)) > return false; > > if (!test_bit(VCPU_EXREG_PDPTR, > @@ -8123,7 +8123,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) > kvm_update_cpuid(vcpu); > > idx = srcu_read_lock(&vcpu->kvm->srcu); > - if (!is_long_mode(vcpu) && is_pae(vcpu)) { > + if (!is_long_mode(vcpu) && is_pae(vcpu) && is_paging(vcpu)) { > load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu)); > mmu_reset_needed = 1; > } > Queued, thanks. Paolo
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 3c83711c0ebe..a726af7d31b6 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -627,7 +627,7 @@ bool pdptrs_changed(struct kvm_vcpu *vcpu) gfn_t gfn; int r; - if (is_long_mode(vcpu) || !is_pae(vcpu)) + if (is_long_mode(vcpu) || !is_pae(vcpu) || !is_paging(vcpu)) return false; if (!test_bit(VCPU_EXREG_PDPTR, @@ -8123,7 +8123,7 @@ static int __set_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) kvm_update_cpuid(vcpu); idx = srcu_read_lock(&vcpu->kvm->srcu); - if (!is_long_mode(vcpu) && is_pae(vcpu)) { + if (!is_long_mode(vcpu) && is_pae(vcpu) && is_paging(vcpu)) { load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu)); mmu_reset_needed = 1; }
kvm should not attempt to read guest PDPTEs when CR0.PG = 0 and CR4.PAE = 1. Signed-off-by: Junaid Shahid <junaids@google.com> --- arch/x86/kvm/x86.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)