Re: [PATCH v2 4/8] xen/riscv: setup fixmap mapping

[oleksii.kurochko@xxxxxxxxx]

On Mon, 2024-07-22 at 19:04 +0200, oleksii.kurochko@xxxxxxxxx wrote:
> On Mon, 2024-07-22 at 17:25 +0200, Jan Beulich wrote:
> > On 22.07.2024 16:36, Oleksii wrote:
> > > On Mon, 2024-07-22 at 14:42 +0200, Jan Beulich wrote:
> > > > On 12.07.2024 18:22, Oleksii Kurochko wrote:
> > > > > --- a/xen/arch/riscv/include/asm/config.h
> > > > > +++ b/xen/arch/riscv/include/asm/config.h
> > > > > @@ -74,11 +74,20 @@
> > > > >  #error "unsupported RV_STAGE1_MODE"
> > > > >  #endif
> > > > >  
> > > > > +#define XEN_SIZE                MB(2)
> > > > > +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
> > > > > +
> > > > > +#define BOOT_FDT_VIRT_START     XEN_VIRT_END
> > > > > +#define BOOT_FDT_VIRT_SIZE      MB(4)
> > > > > +
> > > > >  #define DIRECTMAP_SLOT_END      509
> > > > >  #define DIRECTMAP_SLOT_START    200
> > > > >  #define DIRECTMAP_VIRT_START    SLOTN(DIRECTMAP_SLOT_START)
> > > > >  #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END) -
> > > > > SLOTN(DIRECTMAP_SLOT_START))
> > > > >  
> > > > > +#define FIXMAP_BASE             (BOOT_FDT_VIRT_START +
> > > > > BOOT_FDT_VIRT_SIZE)
> > > > > +#define FIXMAP_ADDR(n)          (FIXMAP_BASE + (n) *
> > > > > PAGE_SIZE)
> > > > 
> > > > Why exactly do you insert this here, and not adjacent to
> > > > BOOT_FDT_VIRT_*,
> > > > which it actually is adjacent with?
> > > I tried to follow alphabetical order.
> > 
> > Oh, X before B (just making fun) ... Anyway, my take here is that
> > sorting
> > by address is going to be more helpful.
> > 
> > > > > --- a/xen/arch/riscv/mm.c
> > > > > +++ b/xen/arch/riscv/mm.c
> > > > > @@ -49,6 +49,9 @@ stage1_pgtbl_root[PAGETABLE_ENTRIES];
> > > > >  pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
> > > > >  stage1_pgtbl_nonroot[PGTBL_INITIAL_COUNT *
> > > > > PAGETABLE_ENTRIES];
> > > > >  
> > > > > +pte_t __section(".bss.page_aligned") __aligned(PAGE_SIZE)
> > > > > +xen_fixmap[PAGETABLE_ENTRIES];
> > > > 
> > > > Any reason this cannot be static?
> > > It will be used by pmap:
> > >    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);
> > >    }
> > 
> > Yet as asked there - shouldn't that be set_fixmap() and
> > clear_fixmap()?
> It should be, I'll rework that in the next patch version.
It couldn't be set_fixmap() and clear_fixmap() as I am going to
implement them using map_pages_to_xen() because:
...
    /*
     * 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);
...

~ Oleksii