Re: [PATCH 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
Avi Kivity wrote:
On 06/15/2010 05:46 AM, Xiao Guangrong wrote:
Introduce gfn_to_page_atomic() and gfn_to_pfn_atomic(), those
functions is fast path and can used in atomic context, the later
patch will use those
@@ -942,6 +942,41 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
}
EXPORT_SYMBOL_GPL(gfn_to_hva);
+static pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
+{
+struct page *page[1];
+int npages;
+pfn_t pfn;
+
+npages = __get_user_pages_fast(addr, 1, 1, page);
+
+if (unlikely(npages != 1)) {
+if (is_hwpoison_address(addr)) {
+get_page(hwpoison_page);
+return page_to_pfn(hwpoison_page);
+}
+get_page(bad_page);
+return page_to_pfn(bad_page);
+} else
+pfn = page_to_pfn(page[0]);
+
+return pfn;
+}
Too much duplication. How about putting the tail end of the function in
a common helper (with an inatomic flag)?
OK, will cleanup it in the next version.
--
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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On Tue, Jun 15, 2010 at 02:22:06PM +0300, Avi Kivity wrote: Too much duplication. How about putting the tail end of the function in a common helper (with an inatomic flag)? btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us. Where is is_hwpoison_address() coming from? I can't find it anywhere. Anyways hwpoison will not remove the page as long as there is a reference to it, so as long as you get the reference race free against another task you're ok. Of course there might be always an error in between and the hardware may poison the data, but we don't try to handle all kernel errors. -Andi -- a...@linux.intel.com -- Speaking for myself only. -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On 06/16/2010 10:59 AM, Andi Kleen wrote: On Tue, Jun 15, 2010 at 02:22:06PM +0300, Avi Kivity wrote: Too much duplication. How about putting the tail end of the function in a common helper (with an inatomic flag)? btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us. Where is is_hwpoison_address() coming from? I can't find it anywhere. kvm.git master mm/memory-failure.c (19564281fe). Anyways hwpoison will not remove the page as long as there is a reference to it, so as long as you get the reference race free against another task you're ok. Of course there might be always an error in between and the hardware may poison the data, but we don't try to handle all kernel errors. The page is fine, the page tables are not. Another task can munmap() the thing while is_hwpoison_address() is running. -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
The page is fine, the page tables are not. Another task can munmap() the thing while is_hwpoison_address() is running. Ok that boils down to me not seeing that source. If it accesses the page tables yes then it's racy. But whoever looked up the page tables in the first place should have returned an -EFAULT. There's no useful address attached to poison. -Andi -- a...@linux.intel.com -- Speaking for myself only. -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On 06/16/2010 11:49 AM, Andi Kleen wrote: The page is fine, the page tables are not. Another task can munmap() the thing while is_hwpoison_address() is running. Ok that boils down to me not seeing that source. If it accesses the page tables yes then it's racy. But whoever looked up the page tables in the first place should have returned an -EFAULT. There's no useful address attached to poison. We need to distinguish between genuine -EFAULT and poisoned address. That's why I suggested get_user_pages_ptes_fast. You can return page = NULL (-EFAULT) and the pte in the same go. No race, and useful for other cases. -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On Tue, Jun 15, 2010 at 7:22 PM, Avi Kivity a...@redhat.com wrote: btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us. Andi/Huang? One way of fixing it is get_user_pages_ptes_fast(), which also returns the pte, also atomically. I want it for other reasons as well (respond to a read fault by gupping the page for read, but allowing write access if the pte indicates it is writeable). Yes. is_hwpoison_address() is racy. But I think it is not absolutely necessary to call is_hwpoison_address() in hva_to_pfn_atomic(), is it? For is_hwpoison_address() in hva_to_pfn(), we can protect it with mmap_sem. Best Regards, Huang Ying -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On 06/16/2010 01:51 PM, huang ying wrote: On Tue, Jun 15, 2010 at 7:22 PM, Avi Kivitya...@redhat.com wrote: btw, is_hwpoison_address() is racy. While it looks up the address, some other task can unmap the page tables under us. Andi/Huang? One way of fixing it is get_user_pages_ptes_fast(), which also returns the pte, also atomically. I want it for other reasons as well (respond to a read fault by gupping the page for read, but allowing write access if the pte indicates it is writeable). Yes. is_hwpoison_address() is racy. But I think it is not absolutely necessary to call is_hwpoison_address() in hva_to_pfn_atomic(), is it? We can probably ignore it, yes. For is_hwpoison_address() in hva_to_pfn(), we can protect it with mmap_sem. Not very appealing, but should work. -- 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 3/6] KVM: MMU: introduce gfn_to_page_atomic() and gfn_to_pfn_atomic()
On 06/15/2010 05:46 AM, Xiao Guangrong wrote:
Introduce gfn_to_page_atomic() and gfn_to_pfn_atomic(), those
functions is fast path and can used in atomic context, the later
patch will use those
@@ -942,6 +942,41 @@ unsigned long gfn_to_hva(struct kvm *kvm, gfn_t gfn)
}
EXPORT_SYMBOL_GPL(gfn_to_hva);
+static pfn_t hva_to_pfn_atomic(struct kvm *kvm, unsigned long addr)
+{
+ struct page *page[1];
+ int npages;
+ pfn_t pfn;
+
+ npages = __get_user_pages_fast(addr, 1, 1, page);
+
+ if (unlikely(npages != 1)) {
+ if (is_hwpoison_address(addr)) {
+ get_page(hwpoison_page);
+ return page_to_pfn(hwpoison_page);
+ }
+ get_page(bad_page);
+ return page_to_pfn(bad_page);
+ } else
+ pfn = page_to_pfn(page[0]);
+
+ return pfn;
+}
Too much duplication. How about putting the tail end of the function in
a common helper (with an inatomic flag)?
btw, is_hwpoison_address() is racy. While it looks up the address, some
other task can unmap the page tables under us.
Andi/Huang?
One way of fixing it is get_user_pages_ptes_fast(), which also returns
the pte, also atomically. I want it for other reasons as well (respond
to a read fault by gupping the page for read, but allowing write access
if the pte indicates it is writeable).
--
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
