Re: [PATCH v1 4/5] xen/riscv: introduce device tree maping function

[Jan Beulich <jbeulich@xxxxxxxx>]

On 03.07.2024 12:42, Oleksii Kurochko wrote:
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
>  xen/arch/riscv/include/asm/config.h |  6 +++++
>  xen/arch/riscv/include/asm/mm.h     |  2 ++
>  xen/arch/riscv/mm.c                 | 37 +++++++++++++++++++++++++----
>  3 files changed, 40 insertions(+), 5 deletions(-)

I don't think a change like this can come without any description.

> --- a/xen/arch/riscv/include/asm/config.h
> +++ b/xen/arch/riscv/include/asm/config.h
> @@ -74,6 +74,9 @@
>  #error "unsupported RV_STAGE1_MODE"
>  #endif
>  
> +#define XEN_SIZE                MB(2)
> +#define XEN_VIRT_END            (XEN_VIRT_START + XEN_SIZE)

Probably wants accompanying by an assertion in the linker script. Or else
how would one notice when Xen grows bigger than this?

> @@ -99,6 +102,9 @@
>  #define VMAP_VIRT_START         SLOTN(VMAP_SLOT_START)
>  #define VMAP_VIRT_SIZE          GB(1)
>  
> +#define BOOT_FDT_VIRT_START     XEN_VIRT_END
> +#define BOOT_FDT_VIRT_SIZE      MB(4)

Is the 4 selected arbitrarily, or derived from something?

Also maybe better to keep these #define-s sorted by address? (As to "keep":
I didn't check whether they currently are.)

> --- 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* early_fdt_map(paddr_t fdt_paddr);

Nit: * and blank want to change places.

> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -1,5 +1,6 @@
>  /* SPDX-License-Identifier: GPL-2.0-only */
>  
> +#include <xen/bootfdt.h>
>  #include <xen/bug.h>
>  #include <xen/compiler.h>
>  #include <xen/init.h>
> @@ -7,7 +8,9 @@
>  #include <xen/macros.h>
>  #include <xen/mm.h>
>  #include <xen/pfn.h>
> +#include <xen/libfdt/libfdt.h>

This wants to move up, to retain sorting.

>  #include <xen/sections.h>
> +#include <xen/sizes.h>
>  
>  #include <asm/early_printk.h>
>  #include <asm/csr.h>
> @@ -20,7 +23,7 @@ struct mmu_desc {
>      unsigned int pgtbl_count;
>      pte_t *next_pgtbl;
>      pte_t *pgtbl_base;
> -};
> +} mmu_desc = { CONFIG_PAGING_LEVELS, 0, NULL, 0 };

__initdata and static?

> @@ -39,9 +42,11 @@ static unsigned long __ro_after_init phys_offset;
>   * isn't 2 MB aligned.
>   *
>   * CONFIG_PAGING_LEVELS page tables are needed for the identity mapping,
> - * except that the root page table is shared with the initial mapping
> + * except that the root page table is shared with the initial mapping.
> + *
> + * CONFIG_PAGING_LEVELS page tables are needed for device tree mapping.
>   */
> -#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 2 + 1)
> +#define PGTBL_INITIAL_COUNT ((CONFIG_PAGING_LEVELS - 1) * 3 + 1 + 1)

Considering what would happen if two or three more of such requirements
were added, maybe better

#define PGTBL_INITIAL_COUNT (CONFIG_PAGING_LEVELS * 3 - 1)

? However, why is it CONFIG_PAGING_LEVELS that's needed, and not
CONFIG_PAGING_LEVELS - 1? The top level table is the same as the
identity map's, isn't it?

> @@ -296,6 +299,30 @@ unsigned long __init calc_phys_offset(void)
>      return phys_offset;
>  }
>  
> +void* __init early_fdt_map(paddr_t fdt_paddr)

See earlier remark regarding * placement.

Jan