On 11.07.2024 13:28, Oleksii wrote:
> On Thu, 2024-07-11 at 12:50 +0200, Jan Beulich wrote:
>> On 11.07.2024 12:26, Oleksii wrote:
>>> On Thu, 2024-07-11 at 11:54 +0200, Jan Beulich wrote:
>>>> On 11.07.2024 11:40, Oleksii wrote:
>>>>> On Wed, 2024-07-10 at 14:38 +0200, Jan Beulich wrote:
>>>>>> On 03.07.2024 12:42, Oleksii Kurochko wrote:
>>>>>>> Except mapping of FDT, it is also printing command line
>>>>>>> passed
>>>>>>> by
>>>>>>> a DTB and initialize bootinfo from a DTB.
>>>>>>
>>>>>> I'm glad the description isn't empty here. However, ...
>>>>>>
>>>>>>> --- a/xen/arch/riscv/riscv64/head.S
>>>>>>> +++ b/xen/arch/riscv/riscv64/head.S
>>>>>>> @@ -41,6 +41,9 @@ FUNC(start)
>>>>>>>
>>>>>>> jal setup_initial_pagetables
>>>>>>>
>>>>>>> + mv a0, s1
>>>>>>> + jal fdt_map
>>>>>>> +
>>>>>>> /* Calculate proper VA after jump from 1:1 mapping
>>>>>>> */
>>>>>>> la a0, .L_primary_switched
>>>>>>> sub a0, a0, s2
>>>>>>
>>>>>> ... it could do with clarifying why this needs calling from
>>>>>> assembly
>>>>>> code. Mapping the FDT clearly looks like something that wants
>>>>>> doing
>>>>>> from start_xen(), i.e. from C code.
>>>>> fdt_map() expected to work while MMU is off as it is using
>>>>> setup_initial_mapping() which is working with physical address.
>>>>
>>>> Hmm, interesting. When the MMU is off, what does "map" mean? Yet
>>>> then
>>>> it feels I'm misunderstanding what you're meaning to tell me ...
>>> Let's look at examples of the code:
>>> 1. The first thing issue will be here:
>>> void* __init early_fdt_map(paddr_t fdt_paddr)
>>> {
>>> unsigned long dt_phys_base = fdt_paddr;
>>> unsigned long dt_virt_base;
>>> unsigned long dt_virt_size;
>>>
>>> BUILD_BUG_ON(MIN_FDT_ALIGN < 8);
>>> if ( !fdt_paddr || fdt_paddr % MIN_FDT_ALIGN || fdt_paddr %
>>> SZ_2M
>>> ||
>>> fdt_totalsize(fdt_paddr) > BOOT_FDT_VIRT_SIZE )
>>> MMU doesn't now about virtual address of fdt_paddr as fdt_paddr
>>> wasn't
>>> mapped.
>>>
>>> 2. In setup_initial_mapping() we have HANDLE_PGTBL() where pgtbl is
>>> a
>>> pointer to physical address ( which also should be mapped in MMU
>>> if we
>>> want to access it after MMU is enabled ):
>>> #define
>>> HANDLE_PGTBL(curr_lvl_num)
>>> \
>>> index = pt_index(curr_lvl_num,
>>> page_addr);
>>> \
>>> if ( pte_is_valid(pgtbl[index])
>>> )
>>> \
>>>
>>> {
>>> \
>>> /* Find L{ 0-3 } table
>>> */
>>> \
>>> pgtbl = (pte_t
>>> *)pte_to_paddr(pgtbl[index]);
>>> \
>>>
>>> }
>>> \
>>>
>>> else
>>> \
>>>
>>> {
>>> \
>>> /* Allocate new L{0-3} page table
>>> */
>>> \
>>> if ( mmu_desc->pgtbl_count == PGTBL_INITIAL_COUNT
>>> )
>>> \
>>>
>>> {
>>> \
>>> early_printk("(XEN) No initial table
>>> available\n");
>>> \
>>> /* panic(), BUG() or ASSERT() aren't ready now.
>>> */
>>> \
>>>
>>> die();
>>> \
>>>
>>> }
>>> \
>>> mmu_desc-
>>>> pgtbl_count++;
>>> \
>>> pgtbl[index] = paddr_to_pte((unsigned long)mmu_desc-
>>> >next_pgtbl, \
>>>
>>> PTE_VALID);
>>> \
>>> pgtbl = mmu_desc-
>>>> next_pgtbl;
>>> \
>>> mmu_desc->next_pgtbl +=
>>> PAGETABLE_ENTRIES;
>>> \
>>> }
>>>
>>> So we can't use setup_initial_mapping() when MMU is enabled as
>>> there is
>>> a part of the code which uses physical address which are not
>>> mapped.
>>>
>>> We have only mapping for for liner_start <-> load_start and the
>>> small
>>> piece of code in text section ( _ident_start ):
>>> setup_initial_mapping(&mmu_desc,
>>> linker_start,
>>> linker_end,
>>> load_start);
>>>
>>> if ( linker_start == load_start )
>>> return;
>>>
>>> ident_start = (unsigned long)turn_on_mmu
>>> &XEN_PT_LEVEL_MAP_MASK(0);
>>> ident_end = ident_start + PAGE_SIZE;
>>>
>>> setup_initial_mapping(&mmu_desc,
>>> ident_start,
>>> ident_end,
>>> ident_start);
>>>
>>> We can use setup_initial_mapping() when MMU is enabled only in case
>>> when linker_start is equal to load_start.
>>>
>>> As an option we can consider for as a candidate for identaty
>>> mapping
>>> also section .bss.page_aligned where root and nonroot page tables
>>> are
>>> located.
>>>
>>> Does it make sense now?
>>
>> I think so, yet at the same time it only changes the question: Why is
>> it
>> that you absolutely need to use setup_initial_mapping()?
> There is no strict requirement to use setup_initial_mapping(). That
> function is available to me at the moment, and I haven't found a better
> option other than reusing what I currently have.
>
> If not to use setup_initial_mapping() then it is needed to introduce
> xen_{un}map_table(), create_xen_table(), xen_pt_next_level(),
> xen_pt_update{_entry}(), map_pages_to_xen(). What I did a little bit
> later here:
> https://gitlab.com/xen-project/people/olkur/xen/-/commit/a4619d0902e0a012ac2f709d31621a8d499bca97
> Am I confusing something?
>
> Could you please recommend me how to better?
I think you've sorted this for yourself already, judging from subsequent
communication on this thread, where you indicate you'll introduce
map_pages_to_xen() first. That's the way I'd have expected you to go.
Jan
