From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wanpeng Li Subject: Re: [PATCH v3] KVM: nVMX: nested TPR shadow/threshold emulation Date: Thu, 7 Aug 2014 14:26:28 +0800 Message-ID: <20140807062628.GA32500@kernel> References: <1407149907-13483-1-git-send-email-wanpeng.li@linux.intel.com> <53E0B931.7010309@redhat.com> Reply-To: Wanpeng Li Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Marcelo Tosatti , Gleb Natapov , Bandan Das , "kvm@vger.kernel.org" , "linux-kernel@vger.kernel.org" To: Paolo Bonzini , Jan Kiszka , "Zhang, Yang Z" Return-path: Content-Disposition: inline In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org List-Id: kvm.vger.kernel.org Hi Paolo, On Wed, Aug 06, 2014 at 06:38:06AM +0000, Zhang, Yang Z wrote: [...] >>>> + >>>> + if (exec_control & CPU_BASED_TPR_SHADOW) { >>>> + if (vmx->nested.virtual_apic_page) >>>> + nested_release_page(vmx->nested.virtual_apic_page); >>>> + vmx->nested.virtual_apic_page = >>>> + nested_get_page(vcpu, vmcs12->virtual_apic_page_addr); >>>> + if (!vmx->nested.virtual_apic_page) >>>> + exec_control &= >>>> + ~CPU_BASED_TPR_SHADOW; >>>> + else >>>> + vmcs_write64(VIRTUAL_APIC_PAGE_ADDR, >>>> + page_to_phys(vmx->nested.virtual_apic_page)); >>>> + >>>> + if (!(exec_control & CPU_BASED_TPR_SHADOW) && >>>> + !((exec_control & CPU_BASED_CR8_LOAD_EXITING) && >>>> + (exec_control & CPU_BASED_CR8_STORE_EXITING))) >>>> + nested_vmx_failValid(vcpu, >>>> VMXERR_ENTRY_INVALID_CONTROL_FIELD); >>> >>> I think this is not correct. The vmx->nested.virtual_apic_page may not >>> valid due to two reasons: 1. The virtual_apic_page_addr is not a valid >>> gfn. In this case, the vmx failure >> must be injected to L1 unconditionally regardless of the setting of >> CR8 load/store exiting. >> >> You're right that accesses to the APIC-access page may also end up >> writing to the virtual-APIC page. Hence, if the "virtualize APIC >> accesses" setting is 1 in the secondary exec controls, you also have to fail the vmentry. >> >> Doing it unconditionally is not correct, but it is the simplest thing > >Why not correct? What's your opinion? > >> to do and it would be okay with a comment, I think. >> >>> 2. The virtual_apic_page is swapped by L0. In this case, we should >>> not inject >> failure to L1. >> >> This cannot happen, nested_get_page ends up calling hva_to_pfn with >> atomic==false, so the page would be swapped in and pinned. >> >>>> + >>>> + vmcs_write32(TPR_THRESHOLD, vmcs12->tpr_threshold); >>>> + } >>> >>> Miss else here: >>> If L2 owns the APIC and doesn't use TPR_SHADOW, we need to setup the >>> vmcs02 based on vmcs01. For example, if vmcs01 is using TPR_SHADOW, >>> then >>> vmcs02 must set it. Also, we need to use vmcs01's virtual_apic_page >>> and TPR_THRESHOLD for vmcs02. >> >> What do you mean by "owns the APIC"? > >Means if L2 touch L1's APIC directly, for example, if L2 not using TPR_SHADOW, then move to/from CR8 will touch L1's TPR directly. And in this case, L0 should aware of L1's TRP change. > Ditto. Regards, Wanpeng Li >> >> Paolo > > >Best regards, >Yang >