| Message ID | 20231108183003.5981-12-xin3.li@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable FRED with KVM VMX | expand |
On Wed, Nov 08, 2023 at 10:29:51AM -0800, Xin Li wrote: >Add kvm_is_fred_enabled() to get if FRED is enabled on a vCPU. > >Tested-by: Shan Kang <shan.kang@intel.com> >Signed-off-by: Xin Li <xin3.li@intel.com> >--- > arch/x86/kvm/kvm_cache_regs.h | 10 ++++++++++ > 1 file changed, 10 insertions(+) > >diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h >index 75eae9c4998a..390643e8c532 100644 >--- a/arch/x86/kvm/kvm_cache_regs.h >+++ b/arch/x86/kvm/kvm_cache_regs.h >@@ -187,6 +187,16 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, > return !!kvm_read_cr4_bits(vcpu, cr4_bit); > } > >+static __always_inline bool kvm_is_fred_enabled(struct kvm_vcpu *vcpu) >+{ >+#ifdef CONFIG_X86_64 >+ return cpu_feature_enabled(X86_FEATURE_FRED) && >+ kvm_is_cr4_bit_set(vcpu, X86_CR4_FRED); FRED is enabled when CR4.FRED = IA32_EFER.LMA = 1. Any reason to omit the check about long mode? >+#else >+ return false; >+#endif >+} >+ > static inline ulong kvm_read_cr3(struct kvm_vcpu *vcpu) > { > if (!kvm_register_is_available(vcpu, VCPU_EXREG_CR3)) >-- >2.42.0 > >
> >+ return cpu_feature_enabled(X86_FEATURE_FRED) && > >+ kvm_is_cr4_bit_set(vcpu, X86_CR4_FRED); > > FRED is enabled when CR4.FRED = IA32_EFER.LMA = 1. Any reason to omit the > check about long mode? It won' t allow CR4.FRED to be set if not in long mode, I don't expect it at runtime. Or you have one? If you are talking about save/restore a corrupted vCPU state, a following VM entry should fail anyway.
On Tue, Nov 14, 2023 at 12:42:13PM +0800, Li, Xin3 wrote: >> >+ return cpu_feature_enabled(X86_FEATURE_FRED) && >> >+ kvm_is_cr4_bit_set(vcpu, X86_CR4_FRED); >> >> FRED is enabled when CR4.FRED = IA32_EFER.LMA = 1. Any reason to omit the >> check about long mode? > >It won' t allow CR4.FRED to be set if not in long mode, I don't expect it >at runtime. Or you have one? I was thinking about a very contrived case: 1. the CPU enters 64-bit long mode and sets CR4.FRED 2. the CPU switches out of 64-bit long mode and SDM vol3 chapter 2.5 CONTROL REGISTERS says: A 64-bit capable processor will retain the upper 32 bits of each control register when transitioning out of IA-32e mode. so, to me, it is possible that CR4.FRED is 1 while IA32_EFER.LMA is 0. and in this case, FRED should be considered disabled. Anyway, I think we should align with FRED SPEC. If we deliberately omit the check about long mode, please add a comment to explain why it is ok to do that. > >If you are talking about save/restore a corrupted vCPU state, a following >VM entry should fail anyway.
> >> FRED is enabled when CR4.FRED = IA32_EFER.LMA = 1. Any reason to omit the > >> check about long mode? > > > >It won' t allow CR4.FRED to be set if not in long mode, I don't expect it > >at runtime. Or you have one? > > I was thinking about a very contrived case: > > 1. the CPU enters 64-bit long mode and sets CR4.FRED > 2. the CPU switches out of 64-bit long mode > > and SDM vol3 chapter 2.5 CONTROL REGISTERS says: > > A 64-bit capable processor will retain the upper 32 bits of each control > register when transitioning out of IA-32e mode. > > so, to me, it is possible that CR4.FRED is 1 while IA32_EFER.LMA is 0. > and in this case, FRED should be considered disabled. You're correct, this is a solid case. It's not one-way, but I forgot the other way around. > > Anyway, I think we should align with FRED SPEC. If we deliberately omit > the check about long mode, please add a comment to explain why it is ok > to do that. Yeah, I will add it.
> > >> FRED is enabled when CR4.FRED = IA32_EFER.LMA = 1. Any reason to > > >> omit the check about long mode? > > > > > >It won' t allow CR4.FRED to be set if not in long mode, I don't > > >expect it at runtime. Or you have one? > > > > I was thinking about a very contrived case: > > > > 1. the CPU enters 64-bit long mode and sets CR4.FRED 2. the CPU > > switches out of 64-bit long mode Actually, SDM3 Section 10.8.5 Initializing IA-32e Mode says: 64-bit mode consistency checks fail on attempts to enable or disable IA-32e mode while paging is enabled. In another word, the CPU allows software to modify IA32_EFER.LME only when CR0.PG = 0 (i.e., only in legacy mode). Thus, attempts to do so when CR0.PG = 1 will cause #GP. Remember FRED only works with 64-bit kernel, which can be entered only if paging is enabled. As such, to clear IA32_EFER.LME, OS/VMM needs to turn off paging first. And to turn off paging, 64-bit mode needs to be turned off (to enter compatibility mode in ring 0 because it's not allowed to disable paging in 64-bit mode). Thus CR4.FRED needs be cleared first. So CR4.FRED fully indicates whether FRED is enabled or not, which is why FRED spec 5.0 section 9.5 says so. But we can put more explanation there.
diff --git a/arch/x86/kvm/kvm_cache_regs.h b/arch/x86/kvm/kvm_cache_regs.h index 75eae9c4998a..390643e8c532 100644 --- a/arch/x86/kvm/kvm_cache_regs.h +++ b/arch/x86/kvm/kvm_cache_regs.h @@ -187,6 +187,16 @@ static __always_inline bool kvm_is_cr4_bit_set(struct kvm_vcpu *vcpu, return !!kvm_read_cr4_bits(vcpu, cr4_bit); } +static __always_inline bool kvm_is_fred_enabled(struct kvm_vcpu *vcpu) +{ +#ifdef CONFIG_X86_64 + return cpu_feature_enabled(X86_FEATURE_FRED) && + 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))