Sean Christopherson <sean.j.christopherson@xxxxxxxxx> writes: > Don't attempt to load PDPTRs if EFER.LME=1, i.e. if 64-bit mode is > enabled. A recent change to reload the PDTPRs when CR0.CD or CR0.NW is > toggled botched the EFER.LME handling and sends KVM down the PDTPR path > when is_paging() is true, i.e. when the guest toggles CD/NW in 64-bit > mode. > > Split the CR0 checks for 64-bit vs. 32-bit PAE into separate paths. The > 64-bit path is specifically checking state when paging is toggled on, > i.e. CR0.PG transititions from 0->1. The PDPTR path now needs to run if > the new CR0 state has paging enabled, irrespective of whether paging was > already enabled. Trying to shave a few cycles to make the PDPTR path an > "else if" case is a mess. > > Fixes: d42e3fae6faed ("kvm: x86: Read PDPTEs on CR0.CD and CR0.NW changes") > Cc: Jim Mattson <jmattson@xxxxxxxxxx> > Cc: Oliver Upton <oupton@xxxxxxxxxx> > Cc: Peter Shier <pshier@xxxxxxxxxx> > Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx> > --- > > The other way to fix this, with a much smaller diff stat, is to simply > move the !is_page(vcpu) check inside (vcpu->arch.efer & EFER_LME). But > that results in a ridiculous amount of nested conditionals for what is a > very straightforward check e.g. > > if (cr0 & X86_CR0_PG) { > if (vcpu->arch.efer & EFER_LME) } > if (!is_paging(vcpu)) { > ... > } > } > } > > Since this doesn't need to be backported anywhere, I didn't see any value > in having an intermediate step. > > arch/x86/kvm/x86.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 95ef629228691..5f526d94c33f3 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -819,22 +819,22 @@ int kvm_set_cr0(struct kvm_vcpu *vcpu, unsigned long cr0) > if ((cr0 & X86_CR0_PG) && !(cr0 & X86_CR0_PE)) > return 1; > > - if (cr0 & X86_CR0_PG) { > #ifdef CONFIG_X86_64 > - if (!is_paging(vcpu) && (vcpu->arch.efer & EFER_LME)) { > - int cs_db, cs_l; > + if ((vcpu->arch.efer & EFER_LME) && !is_paging(vcpu) && > + (cr0 & X86_CR0_PG)) { it seems we have more than one occurance of "if (vcpu->arch.efer & EFER_LME)" under "#ifdef CONFIG_X86_64" and we alredy have static inline int is_long_mode(struct kvm_vcpu *vcpu) { #ifdef CONFIG_X86_64 return vcpu->arch.efer & EFER_LMA; #else return 0; #endif } so if we use this instead, the compilers will just throw away the non-reachable blocks when !(#ifdef CONFIG_X86_64), right? > + int cs_db, cs_l; > > - if (!is_pae(vcpu)) > - return 1; > - kvm_x86_ops.get_cs_db_l_bits(vcpu, &cs_db, &cs_l); > - if (cs_l) > - return 1; > - } else > -#endif > - if (is_pae(vcpu) && ((cr0 ^ old_cr0) & pdptr_bits) && > - !load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu))) > + if (!is_pae(vcpu)) > + return 1; > + kvm_x86_ops.get_cs_db_l_bits(vcpu, &cs_db, &cs_l); > + if (cs_l) > return 1; > } > +#endif > + if (!(vcpu->arch.efer & EFER_LME) && (cr0 & X86_CR0_PG) && > + is_pae(vcpu) && ((cr0 ^ old_cr0) & pdptr_bits) && > + !load_pdptrs(vcpu, vcpu->arch.walk_mmu, kvm_read_cr3(vcpu))) > + return 1; > > if (!(cr0 & X86_CR0_PG) && kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE)) > return 1; -- Vitaly