[PATCH] proc: added ept_ad flag to /proc/cpuinfo

[PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Peter Shier]

From: Peter Feiner <pfeiner@google.com>

The Intel Haswell architecture has an EPT feature whereby the access &
dirty bits in EPT entries are updated without taking a guest exit.
This patch adds the "ept_ad" flag to /proc/cpuinfo if this feature is
available.

Signed-off-by: Peter Feiner <pfeiner@google.com>
Signed-off-by: Peter Shier <pshier@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>
---
 arch/x86/include/asm/cpufeatures.h |  2 +-
 arch/x86/kernel/cpu/intel.c        | 10 +++++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 5701f5cecd312..7fff98fa58558 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -229,7 +229,7 @@
 
 #define X86_FEATURE_VMMCALL		( 8*32+15) /* Prefer VMMCALL to VMCALL */
 #define X86_FEATURE_XENPV		( 8*32+16) /* "" Xen paravirtual guest */
-
+#define X86_FEATURE_EPT_AD		( 8*32+17) /* Intel Extended Page Table access-dirty bit */
 
 /* Intel-defined CPU features, CPUID level 0x00000007:0 (EBX), word 9 */
 #define X86_FEATURE_FSGSBASE		( 9*32+ 0) /* RDFSBASE, WRFSBASE, RDGSBASE, WRGSBASE instructions*/
diff --git a/arch/x86/kernel/cpu/intel.c b/arch/x86/kernel/cpu/intel.c
index eb75564f2d257..c050cd6066af0 100644
--- a/arch/x86/kernel/cpu/intel.c
+++ b/arch/x86/kernel/cpu/intel.c
@@ -465,14 +465,17 @@ static void detect_vmx_virtcap(struct cpuinfo_x86 *c)
 #define X86_VMX_FEATURE_PROC_CTLS2_VIRT_APIC	0x00000001
 #define X86_VMX_FEATURE_PROC_CTLS2_EPT		0x00000002
 #define X86_VMX_FEATURE_PROC_CTLS2_VPID		0x00000020
+#define x86_VMX_FEATURE_EPT_CAP_AD		0x00200000
 
 	u32 vmx_msr_low, vmx_msr_high, msr_ctl, msr_ctl2;
+	u32 msr_vpid_cap, msr_ept_cap;
 
 	clear_cpu_cap(c, X86_FEATURE_TPR_SHADOW);
 	clear_cpu_cap(c, X86_FEATURE_VNMI);
 	clear_cpu_cap(c, X86_FEATURE_FLEXPRIORITY);
 	clear_cpu_cap(c, X86_FEATURE_EPT);
 	clear_cpu_cap(c, X86_FEATURE_VPID);
+	clear_cpu_cap(c, X86_FEATURE_EPT_AD);
 
 	rdmsr(MSR_IA32_VMX_PROCBASED_CTLS, vmx_msr_low, vmx_msr_high);
 	msr_ctl = vmx_msr_high | vmx_msr_low;
@@ -487,8 +490,13 @@ static void detect_vmx_virtcap(struct cpuinfo_x86 *c)
 		if ((msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_VIRT_APIC) &&
 		    (msr_ctl & X86_VMX_FEATURE_PROC_CTLS_TPR_SHADOW))
 			set_cpu_cap(c, X86_FEATURE_FLEXPRIORITY);
-		if (msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_EPT)
+		if (msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_EPT) {
 			set_cpu_cap(c, X86_FEATURE_EPT);
+			rdmsr(MSR_IA32_VMX_EPT_VPID_CAP,
+			      msr_ept_cap, msr_vpid_cap);
+			if (msr_ept_cap & x86_VMX_FEATURE_EPT_CAP_AD)
+				set_cpu_cap(c, X86_FEATURE_EPT_AD);
+		}
 		if (msr_ctl2 & X86_VMX_FEATURE_PROC_CTLS2_VPID)
 			set_cpu_cap(c, X86_FEATURE_VPID);
 	}
-- 
2.18.0.233.g985f88cf7e-goog

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Thomas Gleixner]

On Mon, 30 Jul 2018, Peter Shier wrote:

> Subject: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

The 'proc:' prefix is misleading here. x86/cpufeatures is the right
choice. The /proc/cpuinfo display is a side effect.

Also please avoid 'added'. Changelogs should be written in imperative
mood. Something like this:

 x86/cpufeatures: Add EPT_AD feature bit
 
> The Intel Haswell architecture has an EPT feature whereby the access &
> dirty bits in EPT entries are updated without taking a guest exit.

Why would this be Haswell specific?

Aside of that I don't see what this has to do with exits. From the SDM:

  " * If bit 21 is read as 1, accessed and dirty flags for EPT are
      supported (see Section 28.2.4)"

And nothing in 28.2.4 says anything about exits. It's all about whether the
feature is supported or not. If it is supported it can be enabled in EPTP.

> This patch adds the "ept_ad" flag to /proc/cpuinfo if this feature is
> available.

See Documentation/process/submitting-patches.rst and search for 'This
patch'.

The other question is why is this new feature bit not used in the VMX code?
It needs to be checked to enable the AD bit in EPTP ....

Thanks,

	tglx

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Peter Shier]

Thank you Thomas. Wording issues understood and will post a new patch
with updated subject.

Re goals: purpose is to expose feature bit with side effect of
"ept_ad" in /proc/cpuinfo and is not necessarily related to VMX code.
We are upstreaming some internal patches that we think would be
generally useful and I will be careful from now on to remove any
wording that is not publicly relevant.

On Mon, Jul 30, 2018 at 3:12 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Mon, 30 Jul 2018, Peter Shier wrote:
>
> > Subject: [PATCH] proc: added ept_ad flag to /proc/cpuinfo
>
> The 'proc:' prefix is misleading here. x86/cpufeatures is the right
> choice. The /proc/cpuinfo display is a side effect.
>
> Also please avoid 'added'. Changelogs should be written in imperative
> mood. Something like this:
>
>  x86/cpufeatures: Add EPT_AD feature bit
>
> > The Intel Haswell architecture has an EPT feature whereby the access &
> > dirty bits in EPT entries are updated without taking a guest exit.
>
> Why would this be Haswell specific?
>
> Aside of that I don't see what this has to do with exits. From the SDM:
>
>   " * If bit 21 is read as 1, accessed and dirty flags for EPT are
>       supported (see Section 28.2.4)"
>
> And nothing in 28.2.4 says anything about exits. It's all about whether the
> feature is supported or not. If it is supported it can be enabled in EPTP.
>
> > This patch adds the "ept_ad" flag to /proc/cpuinfo if this feature is
> > available.
>
> See Documentation/process/submitting-patches.rst and search for 'This
> patch'.
>
> The other question is why is this new feature bit not used in the VMX code?
> It needs to be checked to enable the AD bit in EPTP ....
>
> Thanks,
>
>         tglx

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Thomas Gleixner]

A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

On Wed, 1 Aug 2018, Peter Shier wrote:

> Thank you Thomas. Wording issues understood and will post a new patch
> with updated subject.
> 
> Re goals: purpose is to expose feature bit with side effect of
> "ept_ad" in /proc/cpuinfo and is not necessarily related to VMX code.
> 
> We are upstreaming some internal patches that we think would be
> generally useful and I will be careful from now on to remove any
> wording that is not publicly relevant.

Well, it does not matter what your goals are. If you add new functionality
and there is an indication that it might be useful in other places, then
looking into that is not really optional. Kernel development works because
people work together and look over the brim of their own tea cup, otherwise
we would end up with a unmaintainable steaming pile of s....

'Not my goal' surely does not qualify as a proper answer to a sensible
technical question.

Thanks,

	tglx

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Peter Shier]

Thank you Thomas. I missed what I think is your fundamental point
regarding duplication created by this patch between CPU feature bits
and KVM's consumption of the IA32_VMX_EPT_VPID_CAP MSR.

Should all the features in this MSR be exposed via CPU feature bits
and should KVM consume only from there rather than reading the MSR
directly? There are 16 feature bits in the MSR per SDM Vol 3d section
A.10.

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Thomas Gleixner]

Peter,

On Thu, 2 Aug 2018, Peter Shier wrote:
> Thank you Thomas. I missed what I think is your fundamental point
> regarding duplication created by this patch between CPU feature bits
> and KVM's consumption of the IA32_VMX_EPT_VPID_CAP MSR.
> 
> Should all the features in this MSR be exposed via CPU feature bits
> and should KVM consume only from there rather than reading the MSR
> directly? There are 16 feature bits in the MSR per SDM Vol 3d section
> A.10.

Uuurg. Probably not, unless they are important somehow for similar reasons
like the one you are exposing. I assume they are not, so using the MSR and
picking all bits directly from there is ok.

But thanks for taking a look as noew we at least know why it doesn't make
sense to convert VMX over as it has to read the MSR anyway.

Thanks,

	tglx

Re: [PATCH] proc: added ept_ad flag to /proc/cpuinfo

[Paolo Bonzini]

On 02/08/2018 21:33, Peter Shier wrote:
> 
>> > The Intel Haswell architecture has an EPT feature whereby the access &
>> > dirty bits in EPT entries are updated without taking a guest exit.
>>
>> Why would this be Haswell specific?
>>
>> Aside of that I don't see what this has to do with exits. From the SDM:
>>
>>   " * If bit 21 is read as 1, accessed and dirty flags for EPT are
>>       supported (see Section 28.2.4)"
>>
>> And nothing in 28.2.4 says anything about exits. It's all about whether the
>> feature is supported or not. If it is supported it can be enabled in EPTP.

Right, if it's not supported KVM instead has to use the R/W/X permission
bits to emulate accessed and dirty bits, taking an exit on every update.

But then it seems to me that you're more interested in the KVM behavior
than the processor behavior, and then the information you need is
already in the /sys/modules/kvm_intel/parameters directory.  (Most
processor features have a parameter so that it's possible to test code
paths for old processors).  I don't particularly see a need to add them
in /proc/cpuinfo.

Paolo

> Thank you Thomas. I missed what I think is your fundamental point
> regarding duplication created by this patch between CPU feature bits
> and KVM's consumption of the IA32_VMX_EPT_VPID_CAP MSR.
> 
> Should all the features in this MSR be exposed via CPU feature bits
> and should KVM consume only from there rather than reading the MSR
> directly? There are 16 feature bits in the MSR per SDM Vol 3d section
> A.10.
>