From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Zhang, Yang Z" Subject: RE: [PATCH v11 2/3] x86, apicv: add virtual x2apic support Date: Tue, 22 Jan 2013 03:37:56 +0000 Message-ID: References: <1358331672-32384-1-git-send-email-yang.z.zhang@intel.com> <1358331672-32384-3-git-send-email-yang.z.zhang@intel.com> <20130121195907.GA3561@amt.cnet> <20130121202114.GE25818@redhat.com> <20130121212113.GC7110@amt.cnet> <20130121213420.GA8427@redhat.com> <20130121221618.GE10985@amt.cnet> <20130122003346.GA26201@amt.cnet> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Cc: "kvm@vger.kernel.org" , "Shan, Haitao" , "Zhang, Xiantao" , "Tian, Kevin" To: Marcelo Tosatti , Gleb Natapov Return-path: Received: from mga02.intel.com ([134.134.136.20]:37664 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752656Ab3AVDh6 convert rfc822-to-8bit (ORCPT ); Mon, 21 Jan 2013 22:37:58 -0500 In-Reply-To: <20130122003346.GA26201@amt.cnet> Content-Language: en-US Sender: kvm-owner@vger.kernel.org List-ID: Marcelo Tosatti wrote on 2013-01-22: > On Mon, Jan 21, 2013 at 08:16:18PM -0200, Marcelo Tosatti wrote: >> On Mon, Jan 21, 2013 at 11:34:20PM +0200, Gleb Natapov wrote: >>> On Mon, Jan 21, 2013 at 07:21:13PM -0200, Marcelo Tosatti wrote: >>>> On Mon, Jan 21, 2013 at 10:21:14PM +0200, Gleb Natapov wrote: >>>>>>> } >>>>>>> + >>>>>>> + vcpu->arch.apic_base = value; >>>>>> >>>>>> Simpler to have >>>>>> >>>>>> if (apic_x2apic_mode(apic)) { >>>>>> ... >>>>>> kvm_x86_ops->set_virtual_x2apic_mode(vcpu, true); >>>>>> } else { >>>>>> kvm_x86_ops->set_virtual_x2apic_mode(vcpu, false); >>>>>> } >>>>>> >>>>> This will not work during cpu init. That was discussed on one of >>>>> the previous iterations of the patch. When this code is called during >>>>> vcpu init vmcs is not loaded yet so set_virtual_x2apic_mode() cannot >>>>> write into it. >>>> >>>> Are you saying that the logic to write on bit value change is due to >>>> ordering with cpu init or that the callback is at the wrong place? >>>> >>> The logic is because of ordering with cpu init. >> >> OK. Still must move this conditional callback after assignment of apic_base. You are correct. will move it in next patch. >>>>>> Why not disable write intercept for all MSRs which represent APIC >>>>>> registers that are virtualized? Why TPR is special? >>>>>> >>>>> This patch goes before vid is enabled. At this point only TPR is >>>>> vitalized. If APIC_WRITE exit will be generated on unhandled MSR write >>>>> then we can disable intercept for all x2apic MSRs here. >>>> >>>> -ENOPARSE, please be more verbose. "unhandled MSR write" ? >>> By unhandled I mean unintercepted. Write to x2apic MSR will either >>> generate MSR write exit if msr bitmap says so and then x2apic MSR will >>> be handled just like today, or, if we disable intercept, it will >>> generate APIC_WRITE exit (need to consult SDM here, not sure about it). >>> One is not really preferable to the other. >> >> It will generate APIC_WRITE, for example, if due to EOI virtualization >> exiting. > > Err, no, EOI induced vmexit is due to EOI virtualization. > >> The question is, why is intercept for EOI MSR address (0x80B) not being >> disabled here, while TPR is? I don't see intercept disabled by other >> patches either. TPR shadow is required by virtual x2apic mode. So if enabled virtual x2apic mode, that means TPR shadow is also enabled, then we can disable TPR msr intercept. > Point still valid: why intercept for EOI MSR address not being disabled? Please see the third patch. Only vid is enabled then we will disable eoi msr intercept and self ipi. @@ -6249,10 +6286,138 @@ static void vmx_set_virtual_x2apic_mode(struct kvm_vcpu *vcpu, bool set) vmx_intercept_for_msr_read_x2apic(0x839, true); /* TPR */ vmx_intercept_for_msr_write_x2apic(0x808, false); + /* EOI */ + vmx_intercept_for_msr_write_x2apic(0x80b, false); + /* SELF-IPI */ + vmx_intercept_for_msr_write_x2apic(0x83f, false); } vmx_set_msr_bitmap(vcpu); Best regards, Yang