From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kiszka Subject: Re: [PATCH] KVM: nVMX: Fully support of nested VMX preemption timer Date: Sun, 25 Aug 2013 09:28:34 +0200 Message-ID: <5219B222.5070204@web.de> References: <1377369850-18583-1-git-send-email-root@Blade1-01.Blade1-01> <5219A7BA.8050602@web.de> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="foaCPBE3noAXqXPi5RrTL9bmffUF3Igta" Cc: kvm , Gleb Natapov , Paolo Bonzini To: Arthur Chunqi Li Return-path: Received: from mout.web.de ([212.227.15.14]:49840 "EHLO mout.web.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751651Ab3HYH2i (ORCPT ); Sun, 25 Aug 2013 03:28:38 -0400 Received: from mchn199C.mchp.siemens.de ([95.157.58.223]) by smtp.web.de (mrweb103) with ESMTPSA (Nemesis) id 0ML8Sz-1VDDld0vjt-000McS for ; Sun, 25 Aug 2013 09:28:36 +0200 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --foaCPBE3noAXqXPi5RrTL9bmffUF3Igta Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 2013-08-25 09:24, Arthur Chunqi Li wrote: > On Sun, Aug 25, 2013 at 2:44 PM, Jan Kiszka wrote: >> On 2013-08-24 20:44, root wrote: >>> This patch contains the following two changes: >>> 1. Fix the bug in nested preemption timer support. If vmexit L2->L0 >>> with some reasons not emulated by L1, preemption timer value should >>> be save in such exits. >>> 2. Add support of "Save VMX-preemption timer value" VM-Exit controls >>> to nVMX. >>> >>> With this patch, nested VMX preemption timer features are fully >>> supported. >>> >>> Signed-off-by: Arthur Chunqi Li >>> --- >>> arch/x86/kvm/vmx.c | 30 +++++++++++++++++++++++++----- >>> 1 file changed, 25 insertions(+), 5 deletions(-) >>> >>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c >>> index 57b4e12..9579409 100644 >>> --- a/arch/x86/kvm/vmx.c >>> +++ b/arch/x86/kvm/vmx.c >>> @@ -2204,7 +2204,8 @@ static __init void nested_vmx_setup_ctls_msrs(v= oid) >>> #ifdef CONFIG_X86_64 >>> VM_EXIT_HOST_ADDR_SPACE_SIZE | >>> #endif >>> - VM_EXIT_LOAD_IA32_PAT | VM_EXIT_SAVE_IA32_PAT; >>> + VM_EXIT_LOAD_IA32_PAT | VM_EXIT_SAVE_IA32_PAT | >>> + VM_EXIT_SAVE_VMX_PREEMPTION_TIMER; >>> nested_vmx_exit_ctls_high |=3D (VM_EXIT_ALWAYSON_WITHOUT_TRUE_M= SR | >>> VM_EXIT_LOAD_IA32_EFER); >> >> In the absence of VM_EXIT_SAVE_VMX_PREEMPTION_TIMER, you need to hide >> PIN_BASED_VMX_PREEMPTION_TIMER from the guest as we cannot emulate its= >> behavior properly in that case. >> >>> >>> @@ -7578,9 +7579,14 @@ static void prepare_vmcs02(struct kvm_vcpu *vc= pu, struct vmcs12 *vmcs12) >>> (vmcs_config.pin_based_exec_ctrl | >>> vmcs12->pin_based_vm_exec_control)); >>> >>> - if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTIO= N_TIMER) >>> - vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, >>> - vmcs12->vmx_preemption_timer_value); >>> + if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTIO= N_TIMER) { >>> + if (vmcs12->vm_exit_controls & VM_EXIT_SAVE_VMX_PREEMPT= ION_TIMER) >>> + vmcs12->vmx_preemption_timer_value =3D >>> + vmcs_read32(VMX_PREEMPTION_TIMER_VALUE)= ; >>> + else >>> + vmcs_write32(VMX_PREEMPTION_TIMER_VALUE, >>> + vmcs12->vmx_preemption_timer_va= lue); >>> + } >> >> This is not correct. We still need to set the vmcs to >> vmx_preemption_timer_value. The difference is that, on exit from L2, >> vmx_preemption_timer_value has to be updated according to the saved >> hardware state. The corresponding code is missing in your patch so far= =2E >> >>> >>> /* >>> * Whether page-faults are trapped is determined by a combinati= on of >>> @@ -7690,7 +7696,11 @@ static void prepare_vmcs02(struct kvm_vcpu *vc= pu, struct vmcs12 *vmcs12) >>> * we should use its exit controls. Note that VM_EXIT_LOAD_IA32= _EFER >>> * bits are further modified by vmx_set_efer() below. >>> */ >>> - vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl); >>> + if (vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTIO= N_TIMER) >>> + vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl = | >>> + VM_EXIT_SAVE_VMX_PREEMPTION_TIMER); >>> + else >>> + vmcs_write32(VM_EXIT_CONTROLS, vmcs_config.vmexit_ctrl)= ; >> >> Let's prepare the value for VM_EXIT_CONTROLS in a local variable first= , >> then write it to the vmcs. >> >>> >>> /* vmcs12's VM_ENTRY_LOAD_IA32_EFER and VM_ENTRY_IA32E_MODE are= >>> * emulated by vmx_set_efer(), below. >>> @@ -7912,6 +7922,16 @@ static int nested_vmx_run(struct kvm_vcpu *vcp= u, bool launch) >>> } >>> >>> /* >>> + * If L2 support PIN_BASED_VMX_PREEMPTION_TIMER, L0 must suppor= t >>> + * VM_EXIT_SAVE_VMX_PREEMPTION_TIMER. >>> + */ >>> + if ((vmcs12->pin_based_vm_exec_control & PIN_BASED_VMX_PREEMPTI= ON_TIMER) && >>> + !(nested_vmx_exit_ctls_high & VM_EXIT_SAVE_VMX_= PREEMPTION_TIMER)) { >>> + nested_vmx_failValid(vcpu, VMXERR_ENTRY_INVALID_CONTROL= _FIELD); >>> + return 1; >>> + } >> >> Nope, the guest is free to run the preemption timer without saving on >> exits. It may have a valid use case for this, e.g. that it will always= >> reprogram it on entry. > Here "!(nested_vmx_exit_ctls_high & > VM_EXIT_SAVE_VMX_PREEMPTION_TIMER)" is used to detect if hardware > support "save preemption timer" feature, which means if L2 supports > pinbased vmx preemption timer, host must support "save preemption > timer" feature. Sorry, parsed the code incorrectly. > Though nested_vmx_exit_ctls_* is used for nested env, > but it can also used to reflect the host's feature. Here is what I > discuss with you yesterday, and we can also get the feature via > "rdmsr" here to avoid the confusion. Yes. The point is that we will not even expose PIN_BASED_VMX_PREEMPTION_TIMER if VM_EXIT_SAVE_VMX_PREEMPTION_TIMER is missing. If the guest then requests the former, it simply sets an invalid pin-based control value which we already catch and report. So this hunk becomes redundant. Jan --foaCPBE3noAXqXPi5RrTL9bmffUF3Igta Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.16 (GNU/Linux) Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/ iEYEARECAAYFAlIZsiMACgkQitSsb3rl5xRuzgCg65ZY8SCl1nhHo8E3hROPCbwA iwMAoKeaMcDy6obIM4nB2inM+KznMaPL =CoYr -----END PGP SIGNATURE----- --foaCPBE3noAXqXPi5RrTL9bmffUF3Igta--