Re: [PATCH v3 4/8] KVM-HV: Add VCPU running/pre-empted state for guest

[Nikunj A Dadhania <nikunj@xxxxxxxxxxxxxxxxxx>]

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);
 
 		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);

--
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