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

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

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.