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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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 a VALID -> 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 the VALID -> 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