|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] dom0 linux 3.6.0-rc4, crash due to ballooning althoug dom0_mem=X, max:X set
On Wed, 5 Sep 2012, Konrad Rzeszutek Wilk wrote:
> On Tue, Sep 04, 2012 at 04:27:20PM -0400, Robert Phillips wrote:
> > Ben,
> >
> > You have asked me to provide the rationale behind the gnttab_old_mfn patch,
> > which you emailed to Sander earlier today.
> > Here are my findings.
> >
> > I found that xen_blkbk_map() in drivers/block/xen-blkback/blkback.c has
> > changed from our previous version. It now calls gnttab_map_refs() in
> > drivers/xen/grant-table.c.
> >
> > That function first calls HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
> > ... ) and then calls m2p_add_override() in p2m.c
>
> And HYPERVISOR_grant_table_op .. would populate map_ops[i].bus_addr with the
> machine address..
>
> > which is where I made my change.
> >
> > The unpatched code was saving the pfn's old mfn in kmap_op->dev_bus_addr.
> >
> > kmap_op is of type struct gnttab_map_grant_ref. That data type is used to
> > record grant table mappings so later they can be unmapped correctly.
>
> Right, but the blkback makes a distinction by passing NULL as kmap_op, which
> means it should
> use the old mechanism. Meaning that once the hypercall is done, the
> map_ops[i].bus_addr is not
> used anymore..
>
> >
> > The problem with saving the old mfn in kmap_op->dev_bus_addr is that it is
> > later overwritten by __gnttab_map_grant_ref() in xen/common/grant_table.c
>
> Uh, so the problem of saving the old mfn in dev_bus_addr has been there for a
> long long time then?
> Even before this patch set?
I think that Robert identified the real problem: dev_bus_addr shouldn't
have been used here. However the bug only shows up if we are batching
the grant table operations, that we started doing since
f62805f1f30a40e354bd036b4cb799863a39be4b.
That's why Sander's bisection found that
f62805f1f30a40e354bd036b4cb799863a39be4b is the culprit.
However the fix is incorrect because it is modifying a struct that is
part of the Xen ABI.
I am appending an alternative fix that doesn't need any changes to
public headers.
Sander, could you please let me know if it fixes the problem for you?
---
diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
index 93971e8..472b9b7 100644
--- a/arch/x86/include/asm/xen/page.h
+++ b/arch/x86/include/asm/xen/page.h
@@ -51,7 +51,8 @@ extern unsigned long set_phys_range_identity(unsigned long
pfn_s,
extern int m2p_add_override(unsigned long mfn, struct page *page,
struct gnttab_map_grant_ref *kmap_op);
-extern int m2p_remove_override(struct page *page, bool clear_pte);
+extern int m2p_remove_override(struct page *page,
+ struct gnttab_map_grant_ref *kmap_op);
extern struct page *m2p_find_override(unsigned long mfn);
extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long
pfn);
diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
index 64effdc..2825594 100644
--- a/arch/x86/xen/p2m.c
+++ b/arch/x86/xen/p2m.c
@@ -734,9 +734,6 @@ int m2p_add_override(unsigned long mfn, struct page *page,
xen_mc_issue(PARAVIRT_LAZY_MMU);
}
- /* let's use dev_bus_addr to record the old mfn instead */
- kmap_op->dev_bus_addr = page->index;
- page->index = (unsigned long) kmap_op;
}
spin_lock_irqsave(&m2p_override_lock, flags);
list_add(&page->lru, &m2p_overrides[mfn_hash(mfn)]);
@@ -763,7 +760,8 @@ int m2p_add_override(unsigned long mfn, struct page *page,
return 0;
}
EXPORT_SYMBOL_GPL(m2p_add_override);
-int m2p_remove_override(struct page *page, bool clear_pte)
+int m2p_remove_override(struct page *page,
+ struct gnttab_map_grant_ref *kmap_op)
{
unsigned long flags;
unsigned long mfn;
@@ -793,10 +791,8 @@ int m2p_remove_override(struct page *page, bool clear_pte)
WARN_ON(!PagePrivate(page));
ClearPagePrivate(page);
- if (clear_pte) {
- struct gnttab_map_grant_ref *map_op =
- (struct gnttab_map_grant_ref *) page->index;
- set_phys_to_machine(pfn, map_op->dev_bus_addr);
+ set_phys_to_machine(pfn, page->index);
+ if (kmap_op != NULL) {
if (!PageHighMem(page)) {
struct multicall_space mcs;
struct gnttab_unmap_grant_ref *unmap_op;
@@ -808,13 +804,13 @@ int m2p_remove_override(struct page *page, bool clear_pte)
* issued. In this case handle is going to -1 because
* it hasn't been modified yet.
*/
- if (map_op->handle == -1)
+ if (kmap_op->handle == -1)
xen_mc_flush();
/*
- * Now if map_op->handle is negative it means that the
+ * Now if kmap_op->handle is negative it means that the
* hypercall actually returned an error.
*/
- if (map_op->handle == GNTST_general_error) {
+ if (kmap_op->handle == GNTST_general_error) {
printk(KERN_WARNING "m2p_remove_override: "
"pfn %lx mfn %lx, failed to
modify kernel mappings",
pfn, mfn);
@@ -824,8 +820,8 @@ int m2p_remove_override(struct page *page, bool clear_pte)
mcs = xen_mc_entry(
sizeof(struct gnttab_unmap_grant_ref));
unmap_op = mcs.args;
- unmap_op->host_addr = map_op->host_addr;
- unmap_op->handle = map_op->handle;
+ unmap_op->host_addr = kmap_op->host_addr;
+ unmap_op->handle = kmap_op->handle;
unmap_op->dev_bus_addr = 0;
MULTI_grant_table_op(mcs.mc,
@@ -836,10 +832,9 @@ int m2p_remove_override(struct page *page, bool clear_pte)
set_pte_at(&init_mm, address, ptep,
pfn_pte(pfn, PAGE_KERNEL));
__flush_tlb_single(address);
- map_op->host_addr = 0;
+ kmap_op->host_addr = 0;
}
- } else
- set_phys_to_machine(pfn, page->index);
+ }
/* p2m(m2p(mfn)) == FOREIGN_FRAME(mfn): the mfn is already present
* somewhere in this domain, even before being added to the
diff --git a/drivers/block/xen-blkback/blkback.c
b/drivers/block/xen-blkback/blkback.c
index 73f196c..c6decb9 100644
--- a/drivers/block/xen-blkback/blkback.c
+++ b/drivers/block/xen-blkback/blkback.c
@@ -337,7 +337,7 @@ static void xen_blkbk_unmap(struct pending_req *req)
invcount++;
}
- ret = gnttab_unmap_refs(unmap, pages, invcount, false);
+ ret = gnttab_unmap_refs(unmap, NULL, pages, invcount);
BUG_ON(ret);
}
diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
index 1ffd03b..7f12416 100644
--- a/drivers/xen/gntdev.c
+++ b/drivers/xen/gntdev.c
@@ -314,8 +314,9 @@ static int __unmap_grant_pages(struct grant_map *map, int
offset, int pages)
}
}
- err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages + offset,
- pages, true);
+ err = gnttab_unmap_refs(map->unmap_ops + offset,
+ use_ptemod ? map->kmap_ops + offset : NULL, map->pages
+ offset,
+ pages);
if (err)
return err;
diff --git a/drivers/xen/grant-table.c b/drivers/xen/grant-table.c
index 0bfc1ef..0067266 100644
--- a/drivers/xen/grant-table.c
+++ b/drivers/xen/grant-table.c
@@ -870,7 +870,8 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
EXPORT_SYMBOL_GPL(gnttab_map_refs);
int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct page **pages, unsigned int count, bool clear_pte)
+ struct gnttab_map_grant_ref *kmap_ops,
+ struct page **pages, unsigned int count)
{
int i, ret;
bool lazy = false;
@@ -888,7 +889,8 @@ int gnttab_unmap_refs(struct gnttab_unmap_grant_ref
*unmap_ops,
}
for (i = 0; i < count; i++) {
- ret = m2p_remove_override(pages[i], clear_pte);
+ ret = m2p_remove_override(pages[i], kmap_ops ?
+ &kmap_ops[i] : NULL);
if (ret)
return ret;
}
diff --git a/include/xen/grant_table.h b/include/xen/grant_table.h
index 11e27c3..f19fff8 100644
--- a/include/xen/grant_table.h
+++ b/include/xen/grant_table.h
@@ -187,6 +187,7 @@ int gnttab_map_refs(struct gnttab_map_grant_ref *map_ops,
struct gnttab_map_grant_ref *kmap_ops,
struct page **pages, unsigned int count);
int gnttab_unmap_refs(struct gnttab_unmap_grant_ref *unmap_ops,
- struct page **pages, unsigned int count, bool clear_pte);
+ struct gnttab_map_grant_ref *kunmap_ops,
+ struct page **pages, unsigned int count);
#endif /* __ASM_GNTTAB_H__ */
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |