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

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

[Sheng Yang]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

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


[-- Attachment #2: 0001-KVM-VMX-Fix-repeatly-allocate-of-apic-access-page.patch --]
[-- Type: text/x-diff, Size: 1849 bytes --]

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 allocate of apic access page

For SMP guest, alloc_apic_access_page() would be called more than once.
So only the last one works, 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;
 	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;
 	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;
 
-- 
1.5.2


[-- Attachment #3: Type: text/plain, Size: 314 bytes --]

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

[-- Attachment #4: Type: text/plain, Size: 186 bytes --]

_______________________________________________
kvm-devel mailing list
kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/kvm-devel

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

[Avi Kivity]

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?

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

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

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

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


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

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

[Sheng Yang]

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/

Re: [PATCH] KVM: VMX: Fix repeatlycalling alloc_apic_access_page()

[Dong, Eddie]

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

Current LAPIC code assume vcpu_id = 0 for BSP.
And even no real support of APIC_ID change.

How about use  (vcpu->apic_base & MSR_IA32_APICBASE_BSP) ?

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

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

[Avi Kivity]

Sheng Yang wrote:
>
>>>  	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.
>
>   

That's true for qemu.  But we have to be use malicious userspace can't 
crash the kernel or leak memory using the kvm APIs.

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

That's what qemu does.  But another userspace perhaps will do this 
differently.

This is a minor problem compared to the one above.

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

You're right, during vm creation we have no idea if we want the 
FlexPriority page.

So you can do this in the same place, just take kvm->mutex and check if 
it exists first (you need to do that anyway), no dependency on vcpu 0.


-- 
Any sufficiently difficult bug is indistinguishable from a feature.


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

Re: [PATCH] KVM: VMX: Fix repeatlycalling alloc_apic_access_page()

[Avi Kivity]

Dong, Eddie wrote:
>>> -	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).
>>
>>     
>
> Current LAPIC code assume vcpu_id = 0 for BSP.
> And even no real support of APIC_ID change.
>
> How about use  (vcpu->apic_base & MSR_IA32_APICBASE_BSP) ?
>   

Taking kvm->mutex and checking avoids hidden dependencies and is 
necessary anyway.

I need to sit down and document the kvm locking rules.

-- 
Any sufficiently difficult bug is indistinguishable from a feature.


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