[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Xen-devel] [PATCH v3 10/38] arm/p2m: Move hostp2m init/teardown to individual functions
On 02/09/16 10:09, Sergej Proskurin wrote:
Hi Julien,
On 09/01/2016 07:36 PM, Julien Grall wrote:
Hello Sergej,
On 16/08/16 23:16, Sergej Proskurin wrote:
---
xen/arch/arm/p2m.c | 71
+++++++++++++++++++++++++++++++++++++++++------
xen/include/asm-arm/p2m.h | 11 ++++++++
2 files changed, 73 insertions(+), 9 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index e859fca..9ef19d4 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -1245,27 +1245,53 @@ static void p2m_free_vmid(struct domain *d)
spin_unlock(&vmid_alloc_lock);
}
-void p2m_teardown(struct domain *d)
+/* Reset this p2m table to be empty. */
+void p2m_flush_table(struct p2m_domain *p2m)
{
- struct p2m_domain *p2m = p2m_get_hostp2m(d);
- struct page_info *pg;
+ struct page_info *page, *pg;
+ unsigned int i;
+
+ if ( p2m->root )
+ {
+ page = p2m->root;
+
+ /* Clear all concatenated first level pages. */
s/first/root/
Ok.
+ for ( i = 0; i < P2M_ROOT_PAGES; i++ )
+ clear_and_clean_page(page + i);
Clearing live page table like that is not safe. Each entry (64-bit)
should be written atomically to avoid breaking coherency (e.g if the MMU
is walking the page table at the same time). However you don't know the
implementation of clear_and_clean_page.
The function p2m_flush_table gets called by the altp2m subsystem
indrectly through the function p2m_teardown_one when the associated view
gets destroyed. In this case we should not worry about crashing the
domain, as the altp2m views are not active anyway.
The function altp2m_reset calls the function p2m_flush_table directly
(with active altp2m views), however, locks the p2m before flushing the
table. I did not find any locks on page-granularity, so please provide
me with further information about the solution you had in mind.
I never mentioned any locking problem. As said in my previous mail, the
altp2m may still be in use by another vCPU. So the MMU (i.e the
hardware) may want do a table walk whilst you modify the entry.
The MMU is reading the entry (64-bit) at once so it also expects the
entry to be modified atomically. However, you don't know the
implementation of clean_and_clean_page. The function might write 32-bit
at the time, which means that the MMU will see bogus entry. At best it
will lead to a crash, at worst open a security issue.
Also, this adds a small overhead when tearing down a p2m because the
clear is not necessary.
The p2m views are cleared very rarely so the overhead is really minimal
as it affects clearing the root tables.
You seem to forget the p2m teardown is also called during domain
destruction.
Besides the function
altp2m_reset calls the function p2m_flush_table and assumes that the
root tables are cleared as well. If the root tables would not be cleared
at this point, stalled entries in the altp2m views that got wiped out in
the hostp2m would make the system unstable.
As I already mentioned in the previous version, this code was not
present before and based of the description of this commit the patch is
supposed to only move code around. This is not the case here.
radix_tree_destroy(&p2m->mem_access_settings, NULL);
}
-int p2m_init(struct domain *d)
+int p2m_init_one(struct domain *d, struct p2m_domain *p2m)
{
- struct p2m_domain *p2m = p2m_get_hostp2m(d);
int rc = 0;
rwlock_init(&p2m->lock);
@@ -1278,11 +1304,14 @@ int p2m_init(struct domain *d)
return rc;
p2m->max_mapped_gfn = _gfn(0);
- p2m->lowest_mapped_gfn = _gfn(ULONG_MAX);
+ p2m->lowest_mapped_gfn = INVALID_GFN;
Why this change?
Since we compare the gfn's with INVALID_GFN throughout the code it makes
sense to use the macro instead of a hardcoded value.
Please don't do unnecessary change. This patch is complex enough to review.
Regards,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel
|
