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

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

[Wincy Van]

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@gmail.com>
---
 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)) {
 		if (is_long_mode(vcpu))
 			msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
 		else
-- 
1.7.1

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

[Bandan Das]

Wincy Van <fanwenyi0529@gmail.com> 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@gmail.com>
> ---
>  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 ?

Bandan

>  		if (is_long_mode(vcpu))
>  			msr_bitmap = vmx_msr_bitmap_longmode_x2apic;
>  		else

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@redhat.com> wrote:
> Wincy Van <fanwenyi0529@gmail.com> 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@gmail.com>
>> ---
>>  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

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

[Bandan Das]

Wincy Van <fanwenyi0529@gmail.com> writes:

> On Wed, Mar 4, 2015 at 1:39 AM, Bandan Das <bsd@redhat.com> wrote:
>> Wincy Van <fanwenyi0529@gmail.com> 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@gmail.com>
>>> ---
>>>  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.
It's not. The change is in vmx_set_msr_bitmap() and vmx_set_efer
happens to call it. The call to the merge function may very well
belong to prepare_vmcs02() but the write to the vmcs field could
belong to vmx_set_msr_bitmap.

> 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.
Fine, just add a comment in prepare_vmcs02 that that's where the field
is being set. No point in doing the same thing twice.


> Thanks,
> Wincy
> --
> To unsubscribe from this list: send the line "unsubscribe kvm" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Wincy Van]

On Wed, Mar 4, 2015 at 12:04 PM, Bandan Das <bsd@redhat.com> wrote:
> Wincy Van <fanwenyi0529@gmail.com> writes:
>
>> On Wed, Mar 4, 2015 at 1:39 AM, Bandan Das <bsd@redhat.com> wrote:
>>> Wincy Van <fanwenyi0529@gmail.com> 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@gmail.com>
>>>> ---
>>>>  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.
> It's not. The change is in vmx_set_msr_bitmap() and vmx_set_efer
> happens to call it. The call to the merge function may very well
> belong to prepare_vmcs02() but the write to the vmcs field could
> belong to vmx_set_msr_bitmap.
>
>> 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.
> Fine, just add a comment in prepare_vmcs02 that that's where the field
> is being set. No point in doing the same thing twice.
>

Yes, Agreed. I'll send v2 ASAP.

Thanks,
Wincy