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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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.

>  Then ...
> 
> >  #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct
> > page_info))
> >  #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) /
> > FRAMETABLE_SCALE_FACTOR) + 1)
> 
> ... this would also stay next to DIRECTMAP_*, which it uses.
> 
> > --- a/xen/arch/riscv/include/asm/page.h
> > +++ b/xen/arch/riscv/include/asm/page.h
> > @@ -81,6 +81,13 @@ static inline void flush_page_to_ram(unsigned
> > long mfn, bool sync_icache)
> >      BUG_ON("unimplemented");
> >  }
> >  
> > +/* Write a pagetable entry. */
> > +static inline void write_pte(pte_t *p, pte_t pte)
> > +{
> > +    *p = pte;
> > +    asm volatile ("sfence.vma");
> 
> When they don't have operands, asm()-s are volatile anyway (being
> explicit
> about this may still be desirable, yes). But: Can you get away
> without
> operands here? Don't you need a memory clobber for the fence to
> actually
> remain where it is needed?
It should be "::memory" here. Thanks.

> 
> Also, nit (style): Blanks missing.
> 
> > --- 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);
   }

~ Oleksii