| Message ID | 87a9yc7c4f.fsf@abhimanyu.in.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 03, 2012 at 11:25:44AM +0530, Nikunj A Dadhania wrote: > On Thu, 2 Aug 2012 16:56:28 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote: > > > > > > + case MSR_KVM_VCPU_STATE: > > > + vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT); > > > + vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); > > > > Assign vs_offset after success. > > > > > + > > > + if (is_error_page(vcpu->arch.v_state.vs_page)) { > > > + kvm_release_page_clean(vcpu->arch.time_page); > > > + vcpu->arch.v_state.vs_page = NULL; > > > + pr_info("KVM: VCPU_STATE - Unable to pin the page\n"); > > > > Missing break or return; > > > > > + } > > > + vcpu->arch.v_state.msr_val = data; > > > + break; > > > + > > > case MSR_IA32_MCG_CTL: > > > > Please verify this code carefully again. > > > > Also leaking the page reference. > > > > > vcpu->arch.apf.msr_val = 0; > > > vcpu->arch.st.msr_val = 0; > > > + vcpu->arch.v_state.msr_val = 0; > > > > Add a newline and comment (or even better a new helper). > > > > > > kvmclock_reset(vcpu); > > > > How about something like the below. I have tried to look at time_page > for reference: > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > index 580abcf..c82cc12 100644 > --- a/arch/x86/kvm/x86.c > +++ b/arch/x86/kvm/x86.c > @@ -1604,6 +1604,16 @@ static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu) > kunmap_atomic(kaddr); > } > > +static void kvm_vcpu_state_reset(struct kvm_vcpu *vcpu) > +{ > + vcpu->arch.v_state.msr_val = 0; > + vcpu->arch.v_state.vs_offset = 0; > + if (vcpu->arch.v_state.vs_page) { > + kvm_release_page_dirty(vcpu->arch.v_state.vs_page); > + vcpu->arch.v_state.vs_page = NULL; > + } > +} > + > int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > { > bool pr = false; > @@ -1724,14 +1734,17 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > break; > > case MSR_KVM_VCPU_STATE: > + kvm_vcpu_state_reset(vcpu); > + > vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT); > - vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); Should also fail if its not enabled (KVM_MSR_ENABLED bit). What is the point of having non-NULL vs_page pointer if KVM_MSR_ENABLED bit is not set? The rest is fine, thanks. -- 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 Fri, 3 Aug 2012 14:31:22 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote: > On Fri, Aug 03, 2012 at 11:25:44AM +0530, Nikunj A Dadhania wrote: > > On Thu, 2 Aug 2012 16:56:28 -0300, Marcelo Tosatti <mtosatti@redhat.com> wrote: > > > > > > > > + case MSR_KVM_VCPU_STATE: > > > > + vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT); > > > > + vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); > > > > > > Assign vs_offset after success. > > > > > > > + > > > > + if (is_error_page(vcpu->arch.v_state.vs_page)) { > > > > + kvm_release_page_clean(vcpu->arch.time_page); > > > > + vcpu->arch.v_state.vs_page = NULL; > > > > + pr_info("KVM: VCPU_STATE - Unable to pin the page\n"); > > > > > > Missing break or return; > > > > > > > + } > > > > + vcpu->arch.v_state.msr_val = data; > > > > + break; > > > > + > > > > case MSR_IA32_MCG_CTL: > > > > > > Please verify this code carefully again. > > > > > > Also leaking the page reference. > > > > > > > vcpu->arch.apf.msr_val = 0; > > > > vcpu->arch.st.msr_val = 0; > > > > + vcpu->arch.v_state.msr_val = 0; > > > > > > Add a newline and comment (or even better a new helper). > > > > > > > > kvmclock_reset(vcpu); > > > > > > > How about something like the below. I have tried to look at time_page > > for reference: > > > > > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > index 580abcf..c82cc12 100644 > > --- a/arch/x86/kvm/x86.c > > +++ b/arch/x86/kvm/x86.c > > @@ -1604,6 +1604,16 @@ static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu) > > kunmap_atomic(kaddr); > > } > > > > +static void kvm_vcpu_state_reset(struct kvm_vcpu *vcpu) > > +{ > > + vcpu->arch.v_state.msr_val = 0; > > + vcpu->arch.v_state.vs_offset = 0; > > + if (vcpu->arch.v_state.vs_page) { > > + kvm_release_page_dirty(vcpu->arch.v_state.vs_page); > > + vcpu->arch.v_state.vs_page = NULL; > > + } > > +} > > + > > int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > { > > bool pr = false; > > @@ -1724,14 +1734,17 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) > > break; > > > > case MSR_KVM_VCPU_STATE: > > + kvm_vcpu_state_reset(vcpu); > > + > > vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT); > > - vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); > > Should also fail if its not enabled (KVM_MSR_ENABLED bit). > > What is the point of having non-NULL vs_page pointer if KVM_MSR_ENABLED > bit is not set? > Yes, will do that. > The rest is fine, thanks. > Thanks Nikunj -- 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/x86.c b/arch/x86/kvm/x86.c index 580abcf..c82cc12 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -1604,6 +1604,16 @@ static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu) kunmap_atomic(kaddr); } +static void kvm_vcpu_state_reset(struct kvm_vcpu *vcpu) +{ + vcpu->arch.v_state.msr_val = 0; + vcpu->arch.v_state.vs_offset = 0; + if (vcpu->arch.v_state.vs_page) { + kvm_release_page_dirty(vcpu->arch.v_state.vs_page); + vcpu->arch.v_state.vs_page = NULL; + } +} + int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) { bool pr = false; @@ -1724,14 +1734,17 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data) break; case MSR_KVM_VCPU_STATE: + kvm_vcpu_state_reset(vcpu); + vcpu->arch.v_state.vs_page = gfn_to_page(vcpu->kvm, data >> PAGE_SHIFT); - vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); if (is_error_page(vcpu->arch.v_state.vs_page)) { - kvm_release_page_clean(vcpu->arch.time_page); + kvm_release_page_clean(vcpu->arch.v_state.vs_page); vcpu->arch.v_state.vs_page = NULL; pr_info("KVM: VCPU_STATE - Unable to pin the page\n"); + break; } + vcpu->arch.v_state.vs_offset = data & ~(PAGE_MASK | KVM_MSR_ENABLED); vcpu->arch.v_state.msr_val = data; break; @@ -6053,6 +6066,7 @@ void kvm_put_guest_fpu(struct kvm_vcpu *vcpu) void kvm_arch_vcpu_free(struct kvm_vcpu *vcpu) { kvmclock_reset(vcpu); + kvm_vcpu_state_reset(vcpu); free_cpumask_var(vcpu->arch.wbinvd_dirty_mask); fx_free(vcpu); @@ -6109,9 +6123,9 @@ int kvm_arch_vcpu_reset(struct kvm_vcpu *vcpu) kvm_make_request(KVM_REQ_EVENT, vcpu); vcpu->arch.apf.msr_val = 0; vcpu->arch.st.msr_val = 0; - vcpu->arch.v_state.msr_val = 0; kvmclock_reset(vcpu); + kvm_vcpu_state_reset(vcpu); kvm_clear_async_pf_completion_queue(vcpu); kvm_async_pf_hash_reset(vcpu);