| Message ID | 20190430173326.1956-1-vkuznets@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/kvm/mmu: reset MMU context when 32-bit guest switches PAE | expand |
On Tue, Apr 30, 2019 at 07:33:26PM +0200, Vitaly Kuznetsov wrote: > Commit 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it > to 'gpte_size'") introduced a regression: 32-bit PAE guests stopped "gpte_is_8_bytes" is really confusing in this case. :-( Unfortunately I can't think I can't think of a better name that isn't ridiculously verbose. > working. The issue appears to be: when guest switches (enables) PAE we need > to re-initialize MMU context (set context->root_level, do > reset_rsvds_bits_mask(), ...) but init_kvm_tdp_mmu() doesn't do that > because we threw away is_pae(vcpu) flag from mmu role. Restore it to > kvm_mmu_extended_role (as we now don't need it in base role) to fix > the issue. The change makes sense, but I'm amazed that there's a kernel that can actually trigger the bug. The extended role tracks CR0.PG, so I'm pretty sure hitting this bug requires toggling CR4.PAE *while paging is enabled*. Which is legal, but crazy. E.g. my 32-bit Linux VM runs fine with and without PAE enabled. > Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'") > Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> > --- > - RFC: it was proven multiple times that mmu code is more complex than it > appears (at least to me) :-) LOL, maybe you're just more optimistic than most people. Every time I look at the code I say something along the lines of "holy $&*#". > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/mmu.c | 1 + > 2 files changed, 2 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index a9d03af34030..c79abe7ca093 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -295,6 +295,7 @@ union kvm_mmu_extended_role { > unsigned int valid:1; > unsigned int execonly:1; > unsigned int cr0_pg:1; > + unsigned int cr4_pae:1; > unsigned int cr4_pse:1; > unsigned int cr4_pke:1; > unsigned int cr4_smap:1; > diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c > index e10962dfc203..d9c7b45d231f 100644 > --- a/arch/x86/kvm/mmu.c > +++ b/arch/x86/kvm/mmu.c > @@ -4781,6 +4781,7 @@ static union kvm_mmu_extended_role kvm_calc_mmu_role_ext(struct kvm_vcpu *vcpu) > union kvm_mmu_extended_role ext = {0}; > > ext.cr0_pg = !!is_paging(vcpu); > + ext.cr4_pae = !!is_pae(vcpu); This got me thinking, I wonder if we can/should leave the CR4 bits clear if !is_paging(). Technically I think we're unnecessarily purging the MMU when the guest is toggling CR4 bits with CR0.PG==0. And I think we can also git rid of the oddball nx flag in struct kvm_mmu. I'll play around with the code and hopefully send a patch or two. > ext.cr4_smep = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMEP); > ext.cr4_smap = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); > ext.cr4_pse = !!is_pse(vcpu); > -- > 2.20.1 >
Sean Christopherson <sean.j.christopherson@intel.com> writes: > On Tue, Apr 30, 2019 at 07:33:26PM +0200, Vitaly Kuznetsov wrote: >> Commit 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it >> to 'gpte_size'") introduced a regression: 32-bit PAE guests stopped > > "gpte_is_8_bytes" is really confusing in this case. :-( Unfortunately I > can't think I can't think of a better name that isn't ridiculously verbose. > >> working. The issue appears to be: when guest switches (enables) PAE we need >> to re-initialize MMU context (set context->root_level, do >> reset_rsvds_bits_mask(), ...) but init_kvm_tdp_mmu() doesn't do that >> because we threw away is_pae(vcpu) flag from mmu role. Restore it to >> kvm_mmu_extended_role (as we now don't need it in base role) to fix >> the issue. > > The change makes sense, but I'm amazed that there's a kernel that can > actually trigger the bug. The extended role tracks CR0.PG, so I'm pretty > sure hitting this bug requires toggling CR4.PAE *while paging is enabled*. > Which is legal, but crazy. E.g. my 32-bit Linux VM runs fine with and > without PAE enabled. Once upon a time the was an operating system called "RHEL5". Some people think it's long gone but you know, "no one's ever really gone" :-) > >> Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'") >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> > > Reviewed-by: Sean Christopherson <sean.j.christopherson@intel.com> Thanks! > >> --- >> - RFC: it was proven multiple times that mmu code is more complex than it >> appears (at least to me) :-) > > LOL, maybe you're just more optimistic than most people. Every time I > look at the code I say something along the lines of "holy $&*#". > :-) >> --- >> arch/x86/include/asm/kvm_host.h | 1 + >> arch/x86/kvm/mmu.c | 1 + >> 2 files changed, 2 insertions(+) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index a9d03af34030..c79abe7ca093 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -295,6 +295,7 @@ union kvm_mmu_extended_role { >> unsigned int valid:1; >> unsigned int execonly:1; >> unsigned int cr0_pg:1; >> + unsigned int cr4_pae:1; >> unsigned int cr4_pse:1; >> unsigned int cr4_pke:1; >> unsigned int cr4_smap:1; >> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c >> index e10962dfc203..d9c7b45d231f 100644 >> --- a/arch/x86/kvm/mmu.c >> +++ b/arch/x86/kvm/mmu.c >> @@ -4781,6 +4781,7 @@ static union kvm_mmu_extended_role kvm_calc_mmu_role_ext(struct kvm_vcpu *vcpu) >> union kvm_mmu_extended_role ext = {0}; >> >> ext.cr0_pg = !!is_paging(vcpu); >> + ext.cr4_pae = !!is_pae(vcpu); > > This got me thinking, I wonder if we can/should leave the CR4 bits clear > if !is_paging(). Technically I think we're unnecessarily purging the MMU > when the guest is toggling CR4 bits with CR0.PG==0. Yes, probably. Not sure I know any performance critical use-cases with CR0.PG==0 but who knows, maybe running hundreds of 8086 emulators IS a thing :-) > And I think we can > also git rid of the oddball nx flag in struct kvm_mmu. I'll play around > with the code and hopefully send a patch or two. > >> ext.cr4_smep = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMEP); >> ext.cr4_smap = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); >> ext.cr4_pse = !!is_pse(vcpu); >> -- >> 2.20.1 >>
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index a9d03af34030..c79abe7ca093 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -295,6 +295,7 @@ union kvm_mmu_extended_role { unsigned int valid:1; unsigned int execonly:1; unsigned int cr0_pg:1; + unsigned int cr4_pae:1; unsigned int cr4_pse:1; unsigned int cr4_pke:1; unsigned int cr4_smap:1; diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c index e10962dfc203..d9c7b45d231f 100644 --- a/arch/x86/kvm/mmu.c +++ b/arch/x86/kvm/mmu.c @@ -4781,6 +4781,7 @@ static union kvm_mmu_extended_role kvm_calc_mmu_role_ext(struct kvm_vcpu *vcpu) union kvm_mmu_extended_role ext = {0}; ext.cr0_pg = !!is_paging(vcpu); + ext.cr4_pae = !!is_pae(vcpu); ext.cr4_smep = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMEP); ext.cr4_smap = !!kvm_read_cr4_bits(vcpu, X86_CR4_SMAP); ext.cr4_pse = !!is_pse(vcpu);
Commit 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'") introduced a regression: 32-bit PAE guests stopped working. The issue appears to be: when guest switches (enables) PAE we need to re-initialize MMU context (set context->root_level, do reset_rsvds_bits_mask(), ...) but init_kvm_tdp_mmu() doesn't do that because we threw away is_pae(vcpu) flag from mmu role. Restore it to kvm_mmu_extended_role (as we now don't need it in base role) to fix the issue. Fixes: 47c42e6b4192 ("KVM: x86: fix handling of role.cr4_pae and rename it to 'gpte_size'") Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com> --- - RFC: it was proven multiple times that mmu code is more complex than it appears (at least to me) :-) --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/mmu.c | 1 + 2 files changed, 2 insertions(+)