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

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

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


On 7/7/25 9:20 AM, Jan Beulich wrote:
> 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?

It sounds fine to me. I'll use suggested name.

Just want to notice that other arches also have the same function
for the same purpose with the same name.
Does it make sense then to change a name for all arches?

>
>>>> +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.

Thanks, now it is clear what you meant.

>
>>>> +{
>>>> +    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.

Then probably p2me_type_is_valid(struct p2m_domain *p2m, pte_t pte) would better.

>   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.

I am not really sure that I fully understand the question.
Do you ask if a guest can do something which will lead to a call of p2m_set_entry()
with p2m_invalid argument?
If yes, then it seems like it will be done only in case of p2m_remove_mapping() what
will mean that alongside with p2m_invalid INVALID_MFN will be also passed, what means
this entry isn't expected to be used anymore.

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

By using of "p2me_" predicate I wanted to express that not PTE's valid bit will be
checked, but the type saved in radix tree will be used.
As suggested above probably it will be better drop "e" too and just use p2m_type_is_valid().

>
>>>> +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.

Agree, it shouldn't be here. Thanks.

>
>>>> @@ -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()?

It is much better.

>
>>>> +        /*
>>>> +         * 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?

You're right — blindly unmapping the range assumes that no entries were valid
beforehand and I missed that it could be that something valid was mapped before
p2m_set_entry(sgfn,...,smfn) was called.
But then I am not really understand why it won't be an issue if will know
how many GFNs were successfully mapped.

~ Oleksii

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