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

[Jan Beulich <jbeulich@xxxxxxxx>]

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<stefano.stabellini@xxxxxxxxxxxxx>
> 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