On 9/26/25 10:58 AM, Oleksii Kurochko wrote:
+ /*
+ * Free the entry only if the original pte was valid and the base
+ * is different (to avoid freeing when permission is changed).
+ *
+ * If previously MFN 0 was mapped and it is going to be removed
+ * and considering that during removing MFN 0 is used then `entry`
+ * and `new_entry` will be the same and p2m_free_subtree() won't be
+ * called. This case is handled explicitly.
+ */
+ if ( pte_is_valid(orig_pte) &&
+ (!mfn_eq(pte_get_mfn(*entry), pte_get_mfn(orig_pte)) ||
+ (removing_mapping && mfn_eq(pte_get_mfn(*entry), _mfn(0)))) )
+ p2m_free_subtree(p2m, orig_pte, level);
I continue to fail to understand why the MFN would matter here.
My understanding is that if, for the same GFN, the MFN changes fromMFN_1 to
MFN_2, then we need to update any references on the page referenced by
|orig_pte| to ensure the proper reference counter is maintained for the page
pointed to byMFN_1.
Isn't the
need to free strictly tied to a VALID -> NOT VALID transition? A permission
change simply retains the VALID state of an entry.
It covers a case when removing happens and probably in this case we don't need
to check specifically for mfn(0) case "mfn_eq(pte_get_mfn(*entry), _mfn(0))",
but it would be enough to check that pte_is_valid(entry) instead:
...
(removing_mapping && !pte_is_valid(entry)))) )
Or only check removing_mapping variable as `entry` would be invalided by the
code above anyway. So we will get:
+ if ( pte_is_valid(orig_pte) &&
+ (!mfn_eq(pte_get_mfn(*entry), pte_get_mfn(orig_pte)) ||
removing_mapping) )
+ p2m_free_subtree(p2m, orig_pte, level);
Does it make sense now?
Not really, sorry. Imo the complicated condition indicates that something is
wrong (or at least inefficient) here.
Then, in the case of aVALID -> VALID transition, where the MFN is changed for
the
same PTE, should something be done with the old MFN (e.g.,
calling|p2m_put_page()|
for it), or can freeing the old MFN be delayed
until|domain_relinquish_resources() |is called? If so, wouldn’t that lead to a
situation where many old MFNs accumulate
and cannot be re-used until|domain_relinquish_resources()| (or another function
that
explicitly frees pages) is invoked?
If we only need to care about theVALID -> NOT VALID transition, doesn’t that
mean
|p2m_free_subtree()| should be called only when a removal actually occurs?
I've decided to "simplify" the original condition to:
/*
* In case of a VALID -> INVALID transition, the original PTE should
* always be freed.
*
* In case of a VALID -> VALID transition, the original PTE should be
* freed only if the MFNs are different. If the MFNs are the same
* (i.e., only permissions differ), there is no need to free the
* original PTE.
*/
if ( pte_is_valid(orig_pte) &&
(!pte_is_valid(*entry) ||
!mfn_eq(pte_get_mfn(*entry), pte_get_mfn(orig_pte))) )
{
I hope it would make more sense.
~ Oleksii
