| Message ID | 20230719144131.29052-3-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Wed, Jul 19, 2023 at 10:41:24PM +0800, Binbin Wu <binbin.wu@linux.intel.com> wrote: > Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide > a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() > can be adjusted according to new feature(s). > > No functional change intended. > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > arch/x86/kvm/cpuid.h | 5 +++++ > arch/x86/kvm/svm/nested.c | 4 ++-- > arch/x86/kvm/vmx/nested.c | 4 ++-- > arch/x86/kvm/x86.c | 4 ++-- > 4 files changed, 11 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > index f61a2106ba90..8b26d946f3e3 100644 > --- a/arch/x86/kvm/cpuid.h > +++ b/arch/x86/kvm/cpuid.h > @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, > return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > } > > +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > +{ > + return kvm_vcpu_is_legal_gpa(vcpu, cr3); > +} > + The remaining user of kvm_vcpu_is_illegal_gpa() is one left. Can we remove it by replacing !kvm_vcpu_is_legal_gpa()?
On 7/21/2023 7:53 AM, Isaku Yamahata wrote: > On Wed, Jul 19, 2023 at 10:41:24PM +0800, > Binbin Wu <binbin.wu@linux.intel.com> wrote: > >> Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide >> a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() >> can be adjusted according to new feature(s). >> >> No functional change intended. >> >> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> arch/x86/kvm/cpuid.h | 5 +++++ >> arch/x86/kvm/svm/nested.c | 4 ++-- >> arch/x86/kvm/vmx/nested.c | 4 ++-- >> arch/x86/kvm/x86.c | 4 ++-- >> 4 files changed, 11 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h >> index f61a2106ba90..8b26d946f3e3 100644 >> --- a/arch/x86/kvm/cpuid.h >> +++ b/arch/x86/kvm/cpuid.h >> @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, >> return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); >> } >> >> +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) >> +{ >> + return kvm_vcpu_is_legal_gpa(vcpu, cr3); >> +} >> + > The remaining user of kvm_vcpu_is_illegal_gpa() is one left. Can we remove it > by replacing !kvm_vcpu_is_legal_gpa()? There are still two callsites of kvm_vcpu_is_illegal_gpa() left (basing on Linux 6.5-rc2), in handle_ept_violation() and nested_vmx_check_eptp(). But they could be replaced by !kvm_vcpu_is_legal_gpa() and then remove kvm_vcpu_is_illegal_gpa(). I am neutral to this.
On Fri, Jul 21, 2023, Binbin Wu wrote: > > > On 7/21/2023 7:53 AM, Isaku Yamahata wrote: > > On Wed, Jul 19, 2023 at 10:41:24PM +0800, > > Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > > > Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide > > > a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() > > > can be adjusted according to new feature(s). > > > > > > No functional change intended. > > > > > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > > > --- > > > arch/x86/kvm/cpuid.h | 5 +++++ > > > arch/x86/kvm/svm/nested.c | 4 ++-- > > > arch/x86/kvm/vmx/nested.c | 4 ++-- > > > arch/x86/kvm/x86.c | 4 ++-- > > > 4 files changed, 11 insertions(+), 6 deletions(-) > > > > > > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > > > index f61a2106ba90..8b26d946f3e3 100644 > > > --- a/arch/x86/kvm/cpuid.h > > > +++ b/arch/x86/kvm/cpuid.h > > > @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, > > > return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > > > } > > > +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > > +{ > > > + return kvm_vcpu_is_legal_gpa(vcpu, cr3); > > > +} > > > + > > The remaining user of kvm_vcpu_is_illegal_gpa() is one left. Can we remove it > > by replacing !kvm_vcpu_is_legal_gpa()? > > There are still two callsites of kvm_vcpu_is_illegal_gpa() left (basing on > Linux 6.5-rc2), in handle_ept_violation() and nested_vmx_check_eptp(). > But they could be replaced by !kvm_vcpu_is_legal_gpa() and then remove > kvm_vcpu_is_illegal_gpa(). > I am neutral to this. I'm largely neutral on this as well, though I do like the idea of having only "legal" APIs. I think it makes sense to throw together a patch, we can always ignore the patch if end we up deciding to keep kvm_vcpu_is_illegal_gpa().
On 7/21/2023 11:03 PM, Sean Christopherson wrote: > On Fri, Jul 21, 2023, Binbin Wu wrote: >> >> On 7/21/2023 7:53 AM, Isaku Yamahata wrote: >>> On Wed, Jul 19, 2023 at 10:41:24PM +0800, >>> Binbin Wu <binbin.wu@linux.intel.com> wrote: >>> >>>> Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide >>>> a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() >>>> can be adjusted according to new feature(s). >>>> >>>> No functional change intended. >>>> >>>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >>>> --- >>>> arch/x86/kvm/cpuid.h | 5 +++++ >>>> arch/x86/kvm/svm/nested.c | 4 ++-- >>>> arch/x86/kvm/vmx/nested.c | 4 ++-- >>>> arch/x86/kvm/x86.c | 4 ++-- >>>> 4 files changed, 11 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h >>>> index f61a2106ba90..8b26d946f3e3 100644 >>>> --- a/arch/x86/kvm/cpuid.h >>>> +++ b/arch/x86/kvm/cpuid.h >>>> @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, >>>> return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); >>>> } >>>> +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) >>>> +{ >>>> + return kvm_vcpu_is_legal_gpa(vcpu, cr3); >>>> +} >>>> + >>> The remaining user of kvm_vcpu_is_illegal_gpa() is one left. Can we remove it >>> by replacing !kvm_vcpu_is_legal_gpa()? >> There are still two callsites of kvm_vcpu_is_illegal_gpa() left (basing on >> Linux 6.5-rc2), in handle_ept_violation() and nested_vmx_check_eptp(). >> But they could be replaced by !kvm_vcpu_is_legal_gpa() and then remove >> kvm_vcpu_is_illegal_gpa(). >> I am neutral to this. > I'm largely neutral on this as well, though I do like the idea of having only > "legal" APIs. I think it makes sense to throw together a patch, we can always > ignore the patch if end we up deciding to keep kvm_vcpu_is_illegal_gpa(). OK. Thanks for the advice. Should I send a seperate patch or add a patch to remove kvm_vcpu_is_illegal_gpa() in next version?
On Mon, Jul 24, 2023, Binbin Wu wrote: > > > On 7/21/2023 11:03 PM, Sean Christopherson wrote: > > On Fri, Jul 21, 2023, Binbin Wu wrote: > > > > > > On 7/21/2023 7:53 AM, Isaku Yamahata wrote: > > > > On Wed, Jul 19, 2023 at 10:41:24PM +0800, > > > > Binbin Wu <binbin.wu@linux.intel.com> wrote: > > > > > > > > > Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide > > > > > a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() > > > > > can be adjusted according to new feature(s). > > > > > > > > > > No functional change intended. > > > > > > > > > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > > > > > --- > > > > > arch/x86/kvm/cpuid.h | 5 +++++ > > > > > arch/x86/kvm/svm/nested.c | 4 ++-- > > > > > arch/x86/kvm/vmx/nested.c | 4 ++-- > > > > > arch/x86/kvm/x86.c | 4 ++-- > > > > > 4 files changed, 11 insertions(+), 6 deletions(-) > > > > > > > > > > diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h > > > > > index f61a2106ba90..8b26d946f3e3 100644 > > > > > --- a/arch/x86/kvm/cpuid.h > > > > > +++ b/arch/x86/kvm/cpuid.h > > > > > @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, > > > > > return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); > > > > > } > > > > > +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) > > > > > +{ > > > > > + return kvm_vcpu_is_legal_gpa(vcpu, cr3); > > > > > +} > > > > > + > > > > The remaining user of kvm_vcpu_is_illegal_gpa() is one left. Can we remove it > > > > by replacing !kvm_vcpu_is_legal_gpa()? > > > There are still two callsites of kvm_vcpu_is_illegal_gpa() left (basing on > > > Linux 6.5-rc2), in handle_ept_violation() and nested_vmx_check_eptp(). > > > But they could be replaced by !kvm_vcpu_is_legal_gpa() and then remove > > > kvm_vcpu_is_illegal_gpa(). > > > I am neutral to this. > > I'm largely neutral on this as well, though I do like the idea of having only > > "legal" APIs. I think it makes sense to throw together a patch, we can always > > ignore the patch if end we up deciding to keep kvm_vcpu_is_illegal_gpa(). > OK. Thanks for the advice. > Should I send a seperate patch or add a patch to remove > kvm_vcpu_is_illegal_gpa() in next version? Add a patch in the next version, eliminating kvm_vcpu_is_illegal_gpa() without the context of this series probably isn't worth the churn.
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h index f61a2106ba90..8b26d946f3e3 100644 --- a/arch/x86/kvm/cpuid.h +++ b/arch/x86/kvm/cpuid.h @@ -283,4 +283,9 @@ static __always_inline bool guest_can_use(struct kvm_vcpu *vcpu, return vcpu->arch.governed_features.enabled & kvm_governed_feature_bit(x86_feature); } +static inline bool kvm_vcpu_is_legal_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) +{ + return kvm_vcpu_is_legal_gpa(vcpu, cr3); +} + #endif diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c index 96936ddf1b3c..1df801a48451 100644 --- a/arch/x86/kvm/svm/nested.c +++ b/arch/x86/kvm/svm/nested.c @@ -311,7 +311,7 @@ static bool __nested_vmcb_check_save(struct kvm_vcpu *vcpu, if ((save->efer & EFER_LME) && (save->cr0 & X86_CR0_PG)) { if (CC(!(save->cr4 & X86_CR4_PAE)) || CC(!(save->cr0 & X86_CR0_PE)) || - CC(kvm_vcpu_is_illegal_gpa(vcpu, save->cr3))) + CC(!kvm_vcpu_is_legal_cr3(vcpu, save->cr3))) return false; } @@ -520,7 +520,7 @@ static void nested_svm_transition_tlb_flush(struct kvm_vcpu *vcpu) static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool nested_npt, bool reload_pdptrs) { - if (CC(kvm_vcpu_is_illegal_gpa(vcpu, cr3))) + if (CC(!kvm_vcpu_is_legal_cr3(vcpu, cr3))) return -EINVAL; if (reload_pdptrs && !nested_npt && is_pae_paging(vcpu) && diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c index 516391cc0d64..76c9904c6625 100644 --- a/arch/x86/kvm/vmx/nested.c +++ b/arch/x86/kvm/vmx/nested.c @@ -1085,7 +1085,7 @@ static int nested_vmx_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3, bool nested_ept, bool reload_pdptrs, enum vm_entry_failure_code *entry_failure_code) { - if (CC(kvm_vcpu_is_illegal_gpa(vcpu, cr3))) { + if (CC(!kvm_vcpu_is_legal_cr3(vcpu, cr3))) { *entry_failure_code = ENTRY_FAIL_DEFAULT; return -EINVAL; } @@ -2912,7 +2912,7 @@ static int nested_vmx_check_host_state(struct kvm_vcpu *vcpu, if (CC(!nested_host_cr0_valid(vcpu, vmcs12->host_cr0)) || CC(!nested_host_cr4_valid(vcpu, vmcs12->host_cr4)) || - CC(kvm_vcpu_is_illegal_gpa(vcpu, vmcs12->host_cr3))) + CC(!kvm_vcpu_is_legal_cr3(vcpu, vmcs12->host_cr3))) return -EINVAL; if (CC(is_noncanonical_address(vmcs12->host_ia32_sysenter_esp, vcpu)) || diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index a6b9bea62fb8..6a6d08238e8d 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1271,7 +1271,7 @@ int kvm_set_cr3(struct kvm_vcpu *vcpu, unsigned long cr3) * stuff CR3, e.g. for RSM emulation, and there is no guarantee that * the current vCPU mode is accurate. */ - if (kvm_vcpu_is_illegal_gpa(vcpu, cr3)) + if (!kvm_vcpu_is_legal_cr3(vcpu, cr3)) return 1; if (is_pae_paging(vcpu) && !load_pdptrs(vcpu, cr3)) @@ -11449,7 +11449,7 @@ static bool kvm_is_valid_sregs(struct kvm_vcpu *vcpu, struct kvm_sregs *sregs) */ if (!(sregs->cr4 & X86_CR4_PAE) || !(sregs->efer & EFER_LMA)) return false; - if (kvm_vcpu_is_illegal_gpa(vcpu, sregs->cr3)) + if (!kvm_vcpu_is_legal_cr3(vcpu, sregs->cr3)) return false; } else { /*
Add and use kvm_vcpu_is_legal_cr3() to check CR3's legality to provide a clear distinction b/t CR3 and GPA checks. So that kvm_vcpu_is_legal_cr3() can be adjusted according to new feature(s). No functional change intended. Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> --- arch/x86/kvm/cpuid.h | 5 +++++ arch/x86/kvm/svm/nested.c | 4 ++-- arch/x86/kvm/vmx/nested.c | 4 ++-- arch/x86/kvm/x86.c | 4 ++-- 4 files changed, 11 insertions(+), 6 deletions(-)