On 10.12.2024 16:20, Oleksii Kurochko wrote:
> On 12/9/24 4:00 PM, Jan Beulich wrote:
>> 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.
>
> Actually I faced the usage of virt_to_page() in xmalloc_whole_page():
> ```
> static void *xmalloc_whole_pages(unsigned long size, unsigned long align)
> {
> ...
> PFN_ORDER(virt_to_page(res)) = PFN_UP(size);
> /* Check that there was no truncation: */
> ASSERT(PFN_ORDER(virt_to_page(res)) == PFN_UP(size));
>
> return res;
> }
> ```
> which is called from xmalloc().
>
> Do we need a second paragraph of the commit message at all? Or it is just
> obvious if
> flush_page_to_ram(), virt_to_page() and copy_from_paddr() are introduces that
> they are needed for
> relocate_fdt()?
>
> Or perhaps rephrasing in the following way would be enough?
> ```
> For internal use of|xmalloc|, the functions|flush_page_to_ram()|
> and|virt_to_page()| are introduced.
> |virt_to_page()| is also required for|free_xenheap_pages()|. These additions
> are used to support
> |xmalloc|, which is utilized within|relocate_fdt()|.
> Additionally,|copy_from_paddr()| is introduced
> for use in|relocate_fdt()|.
> ```
I think that would do.
>> 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?
>
> Yes, I remember that and at the moment everything ( DIRECTMAP_VIRT_END,
> FRAMETABLE_VIRT_END )
> is following "inclusive" way. Considering that you remind me that could you
> please tell me more time
> what I am missing?
First the table azt the top of config.h uses all exclusive upper bounds.
And then DIRECTMAP_SIZE's definition assumes DIRECTMAP_SLOT_END would be
exclusive, when it's inclusive.
>>> + 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.
>
> It is Arm specific:
> ```
> commit c60209d77e2c02de110ca0fdaa2582ef4e53d8fd
> Author: Stefano Stabellini<[email protected]>
> Date: Mon Jan 21 12:40:31 2013 +0000
>
> xen/arm: flush dcache after memcpy'ing the kernel image
>
> After memcpy'ing the kernel in guest memory we need to flush the dcache
> to make sure that the data actually reaches the memory before we start
> executing guest code with caches disabled.
>
> copy_from_paddr is the function that does the copy, so add a
> flush_xen_dcache_va_range there.
> ```
> I wanted to put copy_from_paddr() to some common place at the end but in
> RISC-V cache is always enabled
> ( I don't see an instruction in the spec for disable/enable cache ) so this
> issue isn't present for RISC-V
> and clean_dcache_va_range() should/could be dropped.
That plus there's no kernel in sight just yet.
Jan
