Hi Henry,
On 17/10/2022 17:51, Henry Wang wrote:
@@ -1736,7 +1739,20 @@ void p2m_final_teardown(struct domain *d)
if ( !p2m->domain )
return;
+ /*
+ * No need to call relinquish_p2m_mapping() here because
+ * p2m_final_teardown() is called either after
domain_relinquish_resources()
+ * where relinquish_p2m_mapping() has been called, or from failure path of
+ * domain_create()/arch_domain_create() where mappings that require
+ * p2m_put_l3_page() should never be created. For the latter case, also see
+ * comment on top of the p2m_set_entry() for more info.
+ */
+
+ BUG_ON(p2m_teardown(d, false));
ASSERT(page_list_empty(&p2m->pages));
+
+ while ( p2m_teardown_allocation(d) == -ERESTART )
+ continue; /* No preemption support here */
ASSERT(page_list_empty(&d->arch.paging.p2m_freelist));
if ( p2m->root )
@@ -1784,6 +1800,8 @@ int p2m_init(struct domain *d)
As Andrew pointed out the change in p2m_init() will end up leaking
either the VMID or the root table.
Andrew's patch #1 [1] should help to solve the problem. So I would
suggest to rebase on top of it.
Other than that, the logic looks good to me. This is even knowning that
Andrew said the code is buggy. I spent some time starring at the code
and can't figure out where the issue lies because p2m_teardown() will do
nothing when the list is empty. If it is not empty, then it is
guaranteed that the VMID and root table is allocated. So the code looks
functional but just not efficient.
We already discussed the latter point earlier in the review and agreed
to look at improve it post 4.17.
For the former, I am happy to be proven wrong. But this is going to
require more substantial explanations.
Cheers,
--
Julien Grall
