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

[Oleksii Kurochko <inisider@xxxxxxxxx>]

On Mon, 2024-07-22 at 15:48 +0100, Julien Grall wrote:
> 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().
I am going to implement that in the following way as I faced the
described by you issue when I first time tried to implement it using
map_pages_to_xen():
   /* Map a 4k page in a fixmap entry */
   void set_fixmap(unsigned map, mfn_t mfn, unsigned int flags)
   {
       pte_t pte;
   
       pte = mfn_to_xen_entry(mfn, flags);
       pte.pte |= PTE_LEAF_DEFAULT;
       write_pte(&xen_fixmap[pt_index(0, FIXMAP_ADDR(map))], pte);
   }
   
   /* Remove a mapping from a fixmap entry */
   void clear_fixmap(unsigned map)
   {
       pte_t pte = {0};
       write_pte(&xen_fixmap[pt_index(0, FIXMAP_ADDR(map))], pte);
   }
   
~ Oleksii

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