linux-kernel - Re: [PATCH v3 03/27] KVM: VMX: Add support for the secondary VM exit controls

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: <ZxcSPpuBHO8Y1jfG@intel.com>
Date: Tue, 22 Oct 2024 10:47:26 +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 03/27] KVM: VMX: Add support for the secondary VM exit
 controls

On Mon, Oct 21, 2024 at 10:03:45AM -0700, Xin Li wrote:
>On 10/21/2024 1:28 AM, Chao Gao wrote:
>> > +	for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit_triplets); i++) {
>> > +		u32 n_ctrl = vmcs_entry_exit_triplets[i].entry_control;
>> > +		u32 x_ctrl = vmcs_entry_exit_triplets[i].exit_control;
>> > +		u64 x_ctrl_2 = vmcs_entry_exit_triplets[i].exit_2nd_control;
>> > +		bool has_n = n_ctrl && ((_vmentry_control & n_ctrl) == n_ctrl);
>> > +		bool has_x = x_ctrl && ((_vmexit_control & x_ctrl) == x_ctrl);
>> > +		bool has_x_2 = x_ctrl_2 && ((_secondary_vmexit_control & x_ctrl_2) == x_ctrl_2);
>> > +
>> > +		if (x_ctrl_2) {
>> > +			/* Only activate secondary VM exit control bit should be set */
>> > +			if ((_vmexit_control & x_ctrl) == VM_EXIT_ACTIVATE_SECONDARY_CONTROLS) {
>> > +				if (has_n == has_x_2)
>> > +					continue;
>> > +			} else {
>> > +				/* The feature should not be supported in any control */
>> > +				if (!has_n && !has_x && !has_x_2)
>> > +					continue;
>> > +			}
>> > +		} else if (has_n == has_x) {
>> > 			continue;
>> > +		}
>> > 
>> > -		pr_warn_once("Inconsistent VM-Entry/VM-Exit pair, entry = %x, exit = %x\n",
>> > -			     _vmentry_control & n_ctrl, _vmexit_control & x_ctrl);
>> > +		pr_warn_once("Inconsistent VM-Entry/VM-Exit triplet, entry = %x, exit = %x, secondary_exit = %llx\n",
>> > +			     _vmentry_control & n_ctrl, _vmexit_control & x_ctrl,
>> > +			     _secondary_vmexit_control & x_ctrl_2);
>> > 
>> > 		if (error_on_inconsistent_vmcs_config)
>> > 			return -EIO;
>> > 
>> > 		_vmentry_control &= ~n_ctrl;
>> > 		_vmexit_control &= ~x_ctrl;
>> 
>> w/ patch 4, VM_EXIT_ACTIVATE_SECONDARY_CONTROLS is cleared if FRED fails in the
>> consistent check. this means, all features in the secondary vm-exit controls
>> are removed. it is overkill.
>
>Good catch!
>
>> 
>> I prefer to maintain a separate table for the secondary VM-exit controls:
>> 
>>   	struct {
>>   		u32 entry_control;
>>   		u64 exit2_control;
>> 	} const vmcs_entry_exit2_pairs[] = {
>> 		{ VM_ENTRY_LOAD_IA32_FRED, SECONDARY_VM_EXIT_SAVE_IA32_FRED |
>> 					   SECONDARY_VM_EXIT_LOAD_IA32_FRED},
>> 	};
>> 
>> 	for (i = 0; i < ARRAY_SIZE(vmcs_entry_exit2_pairs); i++) {
>> 	...
>> 	}
>
>Hmm, I prefer one table, as it's more straight forward.

One table is fine if we can fix the issue and improve readability. The three
nested if() statements hurts readability.

I just thought using two tables would eliminate the need for any if() statements.