| Message ID | 20230719144131.29052-4-binbin.wu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Linear Address Masking (LAM) KVM Enabling | expand |
On Wed, 2023-07-19 at 22:41 +0800, Binbin Wu wrote: > Use the governed feature framework to track if Linear Address Masking (LAM) > is "enabled", i.e. if LAM can be used by the guest. So that guest_can_use() > can be used to support LAM virtualization. Better to explain why to use governed feature for LAM? Is it because there's hot path(s) calling guest_cpuid_has()? Anyway some context of why can help here. > > LAM modifies the checking that is applied to 64-bit linear addresses, allowing > software to use of the untranslated address bits for metadata and masks the > metadata bits before using them as linear addresses to access memory. > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> > --- > arch/x86/kvm/governed_features.h | 2 ++ > arch/x86/kvm/vmx/vmx.c | 3 +++ > 2 files changed, 5 insertions(+) > > diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h > index 40ce8e6608cd..708578d60e6f 100644 > --- a/arch/x86/kvm/governed_features.h > +++ b/arch/x86/kvm/governed_features.h > @@ -5,5 +5,7 @@ BUILD_BUG() > > #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) > > +KVM_GOVERNED_X86_FEATURE(LAM) > + > #undef KVM_GOVERNED_X86_FEATURE > #undef KVM_GOVERNED_FEATURE > diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c > index 0ecf4be2c6af..ae47303c88d7 100644 > --- a/arch/x86/kvm/vmx/vmx.c > +++ b/arch/x86/kvm/vmx/vmx.c > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > vmx->msr_ia32_feature_control_valid_bits &= > ~FEAT_CTL_SGX_LC_ENABLED; > > + if (boot_cpu_has(X86_FEATURE_LAM)) > + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); > + If you want to use boot_cpu_has(), it's better to be done at your last patch to only set the cap bit when boot_cpu_has() is true, I suppose. > /* Refresh #PF interception to account for MAXPHYADDR changes. */ > vmx_update_exception_bitmap(vcpu); > }
On 8/16/2023 11:46 AM, Huang, Kai wrote: > On Wed, 2023-07-19 at 22:41 +0800, Binbin Wu wrote: >> Use the governed feature framework to track if Linear Address Masking (LAM) >> is "enabled", i.e. if LAM can be used by the guest. So that guest_can_use() >> can be used to support LAM virtualization. > Better to explain why to use governed feature for LAM? Is it because there's > hot path(s) calling guest_cpuid_has()? Anyway some context of why can help > here. Yes, to avoid calling guest_cpuid_has() in CR3 handling and instruction emulation paths. I will add the context next version. Thanks! > >> LAM modifies the checking that is applied to 64-bit linear addresses, allowing >> software to use of the untranslated address bits for metadata and masks the >> metadata bits before using them as linear addresses to access memory. >> >> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> >> --- >> arch/x86/kvm/governed_features.h | 2 ++ >> arch/x86/kvm/vmx/vmx.c | 3 +++ >> 2 files changed, 5 insertions(+) >> >> diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h >> index 40ce8e6608cd..708578d60e6f 100644 >> --- a/arch/x86/kvm/governed_features.h >> +++ b/arch/x86/kvm/governed_features.h >> @@ -5,5 +5,7 @@ BUILD_BUG() >> >> #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) >> >> +KVM_GOVERNED_X86_FEATURE(LAM) >> + >> #undef KVM_GOVERNED_X86_FEATURE >> #undef KVM_GOVERNED_FEATURE >> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c >> index 0ecf4be2c6af..ae47303c88d7 100644 >> --- a/arch/x86/kvm/vmx/vmx.c >> +++ b/arch/x86/kvm/vmx/vmx.c >> @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) >> vmx->msr_ia32_feature_control_valid_bits &= >> ~FEAT_CTL_SGX_LC_ENABLED; >> >> + if (boot_cpu_has(X86_FEATURE_LAM)) >> + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); >> + > If you want to use boot_cpu_has(), it's better to be done at your last patch to > only set the cap bit when boot_cpu_has() is true, I suppose. Yes, but new version of kvm_governed_feature_check_and_set() of KVM-governed feature framework will check against kvm_cpu_cap_has() as well. I will remove the if statement and call kvm_governed_feature_check_and_set() directly. https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ > >> /* Refresh #PF interception to account for MAXPHYADDR changes. */ >> vmx_update_exception_bitmap(vcpu); >> }
> > > --- a/arch/x86/kvm/vmx/vmx.c > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > vmx->msr_ia32_feature_control_valid_bits &= > > > ~FEAT_CTL_SGX_LC_ENABLED; > > > > > > + if (boot_cpu_has(X86_FEATURE_LAM)) > > > + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); > > > + > > If you want to use boot_cpu_has(), it's better to be done at your last patch to > > only set the cap bit when boot_cpu_has() is true, I suppose. > Yes, but new version of kvm_governed_feature_check_and_set() of > KVM-governed feature framework will check against kvm_cpu_cap_has() as well. > I will remove the if statement and call > kvm_governed_feature_check_and_set() directly. > https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ > I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you are using boot_cpu_has(). They are not the same. If LAM should be only supported when boot_cpu_has() is true then it seems you can just only set the LAM cap bit when boot_cpu_has() is true. As you also mentioned above the kvm_governed_feature_check_and_set() here internally does kvm_cpu_cap_has().
On Wed, Aug 16, 2023, Kai Huang wrote: > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > vmx->msr_ia32_feature_control_valid_bits &= > > > > ~FEAT_CTL_SGX_LC_ENABLED; > > > > > > > > + if (boot_cpu_has(X86_FEATURE_LAM)) > > > > + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); > > > > + > > > If you want to use boot_cpu_has(), it's better to be done at your last patch to > > > only set the cap bit when boot_cpu_has() is true, I suppose. > > Yes, but new version of kvm_governed_feature_check_and_set() of > > KVM-governed feature framework will check against kvm_cpu_cap_has() as well. > > I will remove the if statement and call > > kvm_governed_feature_check_and_set() directly. > > https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ > > > > I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you > are using boot_cpu_has(). They are not the same. > > If LAM should be only supported when boot_cpu_has() is true then it seems you > can just only set the LAM cap bit when boot_cpu_has() is true. As you also > mentioned above the kvm_governed_feature_check_and_set() here internally does > kvm_cpu_cap_has(). That's covered by the last patch: diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c index e961e9a05847..06061c11d74d 100644 --- a/arch/x86/kvm/cpuid.c +++ b/arch/x86/kvm/cpuid.c @@ -677,7 +677,7 @@ void kvm_set_cpu_caps(void) kvm_cpu_cap_mask(CPUID_7_1_EAX, F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) | F(FZRM) | F(FSRS) | F(FSRC) | - F(AMX_FP16) | F(AVX_IFMA) + F(AMX_FP16) | F(AVX_IFMA) | F(LAM) ); kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, Which highlights a problem with activating a goverened feature before said feature is actually supported by KVM: it's all kinds of confusing. It'll generate a more churn in git history, but I think we should first enable LAM without a goverened feature, and then activate a goverened feature later on. Using a goverened feature is purely an optimization, i.e. the series needs to be function without using a governed feature. That should yield an easier-to-review series on all fronts: the initial supports won't have any more hidden dependencies than absolutely necessary, switching to a goverened feature should be a very mechanical conversion (if it's not, that's a red flag), and last but not least, it makes it super easy to make a judgment call as to whether using a governed feature flag is justified, because all of the users will be in scope. TL;DR: Do the whole goverened feature thing dead last.
On Wed, 2023-08-16 at 14:33 -0700, Sean Christopherson wrote: > On Wed, Aug 16, 2023, Kai Huang wrote: > > > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > > vmx->msr_ia32_feature_control_valid_bits &= > > > > > ~FEAT_CTL_SGX_LC_ENABLED; > > > > > > > > > > + if (boot_cpu_has(X86_FEATURE_LAM)) > > > > > + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); > > > > > + > > > > If you want to use boot_cpu_has(), it's better to be done at your last patch to > > > > only set the cap bit when boot_cpu_has() is true, I suppose. > > > Yes, but new version of kvm_governed_feature_check_and_set() of > > > KVM-governed feature framework will check against kvm_cpu_cap_has() as well. > > > I will remove the if statement and call > > > kvm_governed_feature_check_and_set() directly. > > > https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ > > > > > > > I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you > > are using boot_cpu_has(). They are not the same. > > > > If LAM should be only supported when boot_cpu_has() is true then it seems you > > can just only set the LAM cap bit when boot_cpu_has() is true. As you also > > mentioned above the kvm_governed_feature_check_and_set() here internally does > > kvm_cpu_cap_has(). > > That's covered by the last patch: > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index e961e9a05847..06061c11d74d 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -677,7 +677,7 @@ void kvm_set_cpu_caps(void) > kvm_cpu_cap_mask(CPUID_7_1_EAX, > F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) | > F(FZRM) | F(FSRS) | F(FSRC) | > - F(AMX_FP16) | F(AVX_IFMA) > + F(AMX_FP16) | F(AVX_IFMA) | F(LAM) > ); > > kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, > Ah I missed this piece of code in kvm_set_cpu_caps(): memcpy(&kvm_cpu_caps, &boot_cpu_data.x86_capability, sizeof(kvm_cpu_caps) - (NKVMCAPINTS * sizeof(*kvm_cpu_caps))); which makes sure KVM only reports one feature when it is set in boot_cpu_data, which is obviously more reasonable. Sorry for the noise :-)
On 8/17/2023 5:33 AM, Sean Christopherson wrote: > On Wed, Aug 16, 2023, Kai Huang wrote: >>>>> --- a/arch/x86/kvm/vmx/vmx.c >>>>> +++ b/arch/x86/kvm/vmx/vmx.c >>>>> @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) >>>>> vmx->msr_ia32_feature_control_valid_bits &= >>>>> ~FEAT_CTL_SGX_LC_ENABLED; >>>>> >>>>> + if (boot_cpu_has(X86_FEATURE_LAM)) >>>>> + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); >>>>> + >>>> If you want to use boot_cpu_has(), it's better to be done at your last patch to >>>> only set the cap bit when boot_cpu_has() is true, I suppose. >>> Yes, but new version of kvm_governed_feature_check_and_set() of >>> KVM-governed feature framework will check against kvm_cpu_cap_has() as well. >>> I will remove the if statement and call >>> kvm_governed_feature_check_and_set() directly. >>> https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ >>> >> I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you >> are using boot_cpu_has(). They are not the same. >> >> If LAM should be only supported when boot_cpu_has() is true then it seems you >> can just only set the LAM cap bit when boot_cpu_has() is true. As you also >> mentioned above the kvm_governed_feature_check_and_set() here internally does >> kvm_cpu_cap_has(). > That's covered by the last patch: > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > index e961e9a05847..06061c11d74d 100644 > --- a/arch/x86/kvm/cpuid.c > +++ b/arch/x86/kvm/cpuid.c > @@ -677,7 +677,7 @@ void kvm_set_cpu_caps(void) > kvm_cpu_cap_mask(CPUID_7_1_EAX, > F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) | > F(FZRM) | F(FSRS) | F(FSRC) | > - F(AMX_FP16) | F(AVX_IFMA) > + F(AMX_FP16) | F(AVX_IFMA) | F(LAM) > ); > > kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, > > > Which highlights a problem with activating a goverened feature before said feature > is actually supported by KVM: it's all kinds of confusing. > > It'll generate a more churn in git history, but I think we should first enable > LAM without a goverened feature, and then activate a goverened feature later on. > Using a goverened feature is purely an optimization, i.e. the series needs to be > function without using a governed feature. OK, then how about the second option which has been listed in your v9 patch series discussion. https://lore.kernel.org/kvm/20230606091842.13123-1-binbin.wu@linux.intel.com/T/#m16ee5cec4a46954f985cb6afedb5f5a3435373a1 Temporarily add a bool can_use_lam in kvm_vcpu_arch and use the bool "can_use_lam" instead of guest_can_use(vcpu, X86_FEATURE_LAM). and then put the patch of adopting "KVM-governed feature framework" to the last. > > That should yield an easier-to-review series on all fronts: the initial supports > won't have any more hidden dependencies than absolutely necessary, switching to > a goverened feature should be a very mechanical conversion (if it's not, that's > a red flag), and last but not least, it makes it super easy to make a judgment > call as to whether using a governed feature flag is justified, because all of the > users will be in scope. > > TL;DR: Do the whole goverened feature thing dead last.
On Thu, Aug 17, 2023, Binbin Wu wrote: > > > On 8/17/2023 5:33 AM, Sean Christopherson wrote: > > On Wed, Aug 16, 2023, Kai Huang wrote: > > > > > > --- a/arch/x86/kvm/vmx/vmx.c > > > > > > +++ b/arch/x86/kvm/vmx/vmx.c > > > > > > @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) > > > > > > vmx->msr_ia32_feature_control_valid_bits &= > > > > > > ~FEAT_CTL_SGX_LC_ENABLED; > > > > > > + if (boot_cpu_has(X86_FEATURE_LAM)) > > > > > > + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); > > > > > > + > > > > > If you want to use boot_cpu_has(), it's better to be done at your last patch to > > > > > only set the cap bit when boot_cpu_has() is true, I suppose. > > > > Yes, but new version of kvm_governed_feature_check_and_set() of > > > > KVM-governed feature framework will check against kvm_cpu_cap_has() as well. > > > > I will remove the if statement and call > > > > kvm_governed_feature_check_and_set() directly. > > > > https://lore.kernel.org/kvm/20230815203653.519297-2-seanjc@google.com/ > > > > > > > I mean kvm_cpu_cap_has() checks against the host CPUID directly while here you > > > are using boot_cpu_has(). They are not the same. > > > > > > If LAM should be only supported when boot_cpu_has() is true then it seems you > > > can just only set the LAM cap bit when boot_cpu_has() is true. As you also > > > mentioned above the kvm_governed_feature_check_and_set() here internally does > > > kvm_cpu_cap_has(). > > That's covered by the last patch: > > > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c > > index e961e9a05847..06061c11d74d 100644 > > --- a/arch/x86/kvm/cpuid.c > > +++ b/arch/x86/kvm/cpuid.c > > @@ -677,7 +677,7 @@ void kvm_set_cpu_caps(void) > > kvm_cpu_cap_mask(CPUID_7_1_EAX, > > F(AVX_VNNI) | F(AVX512_BF16) | F(CMPCCXADD) | > > F(FZRM) | F(FSRS) | F(FSRC) | > > - F(AMX_FP16) | F(AVX_IFMA) > > + F(AMX_FP16) | F(AVX_IFMA) | F(LAM) > > ); > > kvm_cpu_cap_init_kvm_defined(CPUID_7_1_EDX, > > > > > > Which highlights a problem with activating a goverened feature before said feature > > is actually supported by KVM: it's all kinds of confusing. > > > > It'll generate a more churn in git history, but I think we should first enable > > LAM without a goverened feature, and then activate a goverened feature later on. > > Using a goverened feature is purely an optimization, i.e. the series needs to be > > function without using a governed feature. > OK, then how about the second option which has been listed in your v9 patch > series discussion. > https://lore.kernel.org/kvm/20230606091842.13123-1-binbin.wu@linux.intel.com/T/#m16ee5cec4a46954f985cb6afedb5f5a3435373a1 > > Temporarily add a bool can_use_lam in kvm_vcpu_arch and use the bool > "can_use_lam" instead of guest_can_use(vcpu, X86_FEATURE_LAM). > and then put the patch of adopting "KVM-governed feature framework" to the > last. No, just do the completely unoptimized, but functionally obvious thing: if (kvm_cpu_cap_has(x86_FEATURE_LAM) && guest_cpuid_has(vcpu, x86_FEATURE_LAM)) ... I don't expect anyone to push back on using a governed feature, i.e. I don't expect to ever see a kernel release with the unoptimized code. If someone is bisecting or doing something *really* weird with their kernel management, then yes, they might see suboptimal performance. Again, the goal is to separate the addition of functionality from the optimization of that functionality, e.g. to make it easier to review and understand each change.
diff --git a/arch/x86/kvm/governed_features.h b/arch/x86/kvm/governed_features.h index 40ce8e6608cd..708578d60e6f 100644 --- a/arch/x86/kvm/governed_features.h +++ b/arch/x86/kvm/governed_features.h @@ -5,5 +5,7 @@ BUILD_BUG() #define KVM_GOVERNED_X86_FEATURE(x) KVM_GOVERNED_FEATURE(X86_FEATURE_##x) +KVM_GOVERNED_X86_FEATURE(LAM) + #undef KVM_GOVERNED_X86_FEATURE #undef KVM_GOVERNED_FEATURE diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c index 0ecf4be2c6af..ae47303c88d7 100644 --- a/arch/x86/kvm/vmx/vmx.c +++ b/arch/x86/kvm/vmx/vmx.c @@ -7783,6 +7783,9 @@ static void vmx_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu) vmx->msr_ia32_feature_control_valid_bits &= ~FEAT_CTL_SGX_LC_ENABLED; + if (boot_cpu_has(X86_FEATURE_LAM)) + kvm_governed_feature_check_and_set(vcpu, X86_FEATURE_LAM); + /* Refresh #PF interception to account for MAXPHYADDR changes. */ vmx_update_exception_bitmap(vcpu); }
Use the governed feature framework to track if Linear Address Masking (LAM) is "enabled", i.e. if LAM can be used by the guest. So that guest_can_use() can be used to support LAM virtualization. LAM modifies the checking that is applied to 64-bit linear addresses, allowing software to use of the untranslated address bits for metadata and masks the metadata bits before using them as linear addresses to access memory. Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com> --- arch/x86/kvm/governed_features.h | 2 ++ arch/x86/kvm/vmx/vmx.c | 3 +++ 2 files changed, 5 insertions(+)