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