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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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?

Jan