Gleb Natapov wrote on 2012-12-24:
> On Mon, Dec 24, 2012 at 02:35:35AM +0000, Zhang, Yang Z wrote:
>> Zhang, Yang Z wrote on 2012-12-24:
>>> Gleb Natapov wrote on 2012-12-20:
>>>> On Mon, Dec 17, 2012 at 01:30:50PM +0800, Yang Zhang wrote:
>>>>> basically to benefit from apicv, we need clear MSR bitmap for
>>>>> corresponding x2apic MSRs:
>>>>> 0x800 - 0x8ff: no read intercept for apicv register virtualization
>>>>> TPR,EOI,SELF-IPI: no write intercept for virtual interrupt delivery
>>>> We do not set "Virtualize x2APIC mode" bit in secondary execution
>>>> control. If I read the spec correctly without that those MSR read/writes
>>>> will go straight to physical local APIC.
>>> Right. Now it cannot get benefit, but we may enable it in future and
>>> then we can benefit from it.
> Without enabling it you cannot disable MSR intercept for x2apic MSRs.
>
>> how about to add the following check:
>> if (apicv_enabled && virtual_x2apic_enabled)
>> clear_msr();
>>
> I do not understand what do you mean here.
In this patch, it will clear MSR bitmap(0x800 -0x8ff) when apicv enabled. As you said, since kvm doesn't set "virtualize x2apic mode", APIC register virtualization never take effect. So we need to clear MSR bitmap only when apicv enabled and virtualize x2apic mode set.
>>
>>>>> Signed-off-by: Yang Zhang <yang.z.zhang@xxxxxxxxx>
>>>>> Signed-off-by: Kevin Tian <kevin.tian@xxxxxxxxx>
>>>>> ---
>>>>> arch/x86/kvm/vmx.c | 62
>>>>> ++++++++++++++++++++++++++++++++++++++++++++++------ 1 files
>>>>> changed, 55 insertions(+), 7 deletions(-)
>>>>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>>>>> index be66c3e..9b5e7a2 100644
>>>>> --- a/arch/x86/kvm/vmx.c
>>>>> +++ b/arch/x86/kvm/vmx.c
>>>>> @@ -3773,7 +3773,10 @@ static void free_vpid(struct vcpu_vmx *vmx)
>>>>> spin_unlock(&vmx_vpid_lock);
>>>>> }
>>>>> -static void __vmx_disable_intercept_for_msr(unsigned long
>>>>> *msr_bitmap, u32 msr) +#define MSR_TYPE_R 1 +#define MSR_TYPE_W 2
>>>>> +static void __vmx_disable_intercept_for_msr(unsigned long
>>>>> *msr_bitmap, + u32 msr, int type)
>>>>> {
>>>>> int f = sizeof(unsigned long);
>>>>> @@ -3786,20 +3789,52 @@ static void
>>>> __vmx_disable_intercept_for_msr(unsigned long *msr_bitmap, u32 msr)
>>>>> * We can control MSRs 0x00000000-0x00001fff and
>>>>> 0xc0000000-0xc0001fff. */ if (msr <= 0x1fff) {
>>>>> - __clear_bit(msr, msr_bitmap + 0x000 / f); /* read-low */
>>>>> - __clear_bit(msr, msr_bitmap + 0x800 / f); /* write-low */
>>>>> + if (type & MSR_TYPE_R)
>>>>> + /* read-low */
>>>>> + __clear_bit(msr, msr_bitmap + 0x000 / f);
>>>>> +
>>>>> + if (type & MSR_TYPE_W)
>>>>> + /* write-low */
>>>>> + __clear_bit(msr, msr_bitmap + 0x800 / f);
>>>>> +
>>>>> } else if ((msr >= 0xc0000000) && (msr <= 0xc0001fff)) {
>>>>> msr &= 0x1fff;
>>>>> - __clear_bit(msr, msr_bitmap + 0x400 / f); /* read-high */
>>>>> - __clear_bit(msr, msr_bitmap + 0xc00 / f); /* write-high */
>>>>> + if (type & MSR_TYPE_R)
>>>>> + /* read-high */
>>>>> + __clear_bit(msr, msr_bitmap + 0x400 / f);
>>>>> +
>>>>> + if (type & MSR_TYPE_W)
>>>>> + /* write-high */
>>>>> + __clear_bit(msr, msr_bitmap + 0xc00 / f);
>>>>> +
>>>>> }
>>>>> }
>>>>>
>>>>> static void vmx_disable_intercept_for_msr(u32 msr, bool
>>>>> longmode_only) { if (!longmode_only)
>>>>> - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, msr);
>>>>> - __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, msr);
>>>>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, +
>>>>> msr, MSR_TYPE_R | MSR_TYPE_W);
>>>>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, +
>>>>> msr, MSR_TYPE_R | MSR_TYPE_W); +} + +static void
>>>>> vmx_disable_intercept_for_msr_read(u32 msr, bool longmode_only) +{ +
>>>>> if (!longmode_only)
>>>>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, +
>>>>> msr, MSR_TYPE_R); +
>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, +
>>>>> msr, MSR_TYPE_R); +} + +static void
>>>>> vmx_disable_intercept_for_msr_write(u32 msr, bool longmode_only) +{
>>>>> + if (!longmode_only)
>>>>> + __vmx_disable_intercept_for_msr(vmx_msr_bitmap_legacy, +
>>>>> msr, MSR_TYPE_W); +
>>>>> __vmx_disable_intercept_for_msr(vmx_msr_bitmap_longmode, +
>>>>> msr, MSR_TYPE_W);
>>>>> }
>>>>>
>>>>> /* @@ -7633,6 +7668,19 @@ static int __init vmx_init(void)
>>>>> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_ESP, false);
>>>>> vmx_disable_intercept_for_msr(MSR_IA32_SYSENTER_EIP, false);
>>>>> + if (enable_apicv_reg_vid) {
>>>>> + int msr;
>>>>> + for (msr = 0x800; msr <= 0x8ff; msr++)
>>>>> + vmx_disable_intercept_for_msr_read(msr, false);
>>>>> +
>>>>> + /* TPR */
>>>>> + vmx_disable_intercept_for_msr_write(0x808, false);
>>>>> + /* EOI */
>>>>> + vmx_disable_intercept_for_msr_write(0x80b, false);
>>>>> + /* SELF-IPI */
>>>>> + vmx_disable_intercept_for_msr_write(0x83f, false);
>>>>> + }
>>>>> +
>>>>> if (enable_ept) {
>>>>> kvm_mmu_set_mask_ptes(0ull,
>>>>> (enable_ept_ad_bits) ? VMX_EPT_ACCESS_BIT : 0ull,
>>>>> --
>>>>> 1.7.1
>>>>
>>>> --
>>>> Gleb.
>>>
>>>
>>> Best regards,
>>> Yang
>>>
>>
>>
>> Best regards,
>> Yang
>>
>
> --
> Gleb.
Best regards,
Yang
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
