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

[Julien Grall <julien@xen.org>]

Hi Oleksii,

On 12/07/2024 17:22, Oleksii Kurochko wrote:
> Introduce a function to set up fixmap mappings and L0 page
> table for fixmap.
> 
> Additionally, defines were introduced in riscv/config.h to
> calculate the FIXMAP_BASE address.
> This involved introducing BOOT_FDT_VIRT_{START, SIZE} and
> XEN_SIZE, XEN_VIRT_END.
> 
> Also, the check of Xen size was updated in the riscv/lds.S
> script to use XEN_SIZE instead of a hardcoded constant.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
> ---
> Changes in V2:
>   - newly introduced patch
> ---
>   xen/arch/riscv/include/asm/config.h |  9 ++++++
>   xen/arch/riscv/include/asm/fixmap.h | 48 +++++++++++++++++++++++++++++
>   xen/arch/riscv/include/asm/mm.h     |  2 ++
>   xen/arch/riscv/include/asm/page.h   |  7 +++++
>   xen/arch/riscv/mm.c                 | 35 +++++++++++++++++++++
>   xen/arch/riscv/setup.c              |  2 ++
>   xen/arch/riscv/xen.lds.S            |  2 +-
>   7 files changed, 104 insertions(+), 1 deletion(-)
>   create mode 100644 xen/arch/riscv/include/asm/fixmap.h
> 
> diff --git a/xen/arch/riscv/include/asm/config.h b/xen/arch/riscv/include/asm/config.h
> index 50583aafdc..3275477c17 100644
> --- 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)

NIT: I would name it XEN_VIRT_SIZE to be consistent with the start/end.

> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)
Can we get away with not introducing *_END and just use START, SIZE? The 
reason I am asking is with "end" it is never clear whether it is 
inclusive or exclusive. For instance, here you use an exclusive range 
but ...

> +
> +#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)
> +
>   #define FRAMETABLE_SCALE_FACTOR  (PAGE_SIZE/sizeof(struct page_info))
>   #define FRAMETABLE_SIZE_IN_SLOTS (((DIRECTMAP_SIZE / SLOTN(1)) / FRAMETABLE_SCALE_FACTOR) + 1)
>   
> diff --git a/xen/arch/riscv/include/asm/fixmap.h b/xen/arch/riscv/include/asm/fixmap.h
> new file mode 100644
> index 0000000000..fcfb82d69c
> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fixmap.h
> @@ -0,0 +1,48 @@
> +/*
> + * fixmap.h: compile-time virtual memory allocation
> + */
> +#ifndef __ASM_FIXMAP_H
> +#define __ASM_FIXMAP_H
> +
> +#include <xen/bug.h>
> +#include <xen/page-size.h>
> +#include <xen/pmap.h>
> +
> +#include <asm/page.h>
> +
> +/* Fixmap slots */
> +#define FIX_PMAP_BEGIN (0) /* Start of PMAP */
> +#define FIX_PMAP_END (FIX_PMAP_BEGIN + NUM_FIX_PMAP - 1) /* End of PMAP */

... here is seems to be inclusive. Furthermore if you had 32-bit address 
space, it is also quite easy to have to create a region right at the top 
of it. So when END is exclusive, it would become 0.

So on Arm, we decided to start to get rid of "end". I would consider to 
do the same on RISC-V for new functions.

> +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of hardware */

Are you going to use this fixmap? If not, then I would consider to 
remove it.

> +
> +#define FIX_LAST FIX_MISC
> +
> +#define FIXADDR_START FIXMAP_ADDR(0)
> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)
> +
> +#ifndef __ASSEMBLY__
> +
> +/*
> + * Direct access to xen_fixmap[] should only happen when {set,
> + * clear}_fixmap() is unusable (e.g. where we would end up to
> + * recursively call the helpers).
> + */
> +extern pte_t xen_fixmap[];
> +
> +/* Map a page in a fixmap entry */
> +extern void set_fixmap(unsigned int map, mfn_t mfn, unsigned int attributes);
> +/* Remove a mapping from a fixmap entry */
> +extern void clear_fixmap(unsigned int map);

Neither of the functions seem to be implemented in this patch. Can you 
clarify what's the plan for them?

Also, I know that for x86/arm, we have some function prefixed with 
extern. But AFAIK, we are trying to get rid of them.

In any case, I think for RISC-V we need some consistency. For instance, 
here you define with extern but...

> +
> +#define fix_to_virt(slot) ((void *)FIXMAP_ADDR(slot))
> +
> +static inline unsigned int virt_to_fix(vaddr_t vaddr)
> +{
> +    BUG_ON(vaddr >= FIXADDR_TOP || vaddr < FIXADDR_START);
> +
> +    return ((vaddr - FIXADDR_START) >> PAGE_SHIFT);
> +}
> +
> +#endif /* __ASSEMBLY__ */
> +
> +#endif /* __ASM_FIXMAP_H */
> diff --git a/xen/arch/riscv/include/asm/mm.h b/xen/arch/riscv/include/asm/mm.h
> index 25af9e1aaa..a0bdc2bc3a 100644
> --- a/xen/arch/riscv/include/asm/mm.h
> +++ b/xen/arch/riscv/include/asm/mm.h
> @@ -255,4 +255,6 @@ static inline unsigned int arch_get_dma_bitsize(void)
>       return 32; /* TODO */
>   }
>   
> +void setup_fixmap_mappings(void);

... here it is without.

> +
>   #endif /* _ASM_RISCV_MM_H */
> diff --git a/xen/arch/riscv/include/asm/page.h b/xen/arch/riscv/include/asm/page.h
> index c831e16417..cbbf3656d1 100644
> --- 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");
> +}
> +
>   #endif /* __ASSEMBLY__ */
>   
>   #endif /* _ASM_RISCV_PAGE_H */
> diff --git a/xen/arch/riscv/mm.c b/xen/arch/riscv/mm.c
> index 7d09e781bf..d69a174b5d 100644
> --- 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];

Can you add a BUILD_BUG_ON() to check that the number of entries in the 
fixmap will never be above PAGETABLE_ENTRIES?

> +
>   #define HANDLE_PGTBL(curr_lvl_num)                                          \
>       index = pt_index(curr_lvl_num, page_addr);                              \
>       if ( pte_is_valid(pgtbl[index]) )                                       \
> @@ -191,6 +194,38 @@ static bool __init check_pgtbl_mode_support(struct mmu_desc *mmu_desc,
>       return is_mode_supported;
>   }
>   
> +void __init setup_fixmap_mappings(void)
> +{
> +    pte_t *pte;
> +    unsigned int i;
> +
> +    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
> +
> +    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )

I am a little bit confused with the - 1. Is this because you only want 
to map at L1 (I am not sure if this is the correct naming for RISC-V)?

In any case, I think it would be worth a comment.

> +    {
> +        BUG_ON(!pte_is_valid(*pte));
> +
> +        pte = (pte_t *)LOAD_TO_LINK(pte_to_paddr(*pte));
> +        pte = &pte[pt_index(i, FIXMAP_ADDR(0))];
> +    }
> +
> +    BUG_ON( pte_is_valid(*pte) );

Coding style: BUG_ON(pte_is_valid(*pte));

> +
> +    if ( !pte_is_valid(*pte) )

I am a bit confused with this check. Above, Xen will crash if the PTE is 
valid. So why do we need a runtime check?

> +    {
> +        pte_t tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);
> +
> +        write_pte(pte, tmp);
> +
> +        printk("(XEN) fixmap is mapped\n");
> +    }
> +
> +    /*
> +     * We only need the zeroeth table allocated, but not the PTEs set, because
> +     * set_fixmap() will set them on the fly.

This function doesn't seem to exists yet (?).

> +     */
> +}
> +
>   /*
>    * setup_initial_pagetables:
>    *
> diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c
> index 4defad68f4..13f0e8c77d 100644
> --- a/xen/arch/riscv/setup.c
> +++ b/xen/arch/riscv/setup.c
> @@ -46,6 +46,8 @@ void __init noreturn start_xen(unsigned long bootcpu_id,
>       test_macros_from_bug_h();
>   #endif
>   
> +    setup_fixmap_mappings();
> +
>       printk("All set up\n");
>   
>       for ( ;; )
> diff --git a/xen/arch/riscv/xen.lds.S b/xen/arch/riscv/xen.lds.S
> index 070b19d915..63b1dd7bb6 100644
> --- a/xen/arch/riscv/xen.lds.S
> +++ b/xen/arch/riscv/xen.lds.S
> @@ -181,6 +181,6 @@ ASSERT(!SIZEOF(.got.plt),  ".got.plt non-empty")
>    * Changing the size of Xen binary can require an update of
>    * PGTBL_INITIAL_COUNT.
>    */
> -ASSERT(_end - _start <= MB(2), "Xen too large for early-boot assumptions")
> +ASSERT(_end - _start <= XEN_SIZE, "Xen too large for early-boot assumptions")
>   
>   ASSERT(_ident_end - _ident_start <= IDENT_AREA_SIZE, "identity region is too big");

Cheers,

-- 
Julien Grall