| Message ID | 20240207172646.3981-12-xin3.li@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Enable FRED with KVM VMX | expand |
On Thu, Feb 08, 2024 at 01:26:31AM +0800, Xin Li wrote: >Add kvm_is_fred_enabled() to get if FRED is enabled on a vCPU. > >Signed-off-by: Xin Li <xin3.li@intel.com> >Tested-by: Shan Kang <shan.kang@intel.com> >--- > >Change since v1: >* Explain why it is ok to only check CR4.FRED (Chao Gao). >--- > arch/x86/kvm/kvm_cache_regs.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > >diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h >index 75eae9c4998a..1d431c703fdf 100644 >--- a/arch/x86/kvm/kvm_cache_regs.h >+++ b/arch/x86/kvm/kvm_cache_regs.h >@@ -187,6 +187,23 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, > return !!kvm_read_cr4_bits(vcpu, cr4_bit); > } > >+/* >+ * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if >+ * a vCPU is running with FRED enabled, because: >+ * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. >+ * 2) To leave IA-32e mode, CR4.FRED must be cleared first. >+ * >+ * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. >+ */ I think we can give more context here, e.g., Although FRED architecture applies to 64-bit mode only, there is no need to check if the CPU is in 64-bit mode (i.e., IA32_EFER.LMA and CS.L) to tell if FRED is enabled because CR4.FRED=1 implies the CPU is in 64-bit mode. Specifically, 1) .. 2) ..
> >+/* > >+ * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if > >+ * a vCPU is running with FRED enabled, because: > >+ * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. > >+ * 2) To leave IA-32e mode, CR4.FRED must be cleared first. > >+ * > >+ * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. > >+ */ > > I think we can give more context here, e.g., > > Although FRED architecture applies to 64-bit mode only, there is no need to check if > the CPU is in 64-bit mode (i.e., IA32_EFER.LMA and CS.L) to tell if FRED is enabled > because CR4.FRED=1 implies the CPU is in 64-bit mode. What is "more context" here? > Specifically,
On Sat, May 11, 2024 at 09:24:12AM +0800, Li, Xin3 wrote: >> >+/* >> >+ * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if >> >+ * a vCPU is running with FRED enabled, because: >> >+ * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. >> >+ * 2) To leave IA-32e mode, CR4.FRED must be cleared first. >> >+ * >> >+ * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. >> >+ */ >> >> I think we can give more context here, e.g., >> >> Although FRED architecture applies to 64-bit mode only, there is no need to check if >> the CPU is in 64-bit mode (i.e., IA32_EFER.LMA and CS.L) to tell if FRED is enabled >> because CR4.FRED=1 implies the CPU is in 64-bit mode. > >What is "more context" here? e.g., why IA32_EFER.LMA and CPU mode are related to FRED here? "it's enough to " implies something else is not necessary. what is it? I don't think the original comment make them super clear. > >> Specifically, > >
On Wed, Feb 07, 2024, Xin Li wrote: > Add kvm_is_fred_enabled() to get if FRED is enabled on a vCPU. > > Signed-off-by: Xin Li <xin3.li@intel.com> > Tested-by: Shan Kang <shan.kang@intel.com> > --- > > Change since v1: > * Explain why it is ok to only check CR4.FRED (Chao Gao). > --- > arch/x86/kvm/kvm_cache_regs.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 75eae9c4998a..1d431c703fdf 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -187,6 +187,23 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, > return !!kvm_read_cr4_bits(vcpu, cr4_bit); > } > > +/* > + * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if > + * a vCPU is running with FRED enabled, because: > + * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. > + * 2) To leave IA-32e mode, CR4.FRED must be cleared first. > + * > + * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. > + */ > +static __always_inline bool kvm_is_fred_enabled(struct kvm_vcpu *vcpu) Maybe just is_fred_enabled(), or even just is_fred()? Most helpers in x86.h that wrap CR4/CR0 in similar ways omit the "kvm_", partly for brevity, but also because the check is architectural, not KVM-defined (though the state obviously comes from KVM). > +{ > +#ifdef CONFIG_X86_64 > + return kvm_is_cr4_bit_set(vcpu, X86_CR4_FRED); > +#else > + return false; > +#endif > +} > + > static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu) > { > if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3)) > -- > 2.43.0 >
On Wed, Feb 07, 2024, Xin Li wrote: > Add kvm_is_fred_enabled() to get if FRED is enabled on a vCPU. > > Signed-off-by: Xin Li <xin3.li@intel.com> > Tested-by: Shan Kang <shan.kang@intel.com> > --- > > Change since v1: > * Explain why it is ok to only check CR4.FRED (Chao Gao). > --- > arch/x86/kvm/kvm_cache_regs.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h > index 75eae9c4998a..1d431c703fdf 100644 > --- a/arch/x86/kvm/kvm_cache_regs.h > +++ b/arch/x86/kvm/kvm_cache_regs.h > @@ -187,6 +187,23 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, > return !!kvm_read_cr4_bits(vcpu, cr4_bit); > } > > +/* > + * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if > + * a vCPU is running with FRED enabled, because: > + * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. > + * 2) To leave IA-32e mode, CR4.FRED must be cleared first. > + * > + * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. Please don't reference specific sections/tables/fields in comments. They always become stale. And the code+comments always reflect the current state, i.e. don't need to worry about spec revisions and whatnot. If there is a spec change, then there darn well needs to be a way for software to differentiate old vs. new, at which point there will be accompanying code to capture the difference. Even in changelogs, references specific specs by section number is usually discouraged. Again, it shouldn't matter if its FRED spec 6.0 vs. spec 5.0, because if there is a difference between those two, then the code better be different too. Instead, for the changelog, if it's really necessary/helpful, reference the section by name and/or keyword, as those are much less likely to become stale. > + */ > +static __always_inline bool kvm_is_fred_enabled(struct kvm_vcpu *vcpu) This doesn't need to be __always_inline, it's not used from a noinstr section. kvm_is_cr4_bit_set() is __always_inline so that @cr4_bit is guaranteed to be a compile-time constant, otherwise the BUILD_BUG_ON() would fail.
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h index 75eae9c4998a..1d431c703fdf 100644 --- a/arch/x86/kvm/kvm_cache_regs.h +++ b/arch/x86/kvm/kvm_cache_regs.h @@ -187,6 +187,23 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, return !!kvm_read_cr4_bits(vcpu, cr4_bit); } +/* + * It's enough to check just CR4.FRED (X86_CR4_FRED) to tell if + * a vCPU is running with FRED enabled, because: + * 1) CR4.FRED can be set to 1 only _after_ IA32_EFER.LMA = 1. + * 2) To leave IA-32e mode, CR4.FRED must be cleared first. + * + * More details at FRED Spec 6.0 Section 4.2 Enabling in CR4. + */ +static __always_inline bool kvm_is_fred_enabled(struct kvm_vcpu *vcpu) +{ +#ifdef CONFIG_X86_64 + return kvm_is_cr4_bit_set(vcpu, X86_CR4_FRED); +#else + return false; +#endif +} + static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu) { if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3))