Re: [PATCH v8 2/6] KVM: x86: Virtualize CR4.LAM_SUP

[Huang, Kai]

On Wed, 2023-05-10 at 14:06 +0800, Binbin Wu wrote:
> From: Robert Hoo <robert.hu@xxxxxxxxxxxxxxx>
> 
> Add support to allow guests to set the new CR4 control bit for guests to enable
> the new Intel CPU feature Linear Address Masking (LAM) on supervisor pointers.
> 
> 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. LAM uses
> CR4.LAM_SUP (bit 28) to configure LAM for supervisor pointers. LAM also changes
> VMENTER to allow the bit to be set in VMCS's HOST_CR4 and GUEST_CR4 for
> virtualization.
> 
> Move CR4.LAM_SUP out of CR4_RESERVED_BITS and its reservation depends on vcpu
> supporting LAM feature or not. Leave the bit intercepted to avoid vmread every
> time when KVM fetches its value, with the expectation that guest won't toggle
> the bit frequently.
> 
> Set CR4.LAM_SUP bit in the emulated IA32_VMX_CR4_FIXED1 MSR for guests to allow
> guests to enable LAM for supervisor pointers in nested VMX operation.
> 
> Hardware is not required to do TLB flush when CR4.LAM_SUP toggled, KVM doesn't
> need to emulate TLB flush based on it.
> There's no other features/vmx_exec_controls connection, no other code needed in
> {kvm,vmx}_set_cr4().
> 
> Signed-off-by: Robert Hoo <robert.hu@xxxxxxxxxxxxxxx>
> Co-developed-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> Signed-off-by: Binbin Wu <binbin.wu@xxxxxxxxxxxxxxx>
> Reviewed-by: Chao Gao <chao.gao@xxxxxxxxx>
> Tested-by: Xuelian Guo <xuelian.guo@xxxxxxxxx>
> ---
>  arch/x86/include/asm/kvm_host.h | 3 ++-
>  arch/x86/kvm/vmx/vmx.c          | 3 +++
>  arch/x86/kvm/x86.h              | 2 ++
>  3 files changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index fb9d1f2d6136..c6f03d151c31 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -125,7 +125,8 @@
>  			  | X86_CR4_PGE | X86_CR4_PCE | X86_CR4_OSFXSR | X86_CR4_PCIDE \
>  			  | X86_CR4_OSXSAVE | X86_CR4_SMEP | X86_CR4_FSGSBASE \
>  			  | X86_CR4_OSXMMEXCPT | X86_CR4_LA57 | X86_CR4_VMXE \
> -			  | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP))
> +			  | X86_CR4_SMAP | X86_CR4_PKE | X86_CR4_UMIP \
> +			  | X86_CR4_LAM_SUP))
>  
>  #define CR8_RESERVED_BITS (~(unsigned long)X86_CR8_TPR)
>  
> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
> index 44fb619803b8..362b2dce7661 100644
> --- a/arch/x86/kvm/vmx/vmx.c
> +++ b/arch/x86/kvm/vmx/vmx.c
> @@ -7603,6 +7603,9 @@ static void nested_vmx_cr_fixed1_bits_update(struct kvm_vcpu *vcpu)
>  	cr4_fixed1_update(X86_CR4_UMIP,       ecx, feature_bit(UMIP));
>  	cr4_fixed1_update(X86_CR4_LA57,       ecx, feature_bit(LA57));
>  
> +	entry = kvm_find_cpuid_entry_index(vcpu, 0x7, 1);
> +	cr4_fixed1_update(X86_CR4_LAM_SUP,    eax, feature_bit(LAM));
> +
>  #undef cr4_fixed1_update
>  }
>  
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index c544602d07a3..fe67b641cce4 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -529,6 +529,8 @@ bool kvm_msr_allowed(struct kvm_vcpu *vcpu, u32 index, u32 type);
>  		__reserved_bits |= X86_CR4_VMXE;        \
>  	if (!__cpu_has(__c, X86_FEATURE_PCID))          \
>  		__reserved_bits |= X86_CR4_PCIDE;       \
> +	if (!__cpu_has(__c, X86_FEATURE_LAM))           \
> +		__reserved_bits |= X86_CR4_LAM_SUP;     \
>  	__reserved_bits;                                \
>  })
>  

LAM only applies to 64-bit linear address, which means LAM can only be enabled
when CPU is in 64-bit mode with either 4-level or 5-level paging enabled.

What's the hardware behaviour if we set CR4.LAM_SUP when CPU isn't in 64-bit
mode?  And how does VMENTRY check GUEST_CR4.LAM_SUP and 64-bit mode? 

Looks they are not clear in the spec you pasted in the cover letter:

https://cdrdv2.intel.com/v1/dl/getContent/671368

Or I am missing something?