Re: [PATCH v2 5/8] xen/riscv: introduce asm/pmap.h header

[Julien Grall <julien@xxxxxxx>]

Hi,

On 22/07/2024 15:44, Oleksii Kurochko wrote:
> On Mon, 2024-07-22 at 14:54 +0200, Jan Beulich wrote:
> > On 12.07.2024 18:22, Oleksii Kurochko wrote:
> > > --- /dev/null
> > > +++ b/xen/arch/riscv/include/asm/pmap.h
> > > @@ -0,0 +1,28 @@
> > > +#ifndef __ASM_PMAP_H__
> > > +#define __ASM_PMAP_H__
> > > +
> > > +#include <xen/bug.h>
> > > +#include <xen/mm.h>
> > > +
> > > +#include <asm/fixmap.h>
> > > +
> > > +static inline void arch_pmap_map(unsigned int slot, mfn_t mfn)
> > > +{
> > > +    pte_t *entry = &xen_fixmap[slot];
> > > +    pte_t pte;
> > > +
> > > +    ASSERT(!pte_is_valid(*entry));
> > > +
> > > +    pte = mfn_to_xen_entry(mfn, PAGE_HYPERVISOR_RW);
> > > +    pte.pte |= PTE_LEAF_DEFAULT;
> > > +    write_pte(entry, pte);
> > > +}
> > > +
> > > +static inline void arch_pmap_unmap(unsigned int slot)
> > > +{
> > > +    pte_t pte = {};
> > > +
> > > +    write_pte(&xen_fixmap[slot], pte);
> > > +}
> > Why are these not using set_fixmap() / clear_fixmap() respectively?
> They haven't been introduced yet. And I thought that these fucntion are
> used only in pmap_{un}map() and that is the reason why I decided to not
> introduce them. But while writing the answer on another comment, I
> found other places where set_fixmap() / clear_fixmap() are used, so I
> will introduce them and reuse here.
I am guessing you are going to implement set_fixmap()/clear_fixmap() 
using map_pages_to_xen(). If so, for early boot you are going to end up 
in a circular loop because map_pages_to_xen() will likely use pmap() 
which will call set_fixmap().
There is a big comment in common/pmap.c which explain why arch_pmap_* 
was introduced rather than calling *_fixmap() directly:
    /*
     * 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().
     */

Cheers,

--
Julien Grall