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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 12/10/24 5:20 PM, Jan Beulich wrote:

> > > 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.
>     
Really missed to update the tale on the top of config.h.

But it seems to me like any *_SIZE will be defined in exclusive way by its nature, doesn't it?
For example, size of directmap is (509-200+1)<<30 = 0x7F80000000 and it is not really (
0x7F80000000 - 1 ) = 7F7FFFFFFF.

I prefer to have DIRECTMAP_{SIZE,VIRT_END} defined as now:
  #define DIRECTMAP_SIZE          (SLOTN(DIRECTMAP_SLOT_END + 1) - SLOTN(DIRECTMAP_SLOT_START))
  #define DIRECTMAP_VIRT_END      (DIRECTMAP_VIRT_START + DIRECTMAP_SIZE - 1)
( of course with making upper bounds inclusive in the table on the top of config.h )

> > > > +        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.
>     
( clarification ) will it change something if kernel will be loaded now? It seems even if we are copying kernel in guest
memory we still don't need to flush the dcache as cache is enabled and cache coherence protocol will do a work automatically.

~ Oleksii