linux-kernel - Re: [PATCH v4 09/25] KVM: VMX: nVMX: Support TSC scaling and PERF_GLOBAL

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: <YuMKBzeB2cE/NZ2K@google.com>
Date:   Thu, 28 Jul 2022 22:13:27 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>, kvm@...r.kernel.org,
        Anirudh Rayabharam <anrayabh@...ux.microsoft.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Maxim Levitsky <mlevitsk@...hat.com>,
        linux-hyperv@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 09/25] KVM: VMX: nVMX: Support TSC scaling and
 PERF_GLOBAL_CTRL with enlightened VMCS

On Thu, Jul 28, 2022, Paolo Bonzini wrote:
> On 7/25/22 20:18, Sean Christopherson wrote:
> > > I kind of like the idea of having a two-dimensional array based on the enums
> > > instead of switch statements, so for now I'll keep Vitaly's enums.
> > I don't have a strong opinion on using a 2d array, but unless I'm missing something,
> > that's nowhere to be found in this patch.  IMO, having the enums without them
> > providing any unique value is silly and obfuscates the code.
> 
> Yeah, like this:
> 
> diff --git a/arch/x86/kvm/vmx/evmcs.c b/arch/x86/kvm/vmx/evmcs.c
> index d8da4026c93d..8055128d8638 100644
> --- a/arch/x86/kvm/vmx/evmcs.c
> +++ b/arch/x86/kvm/vmx/evmcs.c
> @@ -342,9 +342,10 @@ uint16_t nested_get_evmcs_version(struct kvm_vcpu *vcpu)
>  	return 0;
>  }
> -enum evmcs_v1_revision {
> +enum evmcs_revision {
>  	EVMCSv1_2016,
>  	EVMCSv1_2022,
> +	EVMCS_REVISION_MAX,
>  };
>  enum evmcs_unsupported_ctrl_type {
> @@ -353,13 +354,37 @@ enum evmcs_unsupported_ctrl_type {
>  	EVMCS_2NDEXEC,
>  	EVMCS_PINCTRL,
>  	EVMCS_VMFUNC,
> +	EVMCS_CTRL_MAX,
> +};
> +
> +static u32 evmcs_unsupported_ctls[EVMCS_CTRL_MAX][EVMCS_REVISION_MAX] = {

Can this be const?

> +	[EVMCS_EXIT_CTLS] = {
> +		[EVMCSv1_2016] = EVMCS1_UNSUPPORTED_VMEXIT_CTRL | VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL,
> +		[EVMCSv1_2022] = EVMCS1_UNSUPPORTED_VMEXIT_CTRL,
> +	},
> +	[EVMCS_ENTRY_CTLS] = {
> +		[EVMCSv1_2016] = EVMCS1_UNSUPPORTED_VMENTRY_CTRL | VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL,
> +		[EVMCSv1_2022] =  EVMCS1_UNSUPPORTED_VMENTRY_CTRL,
> +	},
> +	[EVMCS_2NDEXEC] = {
> +		[EVMCSv1_2016] = EVMCS1_UNSUPPORTED_2NDEXEC | SECONDARY_EXEC_TSC_SCALING,
> +		[EVMCSv1_2022] = EVMCS1_UNSUPPORTED_2NDEXEC,
> +	},
> +	[EVMCS_PINCTRL] = {
> +		[EVMCSv1_2016] = EVMCS1_UNSUPPORTED_PINCTRL,
> +		[EVMCSv1_2022] = EVMCS1_UNSUPPORTED_PINCTRL,
> +	},
> +	[EVMCS_VMFUNC] = {
> +		[EVMCSv1_2016] = EVMCS1_UNSUPPORTED_VMFUNC,
> +		[EVMCSv1_2022] = EVMCS1_UNSUPPORTED_VMFUNC,
> +	},
>  };

...

> +	return evmcs_unsupported_ctls[ctrl_type][evmcs_rev];
>  }

The only flaw in this is if KVM gets handed a CPUID model that enumerates support
for 2025 (or whenever the next update comes) but not 2022.  Hmm, though if Microsoft
defines each new "version" as a full superset, then even that theoretical bug goes
away.  I'm happy to be optimistic for once and give this a shot.  I definitely like
that it makes it easier to see the deltas between versions.