linux-kernel - Re: [PATCH v3 25/27] KVM: nVMX: Add FRED VMCS fields

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: <Zx9Ua0dTQXwC9lzS@intel.com>
Date: Mon, 28 Oct 2024 17:07:55 +0800
From: Chao Gao <chao.gao@...el.com>
To: Xin Li <xin@...or.com>
CC: <kvm@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-doc@...r.kernel.org>, <seanjc@...gle.com>, <pbonzini@...hat.com>,
	<corbet@....net>, <tglx@...utronix.de>, <mingo@...hat.com>, <bp@...en8.de>,
	<dave.hansen@...ux.intel.com>, <x86@...nel.org>, <hpa@...or.com>,
	<luto@...nel.org>, <peterz@...radead.org>, <andrew.cooper3@...rix.com>
Subject: Re: [PATCH v3 25/27] KVM: nVMX: Add FRED VMCS fields

On Fri, Oct 25, 2024 at 12:25:45AM -0700, Xin Li wrote:
>On 10/24/2024 12:42 AM, Chao Gao wrote:
>> > @@ -7197,6 +7250,9 @@ static void nested_vmx_setup_basic(struct nested_vmx_msrs *msrs)
>> > 	msrs->basic |= VMX_BASIC_TRUE_CTLS;
>> > 	if (cpu_has_vmx_basic_inout())
>> > 		msrs->basic |= VMX_BASIC_INOUT;
>> > +
>> > +	if (cpu_has_vmx_fred())
>> > +		msrs->basic |= VMX_BASIC_NESTED_EXCEPTION;
>> 
>> why not advertising VMX_BASIC_NESTED_EXCEPTION if the CPU supports it? just like
>> VMX_BASIC_INOUT right above.
>
>Because VMX nested-exception support only works with FRED.
>
>We could pass host MSR_IA32_VMX_BASIC.VMX_BASIC_NESTED_EXCEPTION to
>nested, but it's meaningless w/o VMX FRED.

But it seems KVM cannot benefit from this attempt to avoid meaningless
configurations because on FRED-capable system, the userspace VMM can choose to
hide FRED and expose VMX nested exceptions alone. KVM needs to handle this case
anyway. I suggest not bothering with it.

>
>> 
>> 
>> > }
>> > 
>> > static void nested_vmx_setup_cr_fixed(struct nested_vmx_msrs *msrs)
>> > diff --git a/arch/x86/kvm/vmx/nested.h b/arch/x86/kvm/vmx/nested.h
>> > index 2c296b6abb8c..5272f617fcef 100644
>> > --- a/arch/x86/kvm/vmx/nested.h
>> > +++ b/arch/x86/kvm/vmx/nested.h
>> > @@ -251,6 +251,14 @@ static inline bool nested_cpu_has_encls_exit(struct vmcs12 *vmcs12)
>> > 	return nested_cpu_has2(vmcs12, SECONDARY_EXEC_ENCLS_EXITING);
>> > }
>> > 
>> > +static inline bool nested_cpu_has_fred(struct vmcs12 *vmcs12)
>> > +{
>> > +	return vmcs12->vm_entry_controls & VM_ENTRY_LOAD_IA32_FRED &&
>> > +	       vmcs12->vm_exit_controls & VM_EXIT_ACTIVATE_SECONDARY_CONTROLS &&
>> > +	       vmcs12->secondary_vm_exit_controls & SECONDARY_VM_EXIT_SAVE_IA32_FRED &&
>> > +	       vmcs12->secondary_vm_exit_controls & SECONDARY_VM_EXIT_LOAD_IA32_FRED;
>> 
>> Is it a requirement in the SDM that the VMM should enable all FRED controls or
>> none? If not, the VMM is allowed to enable only one or two of them. This means
>> KVM would need to emulate FRED controls for the L1 VMM as three separate
>> features.
>
>The SDM doesn't say that.  But FRED states are used during and
>immediately after VM entry and exit, I don't see a good reason for a VMM
>to enable only one or two of the 3 save/load configs.
>
>Say if VM_ENTRY_LOAD_IA32_FRED is not set, it means a VMM needs to
>switch to guest FRED states before it does a VM entry, which is
>absolutely a big mess.

If the VMM doesn't enable FRED, it's fine to load guest FRED states before VM
entry, right?

The key is to emulate hardware behavior accurately without making assumptions
about guests. If some combinations of controls cannot be emulated properly, KVM
should report internal errors at some point.

>
>TBH I'm not sure this is the question you have in mind.