linux-kernel - Re: [PATCH v8 2/6] KVM: x86: Virtualize CR4.LAM

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <abbb7938-0615-8578-0072-a96d21df3b4d@linux.intel.com>
Date:   Fri, 12 May 2023 09:33:57 +0800
From:   Binbin Wu <binbin.wu@...ux.intel.com>
To:     "Huang, Kai" <kai.huang@...el.com>
Cc:     "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "robert.hu@...ux.intel.com" <robert.hu@...ux.intel.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "Christopherson,, Sean" <seanjc@...gle.com>,
        "Gao, Chao" <chao.gao@...el.com>
Subject: Re: [PATCH v8 2/6] KVM: x86: Virtualize CR4.LAM_SUP



On 5/11/2023 8:50 PM, Huang, Kai wrote:
> On Wed, 2023-05-10 at 14:06 +0800, Binbin Wu wrote:
>> From: Robert Hoo <robert.hu@...ux.intel.com>
>>
>> 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@...ux.intel.com>
>> Co-developed-by: Binbin Wu <binbin.wu@...ux.intel.com>
>> Signed-off-by: Binbin Wu <binbin.wu@...ux.intel.com>
>> Reviewed-by: Chao Gao <chao.gao@...el.com>
>> Tested-by: Xuelian Guo <xuelian.guo@...el.com>
>> ---
>>   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?
Yes, it is not clearly described in LAM spec.
Had some internal discussions and also did some tests in host,
if the processor supports LAM, CR4.LAM_SUP is allowed to be set even 
when cpu isn't in 64bit mode.

There was a statement in commit message of the last version, but I 
missed it in this version. I'll add it back.
"CR4.LAM_SUP is allowed to be set even not in 64-bit mode, but it will not
take effect since LAM only applies to 64-bit linear address."

Also, I will try to ask Intel guys if it's possible to update the document.