RE: [PATCH v3 1/6] KVM: nVMX: Use hardware MSR bitmap

[Zhang, Yang Z]

Wincy Van wrote on 2015-01-28:
> On Wed, Jan 28, 2015 at 8:33 PM, Zhang, Yang Z <yang.z.zhang@xxxxxxxxx>
> wrote:
>>>> 
>>>> You are right, but this is not fit for all the cases, we should
>>>> custom the nested_msr_bitmap.
>>>> e.g.  Currently L0 wants to intercept some of the x2apic msrs' reading:
>>>>          if (enable_apicv) {
>>>>                 for (msr = 0x800; msr <= 0x8ff; msr++)
>>>>                 vmx_disable_intercept_msr_read_x2apic(msr); /*
>>>>                 According SDM, in x2apic mode, the whole id reg
>>>> is
>>> used.
>>>>                  * But in KVM, it only use the highest eight bits.
>>>> Need
> to
>>>>                  * intercept it */
>>>>                 vmx_enable_intercept_msr_read_x2apic(0x802); /* TMCCT
>>>>                 */ vmx_enable_intercept_msr_read_x2apic(0x839); /*
>>>>                 TPR */ vmx_disable_intercept_msr_write_x2apic(0x808);
> /*
>>>> EOI
>>> */
>>>>                 vmx_disable_intercept_msr_write_x2apic(0x80b); /*
>>>>                 SELF-IPI */
>>>>                 vmx_disable_intercept_msr_write_x2apic(0x83f);
>>>>         }
>>>> But L1 may not want this. So I think we are better to deal with
>>>> the
>>> 
>>> Actually, from L0's point, it is totally unaware of the L2. The only
>>> thing L0 aware is that the CPU should follow L0's configuration when
>>> VCPU is running. So if L0 wants to trap a msr, then the read operation
>>> to this msr should cause vmexit unconditionally no matter who is
>>> running(who means L1, L2, L3.....).
>>> 
>>>> msrs seperately, there is not a common way suit for all the cases.
>>>> If other features want to intercept a msr in nested entry, they
>>>> can put the custom code in nested_vmx_merge_msr_bitmap.
>>> 
>>> Yes, if other features want to do it in 'nested' entry, they can
>>> fill nested_vmx_merge_msr_bitmap. But if in non-nested case, it
>>> should be our responsibly to handle it correctly, how about add following check:
>>> 
>>> if (type & MSR_TYPE_R && !test_bit(msr, vmcs01_msr_bitmap) &&
>>> !test_bit(msr, msr_bitmap_l1 + 0x000 / f))
>>>         __clear_bit(msr, msr_bitmap_nested + 0x000 / f);
>> 
>> 
>> Anyway, this is not necessary for your current patch. We can consider
>> it later if there really have other features will use it.
>> 
> 
> Yep, I know what you mean now, for other msrs which are not forwarded
> access by a mechanism like virtual-apic page, we should intercept it
> unconditionally. I think we should ensure the msr can be allowed
> before call nested_vmx_disable_intercept_for_msr, if L0 want to
> intercept it, just do not call nested_vmx_disable_intercept_for_msr.

Yes, this is a solution. But I prefer to handle it in nested code path since others may not familiar with nested and may block it by mistake.

> 
>  !test_bit(msr, vmcs01_msr_bitmap) will introduce a problem that some
> of the msrs will be affcted by vmcs01_msr_bitmap, TMCCT and TPR, for example.
> Intercept reading for these msrs is okay, but it is not efficient.

TMCCT is always trapped by most VMM. I don't think TPR is trapped in KVM.

> 
> Thanks,
> Wincy


Best regards,
Yang