Re: [PATCH v4 10/18] xen/riscv: implement p2m_set_range()

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 3263 bytes --]


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
   

[-- Attachment #2: Type: text/html, Size: 4510 bytes --]