Re: [PATCH v5 2/7] xen/riscv: set up fixmap mappings

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.08.2024 11:53, oleksii.kurochko@xxxxxxxxx wrote:
> On Tue, 2024-08-27 at 12:29 +0200, Jan Beulich wrote:
>>>
>>> +
>>> +/*
>>> + * Direct access to xen_fixmap[] should only happen when {set,
>>> + * clear}_fixmap() is unusable (e.g. where we would end up to
>>> + * recursively call the helpers).
>>> + */
>>> +extern pte_t xen_fixmap[];
>>
>> I'm afraid I keep being irritated by the comment: What recursive use
>> of
>> helpers is being talked about here? I can't see anything recursive in
>> this
>> patch. If this starts happening with a subsequent patch, then you
>> have
>> two options: Move the declaration + comment there, or clarify in the
>> description (in enough detail) what this is about.
> This comment is added because of:
> ```
> void *__init pmap_map(mfn_t mfn)
>   ...
>        /*
>         * We cannot use set_fixmap() here. We use PMAP when the domain map
>         * page infrastructure is not yet initialized, so
>    map_pages_to_xen() called
>         * by set_fixmap() needs to map pages on demand, which then calls
>    pmap()
>         * again, resulting in a loop. Modify the PTEs directly instead.
>    The same
>         * is true for pmap_unmap().
>         */
>        arch_pmap_map(slot, mfn);
>    ...
> ```
> And it happens because set_fixmap() could be defined using generic PT
> helpers

As you say - could be. If I'm not mistaken no set_fixmap() implementation
exists even by the end of the series. Fundamentally I'd expect set_fixmap()
to (possibly) use xen_fixmap[] directly. That in turn ...

> so what will lead to recursive behaviour when when there is no
> direct map:

... would mean no recursion afaict. Hence why clarification is needed as
to what's going on here _and_ what's planned.

Jan

>    static pte_t *map_table(mfn_t mfn)
>    {
>        /*
>         * During early boot, map_domain_page() may be unusable. Use the
>         * PMAP to map temporarily a page-table.
>         */
>        if ( system_state == SYS_STATE_early_boot )
>            return pmap_map(mfn);
>        ...
>    }