| Message ID | 20221209044557.1496580-9-robert.hu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On 12/9/2022 12:45 PM, Robert Hoo wrote: > When only changes LAM bits, ask next vcpu run to load mmu pgd, so that it > will build new CR3 with LAM bits updates. No TLB flush needed on this case. > When changes on effective addresses, no matter LAM bits changes or not, go > through normal pgd update process. > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 48a2ad1e4cd6..6fbe8dd36b1e 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1248,9 +1248,9 @@ static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > { > bool skip_tlb_flush = false; > - unsigned long pcid = 0; > + unsigned long pcid = 0, old_cr3; > #ifdef CONFIG_X86_64 > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > if (pcid_enabled) { > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > @@ -1263,6 +1263,10 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) > goto handle_tlb_flush; > > + if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) && > + (cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57))) > + return 1; > + > /* > * Do not condition the GPA check on long mode, this helper is used to > * stuff CR3, e.g. for RSM emulation, and there is no guarantee that > @@ -1274,8 +1278,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) > return 1; > > - if (cr3 != kvm_read_cr3(vcpu)) > - kvm_mmu_new_pgd(vcpu, cr3); > + old_cr3 = kvm_read_cr3(vcpu); > + if (cr3 != old_cr3) { > + if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) { > + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 | > + X86_CR3_LAM_U57)); "CR3_ADDR_MASK" should not contain "X86_CR3_LAM_U48 | X86_CR3_LAM_U57" But seems it is not defined explicitly. Besides this, looks good for me. Reviewed-by: Jingqi Liu<jingqi.liu@intel.com> > + } else { > + /* > + * Though effective addr no change, mark the > + * request so that LAM bits will take effect > + * when enter guest. > + */ > + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); > + } > + } > > vcpu->arch.cr3 = cr3; > kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
On Tue, 2022-12-20 at 17:10 +0800, Liu, Jingqi wrote: > On 12/9/2022 12:45 PM, Robert Hoo wrote: > > When only changes LAM bits, ask next vcpu run to load mmu pgd, so > > that it > > will build new CR3 with LAM bits updates. No TLB flush needed on > > this case. > > When changes on effective addresses, no matter LAM bits changes or > > not, go > > through normal pgd update process. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 48a2ad1e4cd6..6fbe8dd36b1e 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1248,9 +1248,9 @@ static bool kvm_is_valid_cr3(struct kvm_vcpu > > *vcpu, unsigned long cr3) > > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > { > > bool skip_tlb_flush = false; > > - unsigned long pcid = 0; > > + unsigned long pcid = 0, old_cr3; > > #ifdef CONFIG_X86_64 > > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > > > if (pcid_enabled) { > > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > > @@ -1263,6 +1263,10 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, > > unsigned long cr3) > > if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) > > goto handle_tlb_flush; > > > > + if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) && > > + (cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57))) > > + return 1; > > + > > /* > > * Do not condition the GPA check on long mode, this helper is > > used to > > * stuff CR3, e.g. for RSM emulation, and there is no guarantee > > that > > @@ -1274,8 +1278,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, > > unsigned long cr3) > > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) > > return 1; > > > > - if (cr3 != kvm_read_cr3(vcpu)) > > - kvm_mmu_new_pgd(vcpu, cr3); > > + old_cr3 = kvm_read_cr3(vcpu); > > + if (cr3 != old_cr3) { > > + if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) { > > + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 | > > + X86_CR3_LAM_U57)); > > "CR3_ADDR_MASK" should not contain "X86_CR3_LAM_U48 | > X86_CR3_LAM_U57" Yes, you're right. CR3_ADDR_MASK is the effective pgd address bits range. This hunk of code is: judge if changed bits falls in effective address, if true, need to load new pgd, no matter LAM bits changed or not. > But seems it is not defined explicitly. > Besides this, looks good for me. > Reviewed-by: Jingqi Liu<jingqi.liu@intel.com> > > + } else { > > + /* > > + * Though effective addr no change, mark the > > + * request so that LAM bits will take effect > > + * when enter guest. > > + */ > > + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); > > + } > > + } > > > > vcpu->arch.cr3 = cr3; > > kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
On Fri, Dec 09, 2022 at 12:45:56PM +0800, Robert Hoo wrote: > When only changes LAM bits, ask next vcpu run to load mmu pgd, so that it > will build new CR3 with LAM bits updates. No TLB flush needed on this case. > When changes on effective addresses, no matter LAM bits changes or not, go > through normal pgd update process. Sorry, may I ask why this is related to effective address changes? This patch is only about the CR3 updates... > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > --- > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++---- > 1 file changed, 20 insertions(+), 4 deletions(-) > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 48a2ad1e4cd6..6fbe8dd36b1e 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1248,9 +1248,9 @@ static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > { > bool skip_tlb_flush = false; > - unsigned long pcid = 0; > + unsigned long pcid = 0, old_cr3; > #ifdef CONFIG_X86_64 > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); This may qualify a seperate patch. :) > > if (pcid_enabled) { > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > @@ -1263,6 +1263,10 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) > goto handle_tlb_flush; > > + if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) && > + (cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57))) > + return 1; > + > /* > * Do not condition the GPA check on long mode, this helper is used to > * stuff CR3, e.g. for RSM emulation, and there is no guarantee that > @@ -1274,8 +1278,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) > return 1; > > - if (cr3 != kvm_read_cr3(vcpu)) > - kvm_mmu_new_pgd(vcpu, cr3); > + old_cr3 = kvm_read_cr3(vcpu); > + if (cr3 != old_cr3) { > + if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) { > + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 | > + X86_CR3_LAM_U57)); > + } else { > + /* > + * Though effective addr no change, mark the Same question here. > + * request so that LAM bits will take effect > + * when enter guest. > + */ > + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); > + } > + } > > vcpu->arch.cr3 = cr3; > kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3); > -- B.R. Yu
On Wed, 2022-12-21 at 16:30 +0800, Yu Zhang wrote: > On Fri, Dec 09, 2022 at 12:45:56PM +0800, Robert Hoo wrote: > > When only changes LAM bits, ask next vcpu run to load mmu pgd, so > > that it > > will build new CR3 with LAM bits updates. No TLB flush needed on > > this case. > > When changes on effective addresses, no matter LAM bits changes or > > not, go > > through normal pgd update process. > > Sorry, may I ask why this is related to effective address changes? > This patch is only about the CR3 updates... > Not sure if we mean the same thing. Here effective address I mean the CR3 & CR3_ADDR_MASK, i.e. pgd part. > > > > Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> > > --- > > arch/x86/kvm/x86.c | 24 ++++++++++++++++++++---- > > 1 file changed, 20 insertions(+), 4 deletions(-) > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 48a2ad1e4cd6..6fbe8dd36b1e 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1248,9 +1248,9 @@ static bool kvm_is_valid_cr3(struct kvm_vcpu > > *vcpu, unsigned long cr3) > > int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > { > > bool skip_tlb_flush = false; > > - unsigned long pcid = 0; > > + unsigned long pcid = 0, old_cr3; > > #ifdef CONFIG_X86_64 > > - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); > > This may qualify a seperate patch. :) I had thought of this as well, but it is so trivial, and literally we cannot say original code is wrong. > > > > > if (pcid_enabled) { > > skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; > > @@ -1263,6 +1263,10 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, > > unsigned long cr3) > > if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) > > goto handle_tlb_flush; > > > > + if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) && > > + (cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57))) > > + return 1; > > + > > /* > > * Do not condition the GPA check on long mode, this helper is > > used to > > * stuff CR3, e.g. for RSM emulation, and there is no guarantee > > that > > @@ -1274,8 +1278,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, > > unsigned long cr3) > > if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) > > return 1; > > > > - if (cr3 != kvm_read_cr3(vcpu)) > > - kvm_mmu_new_pgd(vcpu, cr3); > > + old_cr3 = kvm_read_cr3(vcpu); > > + if (cr3 != old_cr3) { > > + if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) { > > + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 | > > + X86_CR3_LAM_U57)); > > + } else { > > + /* > > + * Though effective addr no change, mark the > > Same question here. Here is the case the CR3 updates are only of LAM bits, no changes to PGD part. > > > + * request so that LAM bits will take effect > > + * when enter guest. > > + */ > > + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); > > + } > > + } > > > > vcpu->arch.cr3 = cr3; > > kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3); > > -- > > B.R. > Yu
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index 48a2ad1e4cd6..6fbe8dd36b1e 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1248,9 +1248,9 @@ static bool kvm_is_valid_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) { bool skip_tlb_flush = false; - unsigned long pcid = 0; + unsigned long pcid = 0, old_cr3; #ifdef CONFIG_X86_64 - bool pcid_enabled = kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); + bool pcid_enabled = !!kvm_read_cr4_bits(vcpu, X86_CR4_PCIDE); if (pcid_enabled) { skip_tlb_flush = cr3 & X86_CR3_PCID_NOFLUSH; @@ -1263,6 +1263,10 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) if (cr3 == kvm_read_cr3(vcpu) && !is_pae_paging(vcpu)) goto handle_tlb_flush; + if (!guest_cpuid_has(vcpu, X86_FEATURE_LAM) && + (cr3 & (X86_CR3_LAM_U48 | X86_CR3_LAM_U57))) + return 1; + /* * Do not condition the GPA check on long mode, this helper is used to * stuff CR3, e.g. for RSM emulation, and there is no guarantee that @@ -1274,8 +1278,20 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) return 1; - if (cr3 != kvm_read_cr3(vcpu)) - kvm_mmu_new_pgd(vcpu, cr3); + old_cr3 = kvm_read_cr3(vcpu); + if (cr3 != old_cr3) { + if ((cr3 ^ old_cr3) & CR3_ADDR_MASK) { + kvm_mmu_new_pgd(vcpu, cr3 & ~(X86_CR3_LAM_U48 | + X86_CR3_LAM_U57)); + } else { + /* + * Though effective addr no change, mark the + * request so that LAM bits will take effect + * when enter guest. + */ + kvm_make_request(KVM_REQ_LOAD_MMU_PGD, vcpu); + } + } vcpu->arch.cr3 = cr3; kvm_register_mark_dirty(vcpu, VCPU_EXREG_CR3);
When only changes LAM bits, ask next vcpu run to load mmu pgd, so that it will build new CR3 with LAM bits updates. No TLB flush needed on this case. When changes on effective addresses, no matter LAM bits changes or not, go through normal pgd update process. Signed-off-by: Robert Hoo <robert.hu@linux.intel.com> --- arch/x86/kvm/x86.c | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-)