diff mbox series

[v2,22/28] KVM: VMX: Clear controls obsoleted by EPT at runtime, not setup

Message ID 20220629150625.238286-23-vkuznets@redhat.com (mailing list archive)
State New, archived
Headers show
Series KVM: VMX: Support TscScaling and EnclsExitingBitmap with eVMCS + use vmcs_config for L1 VMX MSRs | expand

Commit Message

Vitaly Kuznetsov June 29, 2022, 3:06 p.m. UTC
From: Sean Christopherson <seanjc@google.com>

Clear the CR3 and INVLPG interception controls at runtime based on
whether or not EPT is being _used_, as opposed to clearing the bits at
setup if EPT is _supported_ in hardware, and then restoring them when EPT
is not used.  Not mucking with the base config will allow using the base
config as the starting point for emulating the VMX capability MSRs.

Signed-off-by: Sean Christopherson <seanjc@google.com>
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
 arch/x86/kvm/vmx/vmx.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

Comments

Jim Mattson July 1, 2022, 4:11 p.m. UTC | #1
On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>
> From: Sean Christopherson <seanjc@google.com>
>
> Clear the CR3 and INVLPG interception controls at runtime based on
> whether or not EPT is being _used_, as opposed to clearing the bits at
> setup if EPT is _supported_ in hardware, and then restoring them when EPT
> is not used.  Not mucking with the base config will allow using the base
> config as the starting point for emulating the VMX capability MSRs.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
Nit: These controls aren't "obsoleted" by EPT; they're just no longer required.

Reviewed-by: Jim Mattson <jmattson@google.com>
Vitaly Kuznetsov July 7, 2022, 2:57 p.m. UTC | #2
Jim Mattson <jmattson@google.com> writes:

> On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>>
>> From: Sean Christopherson <seanjc@google.com>
>>
>> Clear the CR3 and INVLPG interception controls at runtime based on
>> whether or not EPT is being _used_, as opposed to clearing the bits at
>> setup if EPT is _supported_ in hardware, and then restoring them when EPT
>> is not used.  Not mucking with the base config will allow using the base
>> config as the starting point for emulating the VMX capability MSRs.
>>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> Nit: These controls aren't "obsoleted" by EPT; they're just no longer
> required.

Sean,

I'm going to update the subject line to "KVM: VMX: Clear controls
unneded with EPT at runtime, not setup" retaining your authorship in v3
(if there are no objections, of course).

>
> Reviewed-by: Jim Mattson <jmattson@google.com>
>

Thanks!
Sean Christopherson July 7, 2022, 7:30 p.m. UTC | #3
On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
> Jim Mattson <jmattson@google.com> writes:
> 
> > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> >>
> >> From: Sean Christopherson <seanjc@google.com>
> >>
> >> Clear the CR3 and INVLPG interception controls at runtime based on
> >> whether or not EPT is being _used_, as opposed to clearing the bits at
> >> setup if EPT is _supported_ in hardware, and then restoring them when EPT
> >> is not used.  Not mucking with the base config will allow using the base
> >> config as the starting point for emulating the VMX capability MSRs.
> >>
> >> Signed-off-by: Sean Christopherson <seanjc@google.com>
> >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > Nit: These controls aren't "obsoleted" by EPT; they're just no longer
> > required.

Isn't that the definition of "obsolete"?  They're "no longer in use" when KVM
enables EPT.

> I'm going to update the subject line to "KVM: VMX: Clear controls
> unneded with EPT at runtime, not setup" retaining your authorship in v3

That's fine, though s/unneded/unneeded.
Jim Mattson July 7, 2022, 9:32 p.m. UTC | #4
On Thu, Jul 7, 2022 at 12:30 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
> > Jim Mattson <jmattson@google.com> writes:
> >
> > > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > >>
> > >> From: Sean Christopherson <seanjc@google.com>
> > >>
> > >> Clear the CR3 and INVLPG interception controls at runtime based on
> > >> whether or not EPT is being _used_, as opposed to clearing the bits at
> > >> setup if EPT is _supported_ in hardware, and then restoring them when EPT
> > >> is not used.  Not mucking with the base config will allow using the base
> > >> config as the starting point for emulating the VMX capability MSRs.
> > >>
> > >> Signed-off-by: Sean Christopherson <seanjc@google.com>
> > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > Nit: These controls aren't "obsoleted" by EPT; they're just no longer
> > > required.
>
> Isn't that the definition of "obsolete"?  They're "no longer in use" when KVM
> enables EPT.

There are still reasons to use them aside from shadow page table
maintenance. For example, malware analysis may be interested in
intercepting CR3 changes to track process context (and to
enable/disable costly monitoring). EPT doesn't render these events
"obsolete," because you can't intercept these events using EPT.

> > I'm going to update the subject line to "KVM: VMX: Clear controls
> > unneded with EPT at runtime, not setup" retaining your authorship in v3
>
> That's fine, though s/unneded/unneeded.
Sean Christopherson July 7, 2022, 9:39 p.m. UTC | #5
On Thu, Jul 07, 2022, Jim Mattson wrote:
> On Thu, Jul 7, 2022 at 12:30 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
> > > Jim Mattson <jmattson@google.com> writes:
> > >
> > > > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > > >>
> > > >> From: Sean Christopherson <seanjc@google.com>
> > > >>
> > > >> Clear the CR3 and INVLPG interception controls at runtime based on
> > > >> whether or not EPT is being _used_, as opposed to clearing the bits at
> > > >> setup if EPT is _supported_ in hardware, and then restoring them when EPT
> > > >> is not used.  Not mucking with the base config will allow using the base
> > > >> config as the starting point for emulating the VMX capability MSRs.
> > > >>
> > > >> Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > Nit: These controls aren't "obsoleted" by EPT; they're just no longer
> > > > required.

Actually, they're still required if unrestricted guest isn't supported.

> > Isn't that the definition of "obsolete"?  They're "no longer in use" when KVM
> > enables EPT.
> 
> There are still reasons to use them aside from shadow page table
> maintenance. For example, malware analysis may be interested in
> intercepting CR3 changes to track process context (and to
> enable/disable costly monitoring). EPT doesn't render these events
> "obsolete," because you can't intercept these events using EPT.

Fair enough, I was using "EPT" in the "KVM is using EPT" sense.  But even that's
wrong as KVM intercepts CR3 accesses when EPT is enabled, but unrestricted guest
is disabled and the guest disables paging.

Vitaly, since the CR3 fields are still technically "needed", maybe just be
explicit?

  KVM: VMX: Adjust CR3/INVPLG interception for EPT=y at runtime, not setup
Jim Mattson July 7, 2022, 11:12 p.m. UTC | #6
On Thu, Jul 7, 2022 at 2:39 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Thu, Jul 07, 2022, Jim Mattson wrote:
> > On Thu, Jul 7, 2022 at 12:30 PM Sean Christopherson <seanjc@google.com> wrote:
> > >
> > > On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
> > > > Jim Mattson <jmattson@google.com> writes:
> > > >
> > > > > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
> > > > >>
> > > > >> From: Sean Christopherson <seanjc@google.com>
> > > > >>
> > > > >> Clear the CR3 and INVLPG interception controls at runtime based on
> > > > >> whether or not EPT is being _used_, as opposed to clearing the bits at
> > > > >> setup if EPT is _supported_ in hardware, and then restoring them when EPT
> > > > >> is not used.  Not mucking with the base config will allow using the base
> > > > >> config as the starting point for emulating the VMX capability MSRs.
> > > > >>
> > > > >> Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> > > > > Nit: These controls aren't "obsoleted" by EPT; they're just no longer
> > > > > required.
>
> Actually, they're still required if unrestricted guest isn't supported.
>
> > > Isn't that the definition of "obsolete"?  They're "no longer in use" when KVM
> > > enables EPT.
> >
> > There are still reasons to use them aside from shadow page table
> > maintenance. For example, malware analysis may be interested in
> > intercepting CR3 changes to track process context (and to
> > enable/disable costly monitoring). EPT doesn't render these events
> > "obsolete," because you can't intercept these events using EPT.
>
> Fair enough, I was using "EPT" in the "KVM is using EPT" sense.  But even that's
> wrong as KVM intercepts CR3 accesses when EPT is enabled, but unrestricted guest
> is disabled and the guest disables paging.

MOV-to-CR3 is also a required intercept for allow_smaller_maxphyaddr,
when the guest is in PAE mode. So, that one, at least, isn't anywhere
near obsolete. :-)

> Vitaly, since the CR3 fields are still technically "needed", maybe just be
> explicit?
>
>   KVM: VMX: Adjust CR3/INVPLG interception for EPT=y at runtime, not setup
Vitaly Kuznetsov July 8, 2022, 7:55 a.m. UTC | #7
Sean Christopherson <seanjc@google.com> writes:

> On Thu, Jul 07, 2022, Jim Mattson wrote:
>> On Thu, Jul 7, 2022 at 12:30 PM Sean Christopherson <seanjc@google.com> wrote:
>> >
>> > On Thu, Jul 07, 2022, Vitaly Kuznetsov wrote:
>> > > Jim Mattson <jmattson@google.com> writes:
>> > >
>> > > > On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@redhat.com> wrote:
>> > > >>
>> > > >> From: Sean Christopherson <seanjc@google.com>
>> > > >>
>> > > >> Clear the CR3 and INVLPG interception controls at runtime based on
>> > > >> whether or not EPT is being _used_, as opposed to clearing the bits at
>> > > >> setup if EPT is _supported_ in hardware, and then restoring them when EPT
>> > > >> is not used.  Not mucking with the base config will allow using the base
>> > > >> config as the starting point for emulating the VMX capability MSRs.
>> > > >>
>> > > >> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> > > >> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
>> > > > Nit: These controls aren't "obsoleted" by EPT; they're just no longer
>> > > > required.
>
> Actually, they're still required if unrestricted guest isn't supported.
>
>> > Isn't that the definition of "obsolete"?  They're "no longer in use" when KVM
>> > enables EPT.
>> 
>> There are still reasons to use them aside from shadow page table
>> maintenance. For example, malware analysis may be interested in
>> intercepting CR3 changes to track process context (and to
>> enable/disable costly monitoring). EPT doesn't render these events
>> "obsolete," because you can't intercept these events using EPT.
>
> Fair enough, I was using "EPT" in the "KVM is using EPT" sense.  But even that's
> wrong as KVM intercepts CR3 accesses when EPT is enabled, but unrestricted guest
> is disabled and the guest disables paging.
>
> Vitaly, since the CR3 fields are still technically "needed", maybe just be
> explicit?
>
>   KVM: VMX: Adjust CR3/INVPLG interception for EPT=y at runtime, not setup
>

Sounds good, adjusted!
diff mbox series

Patch

diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
index e5e4383d0cff..fb58b0be953d 100644
--- a/arch/x86/kvm/vmx/vmx.c
+++ b/arch/x86/kvm/vmx/vmx.c
@@ -2501,13 +2501,8 @@  static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
 	rdmsr_safe(MSR_IA32_VMX_EPT_VPID_CAP,
 		&vmx_cap->ept, &vmx_cap->vpid);
 
-	if (_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) {
-		/* CR3 accesses and invlpg don't need to cause VM Exits when EPT
-		   enabled */
-		_cpu_based_exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
-					     CPU_BASED_CR3_STORE_EXITING |
-					     CPU_BASED_INVLPG_EXITING);
-	} else if (vmx_cap->ept) {
+	if (!(_cpu_based_2nd_exec_control & SECONDARY_EXEC_ENABLE_EPT) &&
+	    vmx_cap->ept) {
 		pr_warn_once("EPT CAP should not exist if not support "
 				"1-setting enable EPT VM-execution control\n");
 
@@ -4268,10 +4263,11 @@  static u32 vmx_exec_control(struct vcpu_vmx *vmx)
 		exec_control |= CPU_BASED_CR8_STORE_EXITING |
 				CPU_BASED_CR8_LOAD_EXITING;
 #endif
-	if (!enable_ept)
-		exec_control |= CPU_BASED_CR3_STORE_EXITING |
-				CPU_BASED_CR3_LOAD_EXITING  |
-				CPU_BASED_INVLPG_EXITING;
+	/* No need to intercept CR3 access or INVPLG when using EPT. */
+	if (enable_ept)
+		exec_control &= ~(CPU_BASED_CR3_LOAD_EXITING |
+				  CPU_BASED_CR3_STORE_EXITING |
+				  CPU_BASED_INVLPG_EXITING);
 	if (kvm_mwait_in_guest(vmx->vcpu.kvm))
 		exec_control &= ~(CPU_BASED_MWAIT_EXITING |
 				CPU_BASED_MONITOR_EXITING);