Re: [PATCH v3 4/9] xen/riscv: setup fixmap mapping

[Jan Beulich <jbeulich@xxxxxxxx>]

On 24.07.2024 17:31, 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_VIRT_SIZE, XEN_VIRT_END.
> 
> Also, the check of Xen size was updated in the riscv/lds.S
> script to use XEN_VIRT_SIZE instead of a hardcoded constant.
> 
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

Yet set_fixmap() isn't introduced here, so effectively it's all dead
code. This could do with mentioning, as I at least would expect
set_fixmap() to be usable once fixmaps are properly set up.

> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -66,6 +66,14 @@
>  #define SLOTN_ENTRY_BITS        (HYP_PT_ROOT_LEVEL * VPN_BITS + PAGE_SHIFT)
>  #define SLOTN(slot)             (_AT(vaddr_t, slot) << SLOTN_ENTRY_BITS)
>  
> +#define XEN_VIRT_SIZE           MB(2)
> +
> +#define BOOT_FDT_VIRT_START     (XEN_VIRT_START + XEN_VIRT_SIZE)
> +#define BOOT_FDT_VIRT_SIZE      MB(4)
> +
> +#define FIXMAP_BASE             (BOOT_FDT_VIRT_START + BOOT_FDT_VIRT_SIZE)
> +#define FIXMAP_ADDR(n)          (FIXMAP_BASE + (n) * PAGE_SIZE)
> +
>  #if RV_STAGE1_MODE == SATP_MODE_SV39
>  #define XEN_VIRT_START 0xFFFFFFFFC0000000
>  #elif RV_STAGE1_MODE == SATP_MODE_SV48

Related to my earlier comment: Is there a particular reason that what
you add cannot live _below_ the XEN_VIRT_START definitions, so things
actually appear in order?

> --- /dev/null
> +++ b/xen/arch/riscv/include/asm/fixmap.h
> @@ -0,0 +1,44 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * 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 */
> +#define FIX_MISC (FIX_PMAP_END + 1)  /* Ephemeral mappings of hardware */
> +
> +#define FIX_LAST FIX_MISC
> +
> +#define FIXADDR_START FIXMAP_ADDR(0)
> +#define FIXADDR_TOP FIXMAP_ADDR(FIX_LAST)

This doesn't look right, even if it matches what Arm has. Following
what we have for x86, the page at FIXADDR_TOP should be a guard page,
i.e. not have any mapping ever put there. Whereas you put FIX_MISC's
mapping there. This then results in ...

> +#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[];
> +
> +#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);

... this being wrong, i.e. triggering for FIX_MISC. Again, same issue
on Arm afaics, just that it would trigger for FIX_PMAP_END there. (Cc-ing
the other two Arm maintainers for that.)

> @@ -81,6 +82,14 @@ 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)
> +{
> +    RISCV_FENCE(rw, rw);
> +    *p = pte;
> +    RISCV_FENCE(rw, rw);
> +}

Why the first of the two fences? And isn't having the 2nd here going
to badly affect batch updates of page tables?

> @@ -191,6 +195,49 @@ 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, tmp;
> +    unsigned int i;
> +
> +    BUILD_BUG_ON(FIX_LAST >= PAGETABLE_ENTRIES);
> +
> +    pte = &stage1_pgtbl_root[pt_index(HYP_PT_ROOT_LEVEL, FIXMAP_ADDR(0))];
> +
> +    /*
> +     * In RISC-V page table levels are enumerated from Lx to L0 where

Nit: s/enumerated/numbered/ ?

> +     * x is the highest page table level for currect  MMU mode ( for example,
> +     * for Sv39 has 3 page tables so the x = 2 (L2 -> L1 -> L0) ).
> +     *
> +     * In this cycle we want to find L1 page table because as L0 page table
> +     * xen_fixmap[] will be used.
> +     *
> +     * i is defined ( HYP_PT_ROOT_LEVEL - 1 ) becuase pte for L2 ( in
> +     * case of Sv39 ) has been recieved above.
> +     */
> +    for ( i = HYP_PT_ROOT_LEVEL - 1; i != 0; i-- )

I think the subtracting of 1 is unhelpful here. Think of another  case where
you want to walk down to L0. How would you express that with a similar for()
construct. It would imo be more natural to use

    for ( i = HYP_PT_ROOT_LEVEL; i > 1; i-- )

here (and then in that hypothetical other case

    for ( i = HYP_PT_ROOT_LEVEL; i > 0; i-- )

), and then the last part of the comment likely wouldn't be needed either.
However, considering ...

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

... the use of i here, it may instead want to be

    for ( i = HYP_PT_ROOT_LEVEL; i-- > 1; )

> +    }
> +
> +    BUG_ON(pte_is_valid(*pte));
> +
> +    tmp = paddr_to_pte(LINK_TO_LOAD((unsigned long)&xen_fixmap), PTE_TABLE);

I'm a little puzzled by the use of LINK_TO_LOAD() (and LOAD_TO_LINK() a
little further up) here. Don't you have functioning __pa() and __va()?

> +    write_pte(pte, tmp);
> +
> +    sfence_vma();
> +
> +    printk("(XEN) fixmap is mapped\n");

Why the (XEN) prefix? And perhaps why the printk() in the first place?

Jan