Re: [PATCH v2 11/17] xen/riscv: implement p2m_set_entry() and __p2m_set_entry()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.07.2025 17:01, Oleksii Kurochko wrote:
> On 7/1/25 3:49 PM, Jan Beulich wrote:
>> On 10.06.2025 15:05, Oleksii Kurochko wrote:
>>> This patch introduces p2m_set_entry() and its core helper __p2m_set_entry() 
>>> for
>>> RISC-V, based loosely on the Arm implementation, with several 
>>> RISC-V-specific
>>> modifications.
>>>
>>> Key differences include:
>>> - TLB Flushing: RISC-V allows caching of invalid PTEs and does not require
>>>    break-before-make (BBM). As a result, the flushing logic is simplified.
>>>    TLB invalidation can be deferred until p2m_write_unlock() is called.
>>>    Consequently, the p2m->need_flush flag is always considered true and is
>>>    removed.
>>> - Page Table Traversal: The order of walking the page tables differs from 
>>> Arm,
>>>    and this implementation reflects that reversed traversal.
>>> - Macro Adjustments: The macros P2M_ROOT_LEVEL, P2M_ROOT_ORDER, and
>>>    P2M_ROOT_PAGES are updated to align with the new RISC-V implementation.
>>>
>>> The main functionality is in __p2m_set_entry(), which handles mappings 
>>> aligned
>>> to page table block entries (e.g., 1GB, 2MB, or 4KB with 4KB granularity).
>>>
>>> p2m_set_entry() breaks a region down into block-aligned mappings and calls
>>> __p2m_set_entry() accordingly.
>>>
>>> Stub implementations (to be completed later) include:
>>> - p2m_free_entry()
>> What would a function of this name do?
> 
> Recursively visiting all leaf PTE's for sub-tree behind an entry, then calls
> put_page() (which will free if there is no any reference to this page),
> freeing intermediate page table (after all entries were freed) by removing
> it from d->arch.paging.freelist, and removes correspondent page of 
> intermediate page
> table from p2m->pages list.
> 
>> You can clear entries, but you can't
>> free them, can you?
> 
> Is is a question regarding terminology?

Yes. If one sees a call to a function, it should be possible to at least
roughly know what it does without needing to go look at the implementation.

> I can't free entry itself, but a page table or
> a page (if it is a leaf entry) on which it points could free.

Then e.g. pte_free_subtree() or some such?

>>> +static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
>> The rule of thumb is to have inline functions only in header files, leaving
>> decisions to the compiler elsewhere.
> 
> I am not sure what you mean in the second part (after coma) of your sentence.

The compiler does its own inlining decisions quite fine when it can see all
call sites (as is the case for static functions). Hence in general you want
to omit "inline" there. Except of course in header files, where non-inline
static-s are a problem.

>>> +{
>>> +    panic("%s: isn't implemented for now\n", __func__);
>>> +
>>> +    return false;
>>> +}
>> For this function in particular, though: Besides the "p2me" in the name
>> being somewhat odd (supposedly page table entries here are simply pte_t),
>> how is this going to be different from pte_is_valid()?
> 
> pte_is_valid() is checking a real bit of PTE, but p2me_is_valid() is checking
> what is a type stored in the radix tree (p2m->p2m_types):
>    /*
>     * In the case of the P2M, the valid bit is used for other purpose. Use
>     * the type to check whether an entry is valid.
>     */
>    static inline bool p2me_is_valid(struct p2m_domain *p2m, pte_t pte)
>    {
>        return p2m_type_radix_get(p2m, pte) != p2m_invalid;
>    }
> 
> It is done to track which page was modified by a guest.

But then (again) the name doesn't convey what the function does. Plus
can't a guest also arrange for an entry's type to move to p2m_invalid?
That's then still an entry that was modified by the guest.

Overall I think I'm lacking clarity what you mean to use this predicate
for.

>>> +static inline void p2m_write_pte(pte_t *p, pte_t pte, bool clean_pte)
>>> +{
>>> +    write_pte(p, pte);
>>> +    if ( clean_pte )
>>> +        clean_dcache_va_range(p, sizeof(*p));
>>> +}
>>> +
>>> +static inline void p2m_remove_pte(pte_t *p, bool clean_pte)
>>> +{
>>> +    pte_t pte;
>>> +
>>> +    memset(&pte, 0x00, sizeof(pte));
>>> +    p2m_write_pte(p, pte, clean_pte);
>>> +}
>> May I suggest "clear" instead of "remove" and plain 0 instead of 0x00
>> (or simply give the variable a trivial initializer)?
> 
> Sure, I will rename and use plain 0.
> 
>>
>> As to the earlier function that I commented on: Seeing the names here,
>> wouldn't p2m_pte_is_valid() be a more consistent name there?
> 
> Then all p2me_*() should be updated to p2m_pte_*().
> 
> But initial logic was that p2me = p2m entry = p2m page table entry.
> 
> Probably we can just return back to the prefix p2m_ as based on arguments
> it is clear that it is a function for working with P2M's PTE.

In the end it's up to you. Having thought about it some more, perhaps
p2me_*() is still quite helpful to separate from functions dealing with
P2Ms as a while, and to also avoid the verbosity of p2m_pte_*().

>>> +{
>>> +    unsigned int level;
>>> +    unsigned int target = page_order / PAGETABLE_ORDER;
>>> +    pte_t *entry, *table, orig_pte;
>>> +    int rc;
>>> +    /* A mapping is removed if the MFN is invalid. */
>>> +    bool removing_mapping = mfn_eq(smfn, INVALID_MFN);
>>> +    DECLARE_OFFSETS(offsets, gfn_to_gaddr(sgfn));
>>> +
>>> +    ASSERT(p2m_is_write_locked(p2m));
>>> +
>>> +    /*
>>> +     * Check if the level target is valid: we only support
>>> +     * 4K - 2M - 1G mapping.
>>> +     */
>>> +    ASSERT(target <= 2);
>> No provisions towards the division that produced the value having left
>> a remainder?
> 
> The way the order is initialized will always result in division without
> a remainder.
> 
> If it makes sense, the|ASSERT()| could be updated to ensure that the order
> is always a multiple of|PAGETABLE_ORDER|:
>    ASSERT((target <= 2) && !IS_ALIGNED(page_order, PAGETABLE_ORDER));

Except that the ! looks wrong here.

>>> @@ -332,7 +542,55 @@ static int p2m_set_entry(struct p2m_domain *p2m,
>>>                            p2m_type_t t,
>>>                            p2m_access_t a)
>>>   {
>>> -    return -EOPNOTSUPP;
>>> +    int rc = 0;
>>> +
>>> +    /*
>>> +     * Any reference taken by the P2M mappings (e.g. foreign mapping) will
>>> +     * be dropped in relinquish_p2m_mapping(). As the P2M will still
>>> +     * be accessible after, we need to prevent mapping to be added when the
>>> +     * domain is dying.
>>> +     */
>>> +    if ( unlikely(p2m->domain->is_dying) )
>>> +        return -ENOMEM;
>> Why ENOMEM?
> 
> I expect that when a domain is dying, it means there’s no point in using its
> memory—either because it's no longer available or it has already been freed.
> Basically, no memory.

That can end up odd for call sites. Please consider using e.g. EACCES.

>>> +    while ( nr )
>> Why's there a loop here? The function name uses singular, i.e. means to
>> create exactly one entry.
> 
> I will rename the function to  p2m_set_entries().

Or maybe p2m_set_range()?

>>> +        /*
>>> +         * Don't take into account the MFN when removing mapping (i.e
>>> +         * MFN_INVALID) to calculate the correct target order.
>>> +         *
>>> +         * XXX: Support superpage mappings if nr is not aligned to a
>>> +         * superpage size.
>>> +         */
>> Does this really need leaving as a to-do?
> 
> I think so, yes. It won’t break the current workflow if|nr| isn’t aligned,
> a smaller order will simply be chosen.

Well, my question was more like "Isn't it simple enough to cover the case
right away?"

>>> +        mask = !mfn_eq(smfn, INVALID_MFN) ? mfn_x(smfn) : 0;
>>> +        mask |= gfn_x(sgfn) | nr;
>>> +
>>> +        for ( ; i != 0; i-- )
>>> +        {
>>> +            if ( !(mask & (BIT(XEN_PT_LEVEL_ORDER(i), UL) - 1)) )
>>> +            {
>>> +                    order = XEN_PT_LEVEL_ORDER(i);
>>> +                    break;
>> Nit: Style.
>>
>>> +            }
>>> +        }
>>> +
>>> +        rc = __p2m_set_entry(p2m, sgfn, order, smfn, t, a);
>>> +        if ( rc )
>>> +            break;
>>> +
>>> +        sgfn = gfn_add(sgfn, (1 << order));
>>> +        if ( !mfn_eq(smfn, INVALID_MFN) )
>>> +           smfn = mfn_add(smfn, (1 << order));
>>> +
>>> +        nr -= (1 << order);
>> Throughout maybe better be safe right away and use 1UL?
>>
>>> +    }
>>> +
>>> +    return rc;
>>>   }
>> How's the caller going to know how much of the range was successfully
>> mapped?
> 
> There is no such option. Do other arches do that? I mean returns somehow
> the number of successfully mapped (sgfn,smfn).

On x86 we had to introduce some not very nice code to cover for the absence
of proper handling there. For a new port I think it wants at least seriously
considering not to repeat such a potentially unhelpful pattern.

>> That part may need undoing (if not here, then in the caller),
>> or a caller may want to retry.
> 
> So the caller in the case if rc != 0, can just undoing the full range
> (by using the same sgfn, nr, smfn).

Can it? How would it know what the original state was?

Jan