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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 30.08.2024 13:01, oleksii.kurochko@xxxxxxxxx wrote:
> On Wed, 2024-08-28 at 12:44 +0200, Jan Beulich wrote:
>> 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.
> We can't move declaration of xen_fixmap[] to the patch where
> set_fixmap() will be introduced ( which uses PMAP for domain map page
> infrastructure ) as xen_fixmap[] is used in the current patch.
> 
> And we can't properly provide the proper description with the function
> which will be introduced one day in the future ( what can be not good
> too ). I came up with the following description in the comment above
> xen_fixmap[] declaration:
>    /*
>     * 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).
>     * 
>     * One such case is pmap_map() where set_fixmap() can not be used.
>     * It happens because PMAP is used 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.
>     * Modification of the PTEs directly instead in arch_pmap_map().
>     * The same is true for pmap_unmap().
>     */
> 
> Could it be an option just to drop the comment for now at all as at the
> moment there is no such restriction with the usage of
> {set,clear}_fixmap() and xen_fixmap[]?

The comment isn't the right place to explain things here, imo. It's the
patch description where unexpected aspects need shedding light on.

>>> 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.
> Yes, it is true. No recursion will happen in this case but if to look
> at the implementation of set_fixmap() for other Arm or x86 ( but I am
> not sure that x86 uses PMAP inside map_pages_to_xen() ) they are using
> map_pages_to_xen().

There's no PMAP so far on x86.

Jan