Re: [PATCH] KVM: VMX: Fix repeatly calling alloc_apic_access_page ()

[Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Sunday 04 November 2007 15:46:18 Avi Kivity wrote:
> Sheng Yang wrote:
> > From 00a52112d813af983dd4d34cb7dc701f6fe88829 Mon Sep 17 00:00:00 2001
> > From: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > Date: Fri, 2 Nov 2007 11:56:17 +0800
> > Subject: [PATCH] KVM: VMX: Fix repeatly calling alloc_apic_access_page()
> >
> > For SMP guest, alloc_apic_access_page() would be called more than once.
> > So only the last vcpu's vmcs get correct apic access address, causing SMP
> > guest can't benifit from FlexPriority.
> >
> > This patch fixed this issue.
> >
> > Signed-off-by: Sheng Yang <sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> >  drivers/kvm/kvm_main.c |    1 +
> >  drivers/kvm/vmx.c      |    5 ++++-
> >  2 files changed, 5 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> > index 34a681d..519626d 100644
> > --- a/drivers/kvm/kvm_main.c
> > +++ b/drivers/kvm/kvm_main.c
> > @@ -245,6 +245,7 @@ static struct kvm *kvm_create_vm(void)
> >  	spin_lock(&kvm_lock);
> >  	list_add(&kvm->vm_list, &vm_list);
> >  	spin_unlock(&kvm_lock);
> > +	kvm->apic_access_page = NULL;
>
> Seems unnecessary, since the whole thing is kzalloc()ed?

Yeah, that's right. I missed it. 

>
> >  	return kvm;
> >  }
> >
> > diff --git a/drivers/kvm/vmx.c b/drivers/kvm/vmx.c
> > index 42e7fad..89007b2 100644
> > --- a/drivers/kvm/vmx.c
> > +++ b/drivers/kvm/vmx.c
> > @@ -1466,6 +1466,8 @@ static int alloc_apic_access_page(struct kvm *kvm)
> >  	int r;
> >
> >  	r = -EFAULT;
> > +	if (kvm->apic_access_page)
> > +		return 0;
>
> Racy, what if two vcpus are created simultaneously?

I think it is not racy, for BSP have been created before APs in sequence, and 
I am ensure only BSP(vcpu id=0) would call this.

>
> >  	kvm_userspace_mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
> >  	kvm_userspace_mem.flags = 0;
> >  	kvm_userspace_mem.guest_phys_addr = 0xfee00000ULL;
> > @@ -1584,7 +1586,8 @@ static int vmx_vcpu_setup(struct vcpu_vmx *vmx)
> >  	vmcs_writel(CR0_GUEST_HOST_MASK, ~0UL);
> >  	vmcs_writel(CR4_GUEST_HOST_MASK, KVM_GUEST_CR4_MASK);
> >
> > -	if (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm))
> > +	if ((vmx->vcpu.vcpu_id == 0) &&
> > +	    (vm_need_virtualize_apic_accesses(vmx->vcpu.kvm)))
> >  		if (alloc_apic_access_page(vmx->vcpu.kvm) != 0)
> >  			return -ENOMEM;
>
> We may not have vcpu id 0 (though it's very unlikely).

Um... I am not quite understand when we will miss vcpu id 0. I think vcpu id 0 
is used to indicate BSP.

>
> I think the problems arise because we are doing a VM-wide operation
> (memory slot) from a vcpu context.  I think adding a ->vm_create() arch
> op and allocating the memory there will be better (under kvm->lock).

Agree, but a little problem remains. 

I have to do feature detection before call allocate function, but 
kvm_create_vm() is called before kvm_create_irqchip(). So I can't find a 
proper position for create_vm(). Any suggestion?

-- 
Thanks
Yang, Sheng

-------------------------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc.
Still grepping through log files to find problems?  Stop.
Now Search log events and configuration files using AJAX and a browser.
Download your FREE copy of Splunk now >> http://get.splunk.com/