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
