On Thu, 2 Aug 2012 16:56:28 -0300, Marcelo Tosatti <mtosatti@xxxxxxxxxx> wrote:
> On Tue, Jul 31, 2012 at 04:18:41PM +0530, Nikunj A. Dadhania wrote:
> > From: Nikunj A. Dadhania <nikunj@xxxxxxxxxxxxxxxxxx>
> >
> > Hypervisor code to indicate guest running/pre-empteded status through
> > msr. The page is now pinned during MSR write time and use
> > kmap_atomic/kunmap_atomic to access the shared area vcpu_state area.
> >
> > Suggested-by: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
> > Signed-off-by: Nikunj A. Dadhania <nikunj@xxxxxxxxxxxxxxxxxx>
> > ---
> > arch/x86/include/asm/kvm_host.h | 7 ++++
> > arch/x86/kvm/cpuid.c | 1 +
> > arch/x86/kvm/x86.c | 71 ++++++++++++++++++++++++++++++++++++++-
> > 3 files changed, 77 insertions(+), 2 deletions(-)
[...]
> > +static void kvm_set_atomic(u64 *addr, u64 old, u64 new)
> > +{
> > + int loop = 1000000;
> > + while (1) {
> > + if (cmpxchg(addr, old, new) == old)
> > + break;
> > + loop--;
> > + if (!loop) {
> > + pr_info("atomic cur: %lx old: %lx new: %lx\n",
> > + *addr, old, new);
> > + break;
> > + }
> > + }
> > +}
>
> A generic "kvm_set_atomic" would need that loop, but in the particular
> TLB flush case we know that the only information being transmitted is
> a TLB flush.
>
yes, so the next patch gets rid of this in a neater way.
> So this idea should work:
>
> old = *addr;
> if (cmpxchg(addr, old, IN_GUEST_MODE) == FAILURE)
> kvm_x86_ops->tlb_flush()
> atomic_set(addr, IN_GUEST_MODE);
> } else if {
> if (old & TLB_SHOULD_FLUSH)
> kvm_x86_ops->tlb_flush()
> }
>
> (the actual pseucode above is pretty ugly and
> mus be improved but it should be enough to transmit
> the idea).
>
> Of course as long as you make sure the atomic_set does not
> overwrite information.
>
>
> > + char *kaddr;
> > +
> > + if (!(vcpu->arch.v_state.msr_val & KVM_MSR_ENABLED) ||
> > + !vcpu->arch.v_state.vs_page)
> > + return;
>
> If its not enabled vs_page should be NULL?
>
Yes, it should be:
if (!(enabled && vs_page))
return;
> > +
> > + kaddr = kmap_atomic(vcpu->arch.v_state.vs_page);
> > + kaddr += vcpu->arch.v_state.vs_offset;
> > + vs = kaddr;
> > + kvm_set_atomic(&vs->state, 0, 1 << KVM_VCPU_STATE_IN_GUEST_MODE);
> > + kunmap_atomic(kaddr);
> > +}
> > +
> > +static void kvm_clear_vcpu_state(struct kvm_vcpu *vcpu)
> > +{
> > + struct kvm_vcpu_state *vs;
> > + char *kaddr;
> > +
> > + if (!(vcpu->arch.v_state.msr_val & KVM_MSR_ENABLED) ||
> > + !vcpu->arch.v_state.vs_page)
> > + return;
>
> Like above.
>
> > + kaddr = kmap_atomic(vcpu->arch.v_state.vs_page);
> > + kaddr += vcpu->arch.v_state.vs_offset;
> > + vs = kaddr;
> > + kvm_set_atomic(&vs->state, 1 << KVM_VCPU_STATE_IN_GUEST_MODE, 0);
> > + kunmap_atomic(kaddr);
> > +}
> > +
> > int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > {
> > bool pr = false;
> > @@ -1676,6 +1723,18 @@ int kvm_set_msr_common(struct kvm_vcpu *vcpu, u32 msr, u64 data)
> > return 1;
> > break;
> >
> > + 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.
>
Will do that.
> > +
> > + if (is_error_page(vcpu->arch.v_state.vs_page)) {
> > + kvm_release_page_clean(vcpu->arch.time_page);
C&P error :(
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");
>
> Missing break or return;
>
Right
> > + }
> > + 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).
>
Will do.
Thanks for the detailed review.
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
