On Fri, 3 Aug 2012 14:31:22 -0300, Marcelo Tosatti <mtosatti@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
