[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v4 08/11] xen/arm: Handle remove foreign mapping
On 12/16/2013 04:33 PM, Ian Campbell wrote:
On Mon, 2013-12-16 at 16:26 +0000, Julien Grall wrote:
On 12/16/2013 03:40 PM, Ian Campbell wrote:
On Mon, 2013-12-16 at 15:34 +0000, Julien Grall wrote:
On 12/16/2013 11:51 AM, Tim Deegan wrote:
At 19:37 +0000 on 13 Dec (1386959858), Julien Grall wrote:
@@ -693,12 +694,21 @@ long do_memory_op(unsigned long cmd,
XEN_GUEST_HANDLE_PARAM(void) arg)
return rc;
}
- page = get_page_from_gfn(d, xrfp.gpfn, NULL, P2M_ALLOC);
+ /*
+ * If autotranslate guest, (eg pvh), the gfn could be mapped to a mfn
+ * from foreign domain by the user space tool during domain creation.
+ * We need to check for that, free it up from the p2m, and release
+ * refcnt on it. In such a case, page would be NULL and the following
+ * call would not have refcnt'd the page.
+ */
+ page = get_page_from_gfn(d, xrfp.gpfn, &p2mt, P2M_ALLOC);
if ( page )
{
guest_physmap_remove_page(d, xrfp.gpfn, page_to_mfn(page), 0);
put_page(page);
}
+ else if ( p2m_is_foreign(p2mt) )
+ rc = p2m_remove_foreign(d, xrfp.gpfn);
This doesn't seem like the right interface -- having special cases
like this in the callers is something we slipped into in x86 for a lot
of the paging/sharing code and it's not nice. I think maybe we can
just have get_page_from_gfn() DTRT for foreign (and grant) entries.
Also, the comment will have been out of data by the time the x86
version of this code is finished, as we won't be handling the refcount
at this level. :)
I will remove the comment and modify get_page_from_gfn to handle foreign
mapping.
You'll want to coordinate with Mukesh on that latter I think.
Ian.
I have reworked this patch. I get a simpler patch:
commit aab2e5d2ae7d0fa87c74cae2f22044f87be33f70
Author: Julien Grall <julien.grall@xxxxxxxxxx>
Date: Fri Dec 13 16:51:03 2013 +0000
xen/arm: Handle remove foreign mapping
Modify get_page_from_gfn to take reference on foreign mapping. This will
avoid
specific handling in the common code.
Signed-off-by: Julien Grall <julien.grall@xxxxxxxxxx>
---
Changes in v5:
- Remove specific p2m handling in common code
- Handle foreign mapping in get_page_from_gfn
Changes in v4:
- Split patch #6 from dom0 pvh series v6.2 to retrieve only common
code.
- Rework commit title
- Rename xen_rem_foreign_from_p2m in p2m_remove_foreign
- Get the mfn from the pte. We are not sure that maddr given in
parameters is valid
Changes in v3:
- Move put_page in create_p2m_entries
- Move xenmem_rem_foreign_from_p2m in arch/arm/p2m.c
Changes in v2:
- Introduce the patch
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 39d8a03..f7bd7e2 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -317,10 +317,21 @@ static int create_p2m_entries(struct domain *d,
break;
case REMOVE:
{
- lpae_t pte;
+ lpae_t pte = third[third_table_offset(addr)];
+ unsigned long mfn;
+
+ maddr = (pte.bits & PADDR_MASK & PAGE_MASK);
I thought we had a macro for this, but apparently not. While looking for
it I spotted that x86 has pte_to_mfn, which sounds like a useful
innovation... (not essential as part of this series though).
This function is only defined for mini-os
(extras/mini-os/include/x86/arch_mm.h).
+ mfn = paddr_to_pfn(maddr);
+
+ /* TODO: Handle other p2m type */
+ if ( pte.p2m.valid && p2m_is_foreign(pte.p2m.type) )
+ {
+ ASSERT(mfn_valid(mfn));
Something somewhere is making sure we don't put foreign MMIO regions
into the p2m, right?
We retrieve the mfn via page_to_mfn, so the mfn should be valid.
+ put_page(mfn_to_page(mfn));
+ }
+
memset(&pte, 0x00, sizeof(pte));
write_pte(&third[third_table_offset(addr)], pte);
- maddr += PAGE_SIZE;
}
break;
}
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 0eb07a8..e0b58da 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -122,9 +122,21 @@ static inline struct page_info *get_page_from_gfn(
if ( !mfn_valid(mfn) )
return NULL;
page = mfn_to_page(mfn);
- if ( !get_page(page, d) )
- return NULL;
- return page;
+
+ if ( get_page(page, d) )
This isn't noisy (even at debug level) on failure, I thought so?
Might be safer (and TBH more logical) to move it after the foreign
special case.
Will do.
+ return page;
+
+ /* get_page won't work on foreign mapping because the page doesn't
+ * belong to the current domain.
+ */
+ if ( p2mt == p2m_map_foreign )
+ {
+ struct domain *fdom = page_get_owner_and_reference(page);
+ ASSERT(fdom != NULL);
ASSERT(fdom != d)
?
Both are valid. We need to make sure that the page belongs to a domain,
and then it's not the current domain.
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
