RE: [PATCH v11 2/3] x86, apicv: add virtual x2apic support

From: "Zhang, Yang Z" <yang.z.zhang@intel.com>
To: Marcelo Tosatti <mtosatti@redhat.com>, Gleb Natapov <gleb@redhat.com>
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"Shan, Haitao" <haitao.shan@intel.com>,
	"Zhang, Xiantao" <xiantao.zhang@intel.com>,
	"Tian, Kevin" <kevin.tian@intel.com>
Subject: RE: [PATCH v11 2/3] x86, apicv: add virtual x2apic support
Date: Tue, 22 Jan 2013 03:37:56 +0000	[thread overview]
Message-ID: <A9667DDFB95DB7438FA9D7D576C3D87E313237@SHSMSX101.ccr.corp.intel.com> (raw)
In-Reply-To: <20130122003346.GA26201@amt.cnet>

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


