Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 2011-04-26 06:50, Takuya Yoshikawa wrote: On Mon, 25 Apr 2011 11:15:20 +0200 Jan Kiszka jan.kis...@web.de wrote: Sorry, I did not test on x86_32. Introducing a wrapper function with ifdef would be the best way? Maybe you could also add the missing 64-bit get_user for x86-32. Given that we have a corresponding put_user, I wonder why the get_user was left out. Jan Google said that there was a similar talk on LKML in 2004. On that threads, Linus explained how to tackle on the 64-bit get_user implementation. But I could not see what happened after that. Mmh, maybe the kernel was lacking a real use case, so no one seriously cared. I don't see a fundamental blocker for an x86-32 __get_user_8 version based on two mov. I would give it a try. Jan signature.asc Description: OpenPGP digital signature
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/25/2011 11:04 AM, Jan Kiszka wrote:
+
+ ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
+ if (get_user(pte, ptep_user)) {
This doesn't work for x86-32: pte is 64 bit, but get_user is only
defined up to 32 bit on that platform.
I actually considered this, and saw:
#ifdef CONFIG_X86_32
#define __get_user_8(__ret_gu, __val_gu, ptr)\
__get_user_x(X, __ret_gu, __val_gu, ptr)
#else
#define __get_user_8(__ret_gu, __val_gu, ptr)\
__get_user_x(8, __ret_gu, __val_gu, ptr)
#endif
#define get_user(x, ptr)\
({\
int __ret_gu;\
unsigned long __val_gu;\
__chk_user_ptr(ptr);\
might_fault();\
switch (sizeof(*(ptr))) {\
...
case 8:\
__get_user_8(__ret_gu, __val_gu, ptr);\
break;\
...
}\
(x) = (__typeof__(*(ptr)))__val_gu;\
__ret_gu;\
})
so it should work. How does it fail?
Avi, what's your 32-bit buildbot doing? :)
I regularly autotest on x86_64, not on i386, sorry.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/26/2011 10:45 AM, Jan Kiszka wrote:
On 2011-04-26 09:42, Avi Kivity wrote:
On 04/25/2011 11:04 AM, Jan Kiszka wrote:
+
+ptep_user = (pt_element_t __user *)((void *)host_addr +
offset);
+if (get_user(pte, ptep_user)) {
This doesn't work for x86-32: pte is 64 bit, but get_user is only
defined up to 32 bit on that platform.
I actually considered this, and saw:
#ifdef CONFIG_X86_32
#define __get_user_8(__ret_gu, __val_gu, ptr)\
__get_user_x(X, __ret_gu, __val_gu, ptr)
#else
#define __get_user_8(__ret_gu, __val_gu, ptr)\
__get_user_x(8, __ret_gu, __val_gu, ptr)
#endif
#define get_user(x, ptr)\
({\
int __ret_gu;\
unsigned long __val_gu;\
__chk_user_ptr(ptr);\
might_fault();\
switch (sizeof(*(ptr))) {\
...
case 8:\
__get_user_8(__ret_gu, __val_gu, ptr);\
break;\
...
}\
(x) = (__typeof__(*(ptr)))__val_gu;\
__ret_gu;\
})
so it should work. How does it fail?
On x86-32, the above macro resolves to __get_user_X, an undefined symbol.
Tricky stuff.
--
error compiling committee.c: too many arguments to function
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On Tue, 26 Apr 2011 08:34:57 +0200 Jan Kiszka jan.kis...@web.de wrote: Google said that there was a similar talk on LKML in 2004. On that threads, Linus explained how to tackle on the 64-bit get_user implementation. But I could not see what happened after that. Mmh, maybe the kernel was lacking a real use case, so no one seriously cared. I don't see a fundamental blocker for an x86-32 __get_user_8 version based on two mov. I would give it a try. Jan Thank you! Avi, do we revert the patch now, or ...? Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/26/2011 05:40 PM, Takuya Yoshikawa wrote: On Tue, 26 Apr 2011 08:34:57 +0200 Jan Kiszkajan.kis...@web.de wrote: Google said that there was a similar talk on LKML in 2004. On that threads, Linus explained how to tackle on the 64-bit get_user implementation. But I could not see what happened after that. Mmh, maybe the kernel was lacking a real use case, so no one seriously cared. I don't see a fundamental blocker for an x86-32 __get_user_8 version based on two mov. I would give it a try. Jan Thank you! Avi, do we revert the patch now, or ...? Please post a simple patch that uses two get_user()s for that case (64-bit pte on 32-bit host). Then work with the x86 tree to see if they'll accept 64-bit get_user(), and once they do, we can go back to a simple get_user() again. btw, I think we can use __get_user() here since the address must have been already validated. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
Please post a simple patch that uses two get_user()s for that case (64-bit pte on 32-bit host). Then work with the x86 tree to see if they'll accept 64-bit get_user(), and once they do, we can go back to a simple get_user() again. btw, I think we can use __get_user() here since the address must have been already validated. Yes, I thought that at first. But don't we need to care KVM's address translation bugs? Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On Tue, 26 Apr 2011 17:54:24 +0300
Avi Kivity a...@redhat.com wrote:
Please post a simple patch that uses two get_user()s for that case
(64-bit pte on 32-bit host). Then work with the x86 tree to see if
they'll accept 64-bit get_user(), and once they do, we can go back to a
simple get_user() again.
I made a patch which seems to reflect what you said!
If this kind of fix is OK with you, I'll test on both x86_32 and x86_64,
and send the patch with some changelog tomorrow.
Thanks,
Takuya
---
arch/x86/kvm/paging_tmpl.h | 16 +++-
1 files changed, 15 insertions(+), 1 deletions(-)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index a32a1c8..1e44969 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -115,6 +115,20 @@ static unsigned FNAME(gpte_access)(struct kvm_vcpu *vcpu,
pt_element_t gpte)
return access;
}
+static int FNAME(read_gpte)(pt_element_t *pte, pt_element_t __user *ptep_user)
+{
+#if defined(CONFIG_X86_32) (PTTYPE == 64)
+ u32 *p = (u32 *)pte;
+ u32 __user *p_user = (u32 __user *)ptep_user;
+
+ if (get_user(*p, p_user))
+ return -EFAULT;
+ return get_user(*(p + 1), p_user + 1);
+#else
+ return get_user(*pte, ptep_user);
+#endif
+}
+
/*
* Fetch a guest pte for a guest virtual address
*/
@@ -185,7 +199,7 @@ walk:
}
ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
- if (get_user(pte, ptep_user)) {
+ if (FNAME(read_gpte)(pte, ptep_user)) {
present = false;
break;
}
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/26/2011 06:13 PM, Takuya Yoshikawa wrote: Please post a simple patch that uses two get_user()s for that case (64-bit pte on 32-bit host). Then work with the x86 tree to see if they'll accept 64-bit get_user(), and once they do, we can go back to a simple get_user() again. btw, I think we can use __get_user() here since the address must have been already validated. Yes, I thought that at first. But don't we need to care KVM's address translation bugs? It's a pity to do a runtime check when we can do a setup time check instead. So let's review the setup code and then use __get_user(). -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/26/2011 07:26 PM, Takuya Yoshikawa wrote: On Tue, 26 Apr 2011 17:54:24 +0300 Avi Kivitya...@redhat.com wrote: Please post a simple patch that uses two get_user()s for that case (64-bit pte on 32-bit host). Then work with the x86 tree to see if they'll accept 64-bit get_user(), and once they do, we can go back to a simple get_user() again. I made a patch which seems to reflect what you said! If this kind of fix is OK with you, I'll test on both x86_32 and x86_64, and send the patch with some changelog tomorrow. Yes, that looks right. -- error compiling committee.c: too many arguments to function -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 2011-04-21 17:34, Takuya Yoshikawa wrote:
From: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp
This patch optimizes the guest page table walk by using get_user()
instead of copy_from_user().
With this patch applied, paging64_walk_addr_generic() has become
about 0.5us to 1.0us faster on my Phenom II machine with NPT on.
Signed-off-by: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp
---
arch/x86/kvm/paging_tmpl.h | 23 ---
1 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 74f8567..825d953 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -117,6 +117,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker
*walker,
gva_t addr, u32 access)
{
pt_element_t pte;
+ pt_element_t __user *ptep_user;
gfn_t table_gfn;
unsigned index, pt_access, uninitialized_var(pte_access);
gpa_t pte_gpa;
@@ -152,6 +153,9 @@ walk:
pt_access = ACC_ALL;
for (;;) {
+ gfn_t real_gfn;
+ unsigned long host_addr;
+
index = PT_INDEX(addr, walker-level);
table_gfn = gpte_to_gfn(pte);
@@ -160,9 +164,22 @@ walk:
walker-table_gfn[walker-level - 1] = table_gfn;
walker-pte_gpa[walker-level - 1] = pte_gpa;
- if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, pte,
- offset, sizeof(pte),
- PFERR_USER_MASK|PFERR_WRITE_MASK)) {
+ real_gfn = mmu-translate_gpa(vcpu, gfn_to_gpa(table_gfn),
+ PFERR_USER_MASK|PFERR_WRITE_MASK);
+ if (real_gfn == UNMAPPED_GVA) {
+ present = false;
+ break;
+ }
+ real_gfn = gpa_to_gfn(real_gfn);
+
+ host_addr = gfn_to_hva(vcpu-kvm, real_gfn);
+ if (kvm_is_error_hva(host_addr)) {
+ present = false;
+ break;
+ }
+
+ ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
+ if (get_user(pte, ptep_user)) {
This doesn't work for x86-32: pte is 64 bit, but get_user is only
defined up to 32 bit on that platform.
Avi, what's your 32-bit buildbot doing? :)
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On Mon, 25 Apr 2011 10:04:43 +0200
Jan Kiszka jan.kis...@web.de wrote:
+
+ ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
+ if (get_user(pte, ptep_user)) {
This doesn't work for x86-32: pte is 64 bit, but get_user is only
defined up to 32 bit on that platform.
Avi, what's your 32-bit buildbot doing? :)
Jan
Sorry, I did not test on x86_32.
Introducing a wrapper function with ifdef would be the best way?
Takuya
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 2011-04-25 10:32, Takuya Yoshikawa wrote:
On Mon, 25 Apr 2011 10:04:43 +0200
Jan Kiszka jan.kis...@web.de wrote:
+
+ ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
+ if (get_user(pte, ptep_user)) {
This doesn't work for x86-32: pte is 64 bit, but get_user is only
defined up to 32 bit on that platform.
Avi, what's your 32-bit buildbot doing? :)
Jan
Sorry, I did not test on x86_32.
Introducing a wrapper function with ifdef would be the best way?
Maybe you could also add the missing 64-bit get_user for x86-32. Given
that we have a corresponding put_user, I wonder why the get_user was
left out.
Jan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On Mon, 25 Apr 2011 11:15:20 +0200 Jan Kiszka jan.kis...@web.de wrote: Sorry, I did not test on x86_32. Introducing a wrapper function with ifdef would be the best way? Maybe you could also add the missing 64-bit get_user for x86-32. Given that we have a corresponding put_user, I wonder why the get_user was left out. Jan Google said that there was a similar talk on LKML in 2004. On that threads, Linus explained how to tackle on the 64-bit get_user implementation. But I could not see what happened after that. Takuya -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
On 04/21/2011 06:34 PM, Takuya Yoshikawa wrote: From: Takuya Yoshikawayoshikawa.tak...@oss.ntt.co.jp This patch optimizes the guest page table walk by using get_user() instead of copy_from_user(). With this patch applied, paging64_walk_addr_generic() has become about 0.5us to 1.0us faster on my Phenom II machine with NPT on. Applied, thanks. Care to send a follow-on patch that makes cmpxchg_gpte() use ptep_user instead of calculating it by itself? -- I have a truly marvellous patch that fixes the bug which this signature is too narrow to contain. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1 v2] KVM: MMU: Optimize guest page table walk
From: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp
This patch optimizes the guest page table walk by using get_user()
instead of copy_from_user().
With this patch applied, paging64_walk_addr_generic() has become
about 0.5us to 1.0us faster on my Phenom II machine with NPT on.
Signed-off-by: Takuya Yoshikawa yoshikawa.tak...@oss.ntt.co.jp
---
arch/x86/kvm/paging_tmpl.h | 23 ---
1 files changed, 20 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 74f8567..825d953 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -117,6 +117,7 @@ static int FNAME(walk_addr_generic)(struct guest_walker
*walker,
gva_t addr, u32 access)
{
pt_element_t pte;
+ pt_element_t __user *ptep_user;
gfn_t table_gfn;
unsigned index, pt_access, uninitialized_var(pte_access);
gpa_t pte_gpa;
@@ -152,6 +153,9 @@ walk:
pt_access = ACC_ALL;
for (;;) {
+ gfn_t real_gfn;
+ unsigned long host_addr;
+
index = PT_INDEX(addr, walker-level);
table_gfn = gpte_to_gfn(pte);
@@ -160,9 +164,22 @@ walk:
walker-table_gfn[walker-level - 1] = table_gfn;
walker-pte_gpa[walker-level - 1] = pte_gpa;
- if (kvm_read_guest_page_mmu(vcpu, mmu, table_gfn, pte,
- offset, sizeof(pte),
- PFERR_USER_MASK|PFERR_WRITE_MASK)) {
+ real_gfn = mmu-translate_gpa(vcpu, gfn_to_gpa(table_gfn),
+ PFERR_USER_MASK|PFERR_WRITE_MASK);
+ if (real_gfn == UNMAPPED_GVA) {
+ present = false;
+ break;
+ }
+ real_gfn = gpa_to_gfn(real_gfn);
+
+ host_addr = gfn_to_hva(vcpu-kvm, real_gfn);
+ if (kvm_is_error_hva(host_addr)) {
+ present = false;
+ break;
+ }
+
+ ptep_user = (pt_element_t __user *)((void *)host_addr + offset);
+ if (get_user(pte, ptep_user)) {
present = false;
break;
}
--
1.7.1
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
