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

[oleksii.kurochko@xxxxxxxxx]

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[]?

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

~ Oleksii

> 
> >    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);
> >        ...
> >    }
>