Re: [PATCH] KVM: vmx: Set msr bitmap correctly if vcpu is in guest mode

[Wincy Van]

On Wed, Mar 4, 2015 at 1:39 AM, Bandan Das <bsd@xxxxxxxxxx> wrote:
> Wincy Van <fanwenyi0529@xxxxxxxxx> writes:
>
>> In commit 3af18d9c5fe9 ("KVM: nVMX: Prepare for using hardware MSR bitmap"),
>> we are setting MSR_BITMAP in prepare_vmcs02 if we should use hardware. This
>> is not enough since the field will be modified by following vmx_set_efer.
>>
>> Fix this by setting vmx_msr_bitmap_nested in vmx_set_msr_bitmap if vcpu is
>> in guest mode.
>>
>> Signed-off-by: Wincy Van <fanwenyi0529@xxxxxxxxx>
>> ---
>>  arch/x86/kvm/vmx.c |    5 ++++-
>>  1 files changed, 4 insertions(+), 1 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index f7b20b4..f6e3457 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -2168,7 +2168,10 @@ static void vmx_set_msr_bitmap(struct kvm_vcpu *vcpu)
>>  {
>>       unsigned long *msr_bitmap;
>>
>> -     if (irqchip_in_kernel(vcpu->kvm) && apic_x2apic_mode(vcpu->arch.apic)) {
>> +     if (is_guest_mode(vcpu))
>> +             msr_bitmap = vmx_msr_bitmap_nested;
>> +     else if (irqchip_in_kernel(vcpu->kvm) &&
>> +             apic_x2apic_mode(vcpu->arch.apic)) {
>
> So, we end up writing the MSR_BITMAP field twice - once when we
> call nested_vmx_merge_msr_bitmap() and another here. Why don't we just
> remove the former since prepare_vmcs02 will call vmx_set_efer anyway ?
>

Yes, setting MSR_BITMAP twice is redundant, but we can not rely on
vmx_set_efer to set that field, this is not vmx_set_efer 's duty.
Consider that someone wants to make some changes on loading
L2's efer, he may be confused about this. We should reduce the
degree of code coupling.

Thanks,
Wincy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/