| Message ID | 1425353417-2060-1-git-send-email-fanwenyi0529@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 -- 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
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 -- 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
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 -- 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
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 -- 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
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
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(-)