KVM: VMX: Fix repeatly calling alloc_apic_access_page()

PATCH In review Sheng Yang 3 participants 6 messages Nov 2, 2007 – Nov 5, 2007 4 messages not in the archive KVM

Share & tools

Sheng Yang

author

2007-11-02 05:13 UTC #0 lore raw

PATCH 1/1 KVM: VMX: Fix repeatly calling alloc_apic_access_page()

From 00a52112d813af983dd4d34cb7dc701f6fe88829 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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 <[email protected]>

---
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/vmx.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)

drivers/kvm/kvm_main.c +1 -0

index 34a681d..519626d 100644
@@ -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;
 }

drivers/kvm/vmx.c +4 -1

index 42e7fad..89007b2 100644
@@ -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;

Signature · 1 line

1.5.2

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 <[email protected]>

---
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/vmx.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)

drivers/kvm/kvm_main.c +1 -0

index 34a681d..519626d 100644
@@ -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;
 }

drivers/kvm/vmx.c +4 -1

index 42e7fad..89007b2 100644
@@ -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;

Signature · 1 line

1.5.2

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

_______________________________________________
kvm-devel mailing list [email protected]
https://lists.sourceforge.net/lists/listinfo/kvm-devel

A message that is not in the archive (sent off-list or not yet received): 200711021313.43013.sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Avi Kivity

2007-11-04 07:46 UTC #2 lore raw

Quoted 22 lines · Sheng Yang

From 00a52112d813af983dd4d34cb7dc701f6fe88829 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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 <[email protected]>
---
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/vmx.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)
drivers/kvm/kvm_main.c +0 -0
index 34a681d..519626d 100644

drivers/kvm/kvm_main.c static struct kvm *kvm_create_vm(void) · line 249

Quoted 5 lines

drivers/kvm/kvm_main.c +1 -0

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

Quoted 8 lines

drivers/kvm/kvm_main.c +0 -0
 	return kvm;
 }
drivers/kvm/vmx.c +0 -0
index 42e7fad..89007b2 100644

drivers/kvm/vmx.c static int alloc_apic_access_page(struct kvm *kvm) · line 1471

Quoted 6 lines

drivers/kvm/vmx.c +2 -0

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

Quoted 5 lines

drivers/kvm/vmx.c +0 -0

 	kvm_userspace_mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
 	kvm_userspace_mem.flags = 0;

drivers/kvm/vmx.c +0 -0

 	kvm_userspace_mem.guest_phys_addr = 0xfee00000ULL;

drivers/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vmx *vmx) · line 1593

Quoted 9 lines

drivers/kvm/vmx.c +2 -1

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

Signature · 8 lines

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/

A message that is not in the archive (sent off-list or not yet received): 472D78CA.6080508-atKUWr5tajBWk0Htik3J/[email protected]

Sheng Yang

author

2007-11-05 05:52 UTC #4 lore raw

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

Quoted 18 lines · On Sunday 04 November 2007 15:46:18 Avi Kivity wrote:

Quoted 17 lines · Sheng Yang
From 00a52112d813af983dd4d34cb7dc701f6fe88829 Mon Sep 17 00:00:00 2001
From: Sheng Yang <[email protected]>
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 <[email protected]>
---
 drivers/kvm/kvm_main.c |    1 +
 drivers/kvm/vmx.c      |    5 ++++-
 2 files changed, 5 insertions(+), 1 deletions(-)

Quoted 11 lines

drivers/kvm/kvm_main.c static struct kvm *kvm_create_vm(void) · line 248
Quoted 9 lines
drivers/kvm/kvm_main.c +1 -0
index 34a681d..519626d 100644
@@ -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.

Quoted 11 lines

drivers/kvm/kvm_main.c +0 -0

 	return kvm;
 }

drivers/kvm/vmx.c +0 -0

index 42e7fad..89007b2 100644
@@ -1466,6 +1466,8 @@ static int alloc_apic_access_page(struct kvm *kvm)
 	int r;

Quoted 5 lines

drivers/kvm/vmx.c static int alloc_apic_access_page(struct kvm *kvm) · line 1470
Quoted 3 lines
drivers/kvm/vmx.c +2 -0
 	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.

Quoted 8 lines

drivers/kvm/vmx.c +0 -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);

Quoted 7 lines

drivers/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vmx *vmx) · line 1592
Quoted 5 lines
drivers/kvm/vmx.c +2 -1
-	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.

Quoted 4 lines

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?

Signature · 8 lines

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/

A message that is not in the archive (sent off-list or not yet received): 200711051352.15366.sheng.yang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org

Avi Kivity

2007-11-05 13:55 UTC #6 lore raw

Quoted 18 lines · Sheng Yang

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

Quoted 4 lines

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.

Quoted 16 lines

Quoted 7 lines

drivers/kvm/vmx.c +0 -0

 	kvm_userspace_mem.slot = APIC_ACCESS_PAGE_PRIVATE_MEMSLOT;
 	kvm_userspace_mem.flags = 0;

drivers/kvm/vmx.c +0 -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);

Quoted 8 lines

drivers/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vmx *vmx) · line 1593
Quoted 6 lines
drivers/kvm/vmx.c +2 -1
-	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).

Quoted 3 lines

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.

Quoted 6 lines

Quoted 5 lines
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).

Quoted 6 lines

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.

Signature · 8 lines

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/

Dong, Eddie

2007-11-05 08:52 UTC #7 lore raw

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

In reply to a message not in the archive

Quoted 11 lines

drivers/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vmx *vmx) · line 1592
Quoted 8 lines
drivers/kvm/vmx.c +2 -1
-	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) ?

A message that is not in the archive (sent off-list or not yet received): 10EA09EFD8728347A513008B6B0DA77A02557D41-wq7ZOvIWXbNpB2pF5aRoyrfspsVTdybXVpNB7YpNyf8@public.gmane.org

Avi Kivity

2007-11-05 13:58 UTC #9 lore raw

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

Quoted 12 lines · Eddie

Quoted 11 lines
drivers/kvm/vmx.c static int vmx_vcpu_setup(struct vcpu_vmx *vmx) · line 1592
Quoted 8 lines
drivers/kvm/vmx.c +2 -1
-	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).

Quoted 5 lines

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.

Signature · 8 lines

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/