Re: Nested kvm_intel broken on pre 3.3 hosts

From: Stefan Bader <stefan.bader@canonical.com>
To: Avi Kivity <avi@redhat.com>
Cc: nyh@math.technion.ac.il, Gleb Natapov <gleb@redhat.com>,
	Andy Whitcroft <apw@canonical.com>,
	kvm@vger.kernel.org
Subject: Re: Nested kvm_intel broken on pre 3.3 hosts
Date: Thu, 09 Aug 2012 09:13:45 +0200	[thread overview]
Message-ID: <50236329.2040509@canonical.com> (raw)
In-Reply-To: <501FD759.8030104@canonical.com>

[-- Attachment #1: Type: text/plain, Size: 5247 bytes --]

On 06.08.2012 16:40, Stefan Bader wrote:
> On 05.08.2012 11:18, Avi Kivity wrote:
>> On 08/03/2012 01:57 PM, Stefan Bader wrote:
>>>> No, you're backporting the entire feature.  All we need is to expose
>>>> RDPMC intercept to the guest.
>>>
>>> Oh well, I thought that was the thing you asked for...
>>
>> Sorry for being unclear.
>>
>>>
>>>> It should be sufficient to backport the bits in
>>>> nested_vmx_setup_ctls_msrs() and nested_vmx_exit_handled().
>>>
>>> Ok, how about that? It is probably wrong again, but at least it
>>> allows to load the kvm-intel module from within a nested guest
>>> and not having the feature pretend to fail seems the closest
>>> thing to do...
>>>
>>> ---
>>>
>>> From 0aeb99348363b7aeb2b0bd92428cb212159fa468 Mon Sep 17 00:00:00 2001
>>> From: Stefan Bader <stefan.bader@canonical.com>
>>> Date: Thu, 10 Nov 2011 14:57:25 +0200
>>> Subject: [PATCH] KVM: VMX: Fake intercept RDPMC
>>>
>>> Based on commit fee84b079d5ddee2247b5c1f53162c330c622902 upstream.
>>>
>>>   Intercept RDPMC and forward it to the PMU emulation code.
>>>
>>> But drop the requirement for the feature being present and instead
>>> of forwarding, cause a GP as if the call had failed.
>>>
>>> BugLink: http://bugs.launchpad.net/bugs/1031090
>>> Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
>>> ---
>>>  arch/x86/kvm/vmx.c |   10 ++++++++++
>>>  1 file changed, 10 insertions(+)
>>>
>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>> index 7315488..fc937f2 100644
>>> --- a/arch/x86/kvm/vmx.c
>>> +++ b/arch/x86/kvm/vmx.c
>>> @@ -1956,6 +1956,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
>>>  #endif
>>>  		CPU_BASED_MOV_DR_EXITING | CPU_BASED_UNCOND_IO_EXITING |
>>>  		CPU_BASED_USE_IO_BITMAPS | CPU_BASED_MONITOR_EXITING |
>>> +		CPU_BASED_RDPMC_EXITING |
>>>  		CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
>>>  	/*
>>>  	 * We can allow some features even when not supported by the
>>> @@ -4613,6 +4614,14 @@ static int handle_invlpg(struct kvm_vcpu *vcpu)
>>>  	return 1;
>>>  }
>>>  
>>> +static int handle_rdpmc(struct kvm_vcpu *vcpu)
>>> +{
>>> +	/* Instead of implementing the feature, cause a GP */
>>> +	kvm_complete_insn_gp(vcpu, 1);
>>> +
>>> +	return 1;
>>> +}
>>
>> In fact this should never be called, since we never request RDPMC
>> exiting for L1.
>>
>>> +
>>>  static int handle_wbinvd(struct kvm_vcpu *vcpu)
>>>  {
>>>  	skip_emulated_instruction(vcpu);
>>> @@ -5563,6 +5572,7 @@ static int (*kvm_vmx_exit_handlers[])(struct kvm_vcpu *vcpu) = {
>>>  	[EXIT_REASON_HLT]                     = handle_halt,
>>>  	[EXIT_REASON_INVD]		      = handle_invd,
>>>  	[EXIT_REASON_INVLPG]		      = handle_invlpg,
>>> +	[EXIT_REASON_RDPMC]                   = handle_rdpmc,
>>>  	[EXIT_REASON_VMCALL]                  = handle_vmcall,
>>>  	[EXIT_REASON_VMCLEAR]	              = handle_vmclear,
>>>  	[EXIT_REASON_VMLAUNCH]                = handle_vmlaunch,
>>>
>>
>> Provided you backport the bit in nested_vmx_exit_handled().  That takes
>> the L2->L1 RDPMC exit and forwards it to L1.
>>
> The nested_vmx_exit_handled handled function is already there (in 3.1).
> 
> commit 644d711aa0e16111d8aba6d289caebec013e26ea
> Author: Nadav Har'El <nyh@il.ibm.com>
> Date:   Wed May 25 23:12:35 2011 +0300
> 
>     KVM: nVMX: Deciding if L0 or L1 should handle an L2 exit
> 
> Though I checked what happens if I drop anything beside the change in
> nested_vmx_setup_ctls_msrs. If running "perf test" (which does some
> RDPMC test) is sufficient to prove things are ok, then it seems to be
> good enough. That test fails already on the perf_event_open syscall
> (returning -ENOENT).
> 
> So this remaining (attached patch) change would be enough to allow
> the kvm_intel module get loaded from newer kernels on an 3.2 host.
> 
Avi, was the last version of the patch (only adding the flag to the nested MSRs)
good for submitting to stable from your point of view?

(inlining though this will be broken)
From b79a5f03b4d9a1a56949d6ef38fd4879ff1b8aee Mon Sep 17 00:00:00 2001
From: Stefan Bader <stefan.bader@canonical.com>
Date: Thu, 10 Nov 2011 14:57:25 +0200
Subject: [PATCH] KVM: VMX: Set CPU_BASED_RDPMC_EXITING for nested

Based on commit fee84b079d5ddee2247b5c1f53162c330c622902 upstream.

  Intercept RDPMC and forward it to the PMU emulation code.

Newer vmx support will only allow to load the kvm_intel module
if RDPMC_EXITING is supported. Even without the actual support
this part of the change is required on 3.2 hosts.

BugLink: http://bugs.launchpad.net/bugs/1031090
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 arch/x86/kvm/vmx.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 114fe29..94e6749 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -1957,6 +1957,7 @@ static __init void nested_vmx_setup_ctls_msrs(void)
 #endif
 		CPU_BASED_MOV_DR_EXITING | CPU_BASED_UNCOND_IO_EXITING |
 		CPU_BASED_USE_IO_BITMAPS | CPU_BASED_MONITOR_EXITING |
+		CPU_BASED_RDPMC_EXITING |
 		CPU_BASED_ACTIVATE_SECONDARY_CONTROLS;
 	/*
 	 * We can allow some features even when not supported by the


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 900 bytes --]
