Linux-kselftest Archive on lore.kernel.org
 help / color / Atom feed
From: Borislav Petkov <bp@alien8.de>
To: Sean Christopherson <sean.j.christopherson@intel.com>
Cc: "Thomas Gleixner" <tglx@linutronix.de>,
	"Ingo Molnar" <mingo@redhat.com>,
	x86@kernel.org, "H. Peter Anvin" <hpa@zytor.com>,
	"Peter Zijlstra" <peterz@infradead.org>,
	"Arnaldo Carvalho de Melo" <acme@kernel.org>,
	"Mark Rutland" <mark.rutland@arm.com>,
	"Alexander Shishkin" <alexander.shishkin@linux.intel.com>,
	"Jiri Olsa" <jolsa@redhat.com>,
	"Namhyung Kim" <namhyung@kernel.org>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Radim Krčmář" <rkrcmar@redhat.com>,
	"Vitaly Kuznetsov" <vkuznets@redhat.com>,
	"Wanpeng Li" <wanpengli@tencent.com>,
	"Jim Mattson" <jmattson@google.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	"Tony Luck" <tony.luck@intel.com>,
	"Tony W Wang-oc" <TonyWWang-oc@zhaoxin.com>,
	"Shuah Khan" <shuah@kernel.org>,
	linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
	linux-edac@vger.kernel.org, linux-kselftest@vger.kernel.org,
	"Jarkko Sakkinen" <jarkko.sakkinen@linux.intel.com>
Subject: Re: [PATCH v3 12/19] x86/vmx: Introduce VMX_FEATURES_*
Date: Thu, 21 Nov 2019 17:52:58 +0100
Message-ID: <20191121165250.GK6540@zn.tnic> (raw)
In-Reply-To: <20191119031240.7779-13-sean.j.christopherson@intel.com>

On Mon, Nov 18, 2019 at 07:12:33PM -0800, Sean Christopherson wrote:
> Add a VMX specific variant of X86_FEATURE_* flags, which will eventually
> supplant the synthetic VMX flags defined in cpufeatures word 8.  Use the
> Intel-defined layouts for the major VMX execution controls so that their
> word entries can be directly populated from their respective MSRs, and
> so that the VMX_FEATURE_* flags can be used to define the existing bit
> definitions in asm/vmx.h, i.e. force developers to define a VMX_FEATURE
> flag when adding support for a new hardware feature.
> 
> The majority of Intel's (and compatible CPU's) VMX capabilities are
> enumerated via MSRs and not CPUID, i.e. querying /proc/cpuinfo doesn't
> naturally provide any insight into the virtualization capabilities of
> VMX enabled CPUs.  Commit e38e05a85828d ("x86: extended "flags" to show
> virtualization HW feature in /proc/cpuinfo") attempted to address the
> issue by synthesizing select VMX features into a Linux-defined word in
> cpufeatures.
> 
> The synthetic cpufeatures approach has several flaws:
> 
>   - The set of synthesized VMX flags has become extremely stale with
>     respect to the full set of VMX features, e.g. only one new flag
>     (EPT A/D) has been added in the the decade since the introduction of
>     the synthetic VMX features.  Failure to keep the VMX flags up to
>     date is likely due to the lack of a mechanism that forces developers
>     to consider whether or not a new feature is worth reporting.
> 
>   - The synthetic flags may incorrectly be misinterpreted as affecting
>     kernel behavior, i.e. KVM, the kernel's sole consumer of VMX,
>     completely ignores the synthetic flags.
> 
>   - New CPU vendors that support VMX have duplicated the hideous code
>     that propagates VMX features from MSRs to cpufeatures.  Bringing the
>     synthetic VMX flags up to date would exacerbate the copy+paste
>     trainwreck.
> 
> Define separate VMX_FEATURE flags to set the stage for enumerating VMX
> capabilities outside of the cpu_has() framework, and for adding
> functional usage of VMX_FEATURE_* to help ensure the features reported
> via /proc/cpuinfo is up to date with respect to kernel recognition of
> VMX capabilities.

That's all fine and good but who's going to use those feature bits?
Or are we reporting them just for the sake of it? Because if only
that, then it is not worth the effort. Sure, I don't mind extending
the framework so that you can use cpu_has() for VMX features but the
/proc/cpuinfo angle is not clear to me.

Especially since you're hiding most of them with the "" prepended in the
define comment.

> Note, the displayed names 'vnmi', 'tpr_shadow' and 'flexpriority' are
> retained for backwards compatibility with the existing ABI.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> ---
>  MAINTAINERS                        |  2 +-
>  arch/x86/include/asm/processor.h   |  1 +
>  arch/x86/include/asm/vmxfeatures.h | 81 ++++++++++++++++++++++++++++++
>  3 files changed, 83 insertions(+), 1 deletion(-)
>  create mode 100644 arch/x86/include/asm/vmxfeatures.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index df711965c377..6b736e78ee9e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9009,7 +9009,7 @@ F:	arch/x86/include/uapi/asm/svm.h
>  F:	arch/x86/include/asm/kvm*
>  F:	arch/x86/include/asm/pvclock-abi.h
>  F:	arch/x86/include/asm/svm.h
> -F:	arch/x86/include/asm/vmx.h
> +F:	arch/x86/include/asm/vmx*.h
>  F:	arch/x86/kernel/kvm.c
>  F:	arch/x86/kernel/kvmclock.c
>  
> diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
> index b4e29d8b9e5a..772de8917430 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -25,6 +25,7 @@ struct vm86;
>  #include <asm/special_insns.h>
>  #include <asm/fpu/types.h>
>  #include <asm/unwind_hints.h>
> +#include <asm/vmxfeatures.h>
>  
>  #include <linux/personality.h>
>  #include <linux/cache.h>
> diff --git a/arch/x86/include/asm/vmxfeatures.h b/arch/x86/include/asm/vmxfeatures.h
> new file mode 100644
> index 000000000000..aea39b9f1587
> --- /dev/null
> +++ b/arch/x86/include/asm/vmxfeatures.h
> @@ -0,0 +1,81 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_X86_VMXFEATURES_H
> +#define _ASM_X86_VMXFEATURES_H
> +
> +/*
> + * Note: If the comment begins with a quoted string, that string is used
> + * in /proc/cpuinfo instead of the macro name.  If the string is "",
> + * this feature bit is not displayed in /proc/cpuinfo at all.
> + */
> +
> +/* Pin-Based VM-Execution Controls, EPT/VPID, APIC and VM-Functions, word 0 */
> +#define VMX_FEATURE_INTR_EXITING	( 0*32+  0) /* "" VM-Exit on vectored interrupts */
> +#define VMX_FEATURE_NMI_EXITING		( 0*32+  3) /* "" VM-Exit on NMIs */
> +#define VMX_FEATURE_VIRTUAL_NMIS	( 0*32+  5) /* "vnmi" NMI virtualization */
> +#define VMX_FEATURE_PREEMPTION_TIMER	( 0*32+  6) /* VMX Preemption Timer */

You really wanna have "preemption_timer" in /proc/cpuinfo? That should
at least say vmx-something, if it should be visible there at all.

> +#define VMX_FEATURE_POSTED_INTR		( 0*32+  7) /* Posted Interrupts */

Same here.

In general, the questions stand for all those feature bits which will be
visible in /proc/cpuinfo.

1. Which to show and why?

2. Who's going to use them?

3. If show and dumping them together with the other feature flags, have
their name be proper (vmx-prefixed etc).

> +/* EPT/VPID features, scattered to bits 16-23 */
> +#define VMX_FEATURE_INVVPID	        ( 0*32+ 16) /* INVVPID is supported */
> +#define VMX_FEATURE_EPT_EXECUTE_ONLY	( 0*32+ 17) /* "ept_x_only" EPT entries can be execute only */
> +#define VMX_FEATURE_EPT_AD      	( 0*32+ 18) /* EPT Accessed/Dirty bits */
> +#define VMX_FEATURE_EPT_1GB      	( 0*32+ 19) /* 1GB EPT pages */
			      ^^^^^^^^^^^^

There are some spaces that need to be converted to tabs here.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

  reply index

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-19  3:12 [PATCH v3 00/19] x86/cpu: Clean up handling of VMX features Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 01/19] x86/msr-index: Clean up bit defines for IA32_FEATURE_CONTROL MSR Sean Christopherson
2019-11-19 11:15   ` Borislav Petkov
2019-11-19 23:18     ` Sean Christopherson
2019-11-20 17:48       ` Borislav Petkov
2019-11-21  9:46   ` Jarkko Sakkinen
2019-11-21 22:14     ` Sean Christopherson
2019-11-29 21:06       ` Jarkko Sakkinen
2019-11-29 21:11         ` Jarkko Sakkinen
2019-11-19  3:12 ` [PATCH v3 02/19] selftests: kvm: Replace manual MSR defs with common msr-index.h Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 03/19] tools arch x86: Sync msr-index.h from kernel sources Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 04/19] x86/intel: Initialize IA32_FEATURE_CONTROL MSR at boot Sean Christopherson
2019-11-19  4:41   ` Kai Huang
2019-11-19  5:03     ` Sean Christopherson
2019-11-21 10:39   ` Jarkko Sakkinen
2019-11-21 10:41     ` Jarkko Sakkinen
2019-11-21 11:05       ` Borislav Petkov
2019-11-21 22:12         ` Sean Christopherson
2019-11-22 12:34           ` Borislav Petkov
2019-11-19  3:12 ` [PATCH v3 05/19] x86/mce: WARN once if IA32_FEATURE_CONTROL MSR is left unlocked Sean Christopherson
2019-11-21 10:45   ` Jarkko Sakkinen
2019-11-19  3:12 ` [PATCH v3 06/19] x86/centaur: Use common IA32_FEATURE_CONTROL MSR initialization Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 07/19] x86/zhaoxin: " Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 08/19] KVM: VMX: Drop initialization of IA32_FEATURE_CONTROL MSR Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 09/19] x86/cpu: Clear VMX feature flag if VMX is not fully enabled Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 10/19] KVM: VMX: Use VMX feature flag to query BIOS enabling Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 11/19] KVM: VMX: Check for full VMX support when verifying CPU compatibility Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 12/19] x86/vmx: Introduce VMX_FEATURES_* Sean Christopherson
2019-11-21 16:52   ` Borislav Petkov [this message]
2019-11-21 21:50     ` Sean Christopherson
2019-11-22 18:36       ` Borislav Petkov
2019-11-22 19:09         ` Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 13/19] x86/cpu: Detect VMX features on Intel, Centaur and Zhaoxin CPUs Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 14/19] x86/cpu: Print VMX flags in /proc/cpuinfo using VMX_FEATURES_* Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 15/19] x86/cpufeatures: Drop synthetic VMX feature flags Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 16/19] KVM: VMX: Use VMX_FEATURE_* flags to define VMCS control bits Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 17/19] x86/cpufeatures: Clean up synthetic virtualization flags Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 18/19] perf/x86: Provide stubs of KVM helpers for non-Intel CPUs Sean Christopherson
2019-11-19  3:12 ` [PATCH v3 19/19] KVM: VMX: Allow KVM_INTEL when building for Centaur and/or Zhaoxin CPUs Sean Christopherson

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191121165250.GK6540@zn.tnic \
    --to=bp@alien8.de \
    --cc=TonyWWang-oc@zhaoxin.com \
    --cc=acme@kernel.org \
    --cc=alexander.shishkin@linux.intel.com \
    --cc=hpa@zytor.com \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jmattson@google.com \
    --cc=jolsa@redhat.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=namhyung@kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rkrcmar@redhat.com \
    --cc=sean.j.christopherson@intel.com \
    --cc=shuah@kernel.org \
    --cc=tglx@linutronix.de \
    --cc=tony.luck@intel.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-kselftest Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-kselftest/0 linux-kselftest/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-kselftest linux-kselftest/ https://lore.kernel.org/linux-kselftest \
		linux-kselftest@vger.kernel.org
	public-inbox-index linux-kselftest

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kselftest


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git