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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

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().
```

> > @@ -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()?
>     
It is just a mistake as `directmap_virt_end` is directmap_virt_start-relative but `v` is DIRECTMAP_VIRT_START-relative.
The check should be following:
  ASSERT((va >= DIRECTMAP_VIRT_START) && (va <= DIRECTMAP_VIRT_END));
and then directmap_virt_end should be dropped at all.

> 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?

> > +        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.

> > +/* 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.
>     
Yes, it should be a problem, missed that. Then I have to merge it with the next one patch.
Thanks.

~ Oleksii