diff mbox series

[v1,11/23] KVM: x86: Add kvm_is_fred_enabled()

Message ID 20231108183003.5981-12-xin3.li@intel.com
State New
Headers
Series Enable FRED with KVM VMX |

Commit Message

Li, Xin3 Nov. 8, 2023, 6:29 p.m. UTC 
  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(+)

Comments

Chao Gao Nov. 13, 2023, 7:35 a.m. UTC | #1 
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
>
>
Li, Xin3 Nov. 14, 2023, 4:42 a.m. UTC | #2 
> >+	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.
Chao Gao Nov. 14, 2023, 8:16 a.m. UTC | #3 
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.
Li, Xin3 Nov. 14, 2023, 6:57 p.m. UTC | #4 
> >> 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.
Li, Xin3 Nov. 20, 2023, 9:04 a.m. UTC | #5 
> > >> 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 mbox series

Patch

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))