Re: [PATCH v1 5/6] xen/riscv: implement relocate_fdt()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.11.2024 13:50, Oleksii Kurochko wrote:
> relocate_fdt() relocates FDT to Xen heap instead of using early mapping
> as it is expected that discard_initial_modules() ( is supposed to call
> in the future ) discards the FDT boot module and remove_early_mappings()
> destroys the early mapping.
> 
> To implement that the following things are introduced as they are called
> by internals of xmalloc_bytes() which is used in relocate_fdt():
> 1. As RISC-V may have non-coherent access for RAM ( f.e., in case
>    of non-coherent IO devices ) flush_page_to_ram() is implemented
>    to ensure that cache and RAM are consistent for such platforms.

This is a detail of the page allocator, yes. It can then be viewed as also
a detail of xmalloc() et al, but I consider the wording a little misleading.

> 2. copy_from_paddr() to copy FDT from a physical address to allocated
>    by xmalloc_bytes() in Xen heap.

This doesn't look to be related to the internals of xmalloc() et al.

> 3. virt_to_page() to convert virtual address to page. Also introduce
>    directmap_virt_end to check that VA argument of virt_to_page() is
>    inside directmap region.

This is a need of free_xenheap_pages(), yes; see remark on point 1.

> @@ -148,8 +150,12 @@ static inline void *page_to_virt(const struct page_info 
> *pg)
>  /* Convert between Xen-heap virtual addresses and page-info structures. */
>  static inline struct page_info *virt_to_page(const void *v)
>  {
> -    BUG_ON("unimplemented");
> -    return NULL;
> +    unsigned long va = (unsigned long)v;
> +
> +    ASSERT(va >= DIRECTMAP_VIRT_START);
> +    ASSERT(va <= directmap_virt_end);

Why the difference compared to virt_to_maddr()?

Also recall my comment on one of your earlier series, regarding inclusive
vs exclusive ranges. Can that please be sorted properly as a prereq, to
avoid extending the inconsistency?

> @@ -172,10 +173,15 @@ static inline void invalidate_icache(void)
>  #define clear_page(page) memset((void *)(page), 0, PAGE_SIZE)
>  #define copy_page(dp, sp) memcpy(dp, sp, PAGE_SIZE)
>  
> -/* TODO: Flush the dcache for an entire page. */
>  static inline void flush_page_to_ram(unsigned long mfn, bool sync_icache)
>  {
> -    BUG_ON("unimplemented");
> +    void *v = map_domain_page(_mfn(mfn));

const void *?

> +    clean_and_invalidate_dcache_va_range(v, PAGE_SIZE);
> +    unmap_domain_page(v);
> +
> +    if ( sync_icache )
> +        invalidate_icache();
>  }
>  
>  /* Write a pagetable entry. */
> --- a/xen/arch/riscv/mm.c
> +++ b/xen/arch/riscv/mm.c
> @@ -419,6 +419,7 @@ void * __init early_fdt_map(paddr_t fdt_paddr)
>  }
>  
>  vaddr_t __ro_after_init directmap_virt_start = DIRECTMAP_VIRT_START;
> +vaddr_t __ro_after_init directmap_virt_end;

If the variable is needed (see above) it pretty certainly wants an
initializer, too.

> @@ -26,6 +27,46 @@ void arch_get_xen_caps(xen_capabilities_info_t *info)
>  unsigned char __initdata cpu0_boot_stack[STACK_SIZE]
>      __aligned(STACK_SIZE);
>  
> +/**
> + * copy_from_paddr - copy data from a physical address
> + * @dst: destination virtual address
> + * @paddr: source physical address
> + * @len: length to copy
> + */
> +void __init copy_from_paddr(void *dst, paddr_t paddr, unsigned long len)

Without a declaration in a header this function ought to be static.

> +{
> +    void *src = (void *)FIXMAP_ADDR(FIX_MISC);

const void *

> +    while (len) {

Nit: Style.

> +        unsigned long l, s;
> +
> +        s = paddr & (PAGE_SIZE - 1);
> +        l = min(PAGE_SIZE - s, len);

Make these the variables' initializers?

> +        set_fixmap(FIX_MISC, maddr_to_mfn(paddr), PAGE_HYPERVISOR_RW);
> +        memcpy(dst, src + s, l);
> +        clean_dcache_va_range(dst, l);

Why is this necessary here? You're copying to plain RAM that Xen alone
is using.

> +/* Relocate the FDT in Xen heap */
> +static void * __init relocate_fdt(paddr_t dtb_paddr, size_t dtb_size)

This function having no caller will - aiui - mean build breakage at
this point of the series.

> +{
> +    void *fdt = xmalloc_bytes(dtb_size);

New code ought to be using xvmalloc() et al. Unless there's a firm
reason not to.

Jan