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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.12.2024 11:26, Oleksii Kurochko wrote:
> 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?

Of course. I'm not even sure "size" can be reasonably qualified as "exclusive" 
or
"inclusive".

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

Right.

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

Correct. My point merely was that there are two reasons this isn't needed, each
of which is by itself sufficient to justify omitting that call.

Jan