| Message ID | 7e492df051c949744755a221c0448c740d2c681b.1720002425.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RISCV device tree mapping | expand |
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. > @@ -33,15 +35,34 @@ static void test_macros_from_bug_h(void) > printk("WARN is most likely working\n"); > } > > +void __init fdt_map(paddr_t dtb_addr) > +{ > + device_tree_flattened = early_fdt_map(dtb_addr); > + if ( !device_tree_flattened ) > + { > + printk("wrong FDT\n"); > + die(); > + } > +} > + > void __init noreturn start_xen(unsigned long bootcpu_id, > paddr_t dtb_addr) > { > + size_t fdt_size; > + const char *cmdline; > + > remove_identity_mapping(); > > trap_init(); > > test_macros_from_bug_h(); > > + fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr); You don't use the return value anywhere below. What use is the local var then? Jan > + cmdline = boot_fdt_cmdline(device_tree_flattened); > + printk("Command line: %s\n", cmdline); > + cmdline_parse(cmdline); > + > printk("All set up\n"); > > for ( ;; )
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. > > > @@ -33,15 +35,34 @@ static void test_macros_from_bug_h(void) > > printk("WARN is most likely working\n"); > > } > > > > +void __init fdt_map(paddr_t dtb_addr) > > +{ > > + device_tree_flattened = early_fdt_map(dtb_addr); > > + if ( !device_tree_flattened ) > > + { > > + printk("wrong FDT\n"); > > + die(); > > + } > > +} > > + > > void __init noreturn start_xen(unsigned long bootcpu_id, > > paddr_t dtb_addr) > > { > > + size_t fdt_size; > > + const char *cmdline; > > + > > remove_identity_mapping(); > > > > trap_init(); > > > > test_macros_from_bug_h(); > > > > + fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr); > > You don't use the return value anywhere below. What use is the local > var > then? I returned just for debug ( to see what is the fdt size ), it can be dropped now. ~ Oleksii > > Jan > > > + cmdline = boot_fdt_cmdline(device_tree_flattened); > > + printk("Command line: %s\n", cmdline); > > + cmdline_parse(cmdline); > > + > > printk("All set up\n"); > > > > for ( ;; ) >
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 ... Jan
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? ~ Oleksii
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()? Surely down the road there are going to be more thing that need mapping relatively early, but after the MMU was enabled. Jan
Hi, On 11/07/2024 10: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. Somewhat related to what Jan is asking. You only seem to part the FDT in start_xen(). So why do you need to call fdt_map() that early? Cheers,
Add Julien as he asked basically the same question in another thread. 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? > Surely down the > road there are going to be more thing that need mapping relatively > early, > but after the MMU was enabled. For sure, but for mapping other things it would be introduced setup_mm() and necessary functions to implementinitialization and handling of mm. ~ Oleksii
On Thu, 2024-07-11 at 11:50 +0100, Julien Grall wrote: > Hi, Hi Julien, > > On 11/07/2024 10: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. > > Somewhat related to what Jan is asking. You only seem to part the FDT > in > start_xen(). So why do you need to call fdt_map() that early? Let's continue our discussion in another thread ( I added you there ). But the answer on your question is here: https://lore.kernel.org/xen-devel/cover.1720002425.git.oleksii.kurochko@gmail.com/T/#m2890200a53c6ea2101e0f9e9ea77f589bd187e26 And here:https://lore.kernel.org/xen-devel/cover.1720002425.git.oleksii.kurochko@gmail.com/T/#m4e20792121b97465db7081cc4c1e27bdb15cdd51 Let me know if the link above answers your question. ~ Oleksii
Hi, On 11/07/2024 12:28, Oleksii wrote: > Add Julien as he asked basically the same question in another thread. Thanks! > > 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: >>> 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. I am not very familiar with the code base for RISC-V, but looking at the context in the patch, it seems you will still have the identity mapping mapped until start_xen(). I assume we don't exactly know where the loader will put Xen in memory. Which means that the region may clash with your defined runtime regions (such as the FDT). Did I misunderstand anything? That's one of the reason on Arm we are trying to enable the MMU very early. The only thing we setup is a mapping for Xen (and earlyprintk) all the rest will be mapped once the MMU is on. With that, the only thing you need to take care off the runtime Xen address overlapping with the identity mapping. Cheers,
On Thu, 2024-07-11 at 12:54 +0100, Julien Grall wrote: > Hi, > > On 11/07/2024 12:28, Oleksii wrote: > > Add Julien as he asked basically the same question in another > > thread. > > Thanks! > > > > > 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: > > > > 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. > > I am not very familiar with the code base for RISC-V, but looking at > the > context in the patch, it seems you will still have the identity > mapping > mapped until start_xen(). We have identity mapping only for a small piece of .text section: . = ALIGN(IDENT_AREA_SIZE); _ident_start = .; *(.text.ident) _ident_end = .; All other will be identically mapped only in case of linker address is equal to load address. > > I assume we don't exactly know where the loader will put Xen in > memory. > Which means that the region may clash with your defined runtime > regions > (such as the FDT). Did I misunderstand anything? I am not really get what is the issue here. If we are speaking about physical regions then loader will guarantee that Xen and FDT regions don't overlap. If we are speaking about virtual regions then Xen will check that nothing is overlaped. And the virtual regions are mapped so high so I am not sure that loader will put something there. ( FDT in Xen is mapped to 0xffffffffc0200000 ). Could you please clarify what I am missing? > > That's one of the reason on Arm we are trying to enable the MMU very > early. The only thing we setup is a mapping for Xen (and earlyprintk) > all the rest will be mapped once the MMU is on. It makes sense. Then I have to introduce map_pages_to_xen() first and then early_fdt_map(). ~ Oleksii > > With that, the only thing you need to take care off the runtime Xen > address overlapping with the identity mapping.
Hi Oleksii, On 11/07/2024 13:29, Oleksii wrote: > On Thu, 2024-07-11 at 12:54 +0100, Julien Grall wrote: >> Hi, >> >> On 11/07/2024 12:28, Oleksii wrote: >>> Add Julien as he asked basically the same question in another >>> thread. >> >> Thanks! >> >>> >>> 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: >>>>> 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. >> >> I am not very familiar with the code base for RISC-V, but looking at >> the >> context in the patch, it seems you will still have the identity >> mapping >> mapped until start_xen(). > We have identity mapping only for a small piece of .text section: > . = ALIGN(IDENT_AREA_SIZE); > _ident_start = .; > *(.text.ident) > _ident_end = .; > > All other will be identically mapped only in case of linker address is > equal to load address. > >> >> I assume we don't exactly know where the loader will put Xen in >> memory. >> Which means that the region may clash with your defined runtime >> regions >> (such as the FDT). Did I misunderstand anything? > I am not really get what is the issue here. > > If we are speaking about physical regions then loader will guarantee > that Xen and FDT regions don't overlap. Sure. But I was referring to... > > If we are speaking about virtual regions then Xen will check that > nothing is overlaped. ... this part. The more regions you mapped with MMU off, the more work you have to do to ensure nothing will clash. > And the virtual regions are mapped so high so I > am not sure that loader will put something there. ( FDT in Xen is > mapped to 0xffffffffc0200000 ) Never say never :). On Arm, some 64-bit HW (such as ADLink AVA platform) has the RAM starting very high and load Xen around 8TB. For Arm, we still decided to put a limit (10TB) where Xen can be loaded but this is mainly done for convenience (otherwise it is a bit more complicated to get off the identity mapping). We still have a check in place to ensure that Xen is not loaded above 10TB. If you map the FDT within the same L1. Cheers,
On Thu, 2024-07-11 at 13:44 +0100, Julien Grall wrote: > Hi Oleksii, Hi Julien, > > On 11/07/2024 13:29, Oleksii wrote: > > On Thu, 2024-07-11 at 12:54 +0100, Julien Grall wrote: > > > Hi, > > > > > > On 11/07/2024 12:28, Oleksii wrote: > > > > Add Julien as he asked basically the same question in another > > > > thread. > > > > > > Thanks! > > > > > > > > > > > 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: > > > > > > 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. > > > > > > I am not very familiar with the code base for RISC-V, but looking > > > at > > > the > > > context in the patch, it seems you will still have the identity > > > mapping > > > mapped until start_xen(). > > We have identity mapping only for a small piece of .text section: > > . = ALIGN(IDENT_AREA_SIZE); > > _ident_start = .; > > *(.text.ident) > > _ident_end = .; > > > > All other will be identically mapped only in case of linker address > > is > > equal to load address. > > > > > > > > I assume we don't exactly know where the loader will put Xen in > > > memory. > > > Which means that the region may clash with your defined runtime > > > regions > > > (such as the FDT). Did I misunderstand anything? > > I am not really get what is the issue here. > > > > If we are speaking about physical regions then loader will > > guarantee > > that Xen and FDT regions don't overlap. > > Sure. But I was referring to... > > > > > If we are speaking about virtual regions then Xen will check that > > nothing is overlaped. > > ... this part. The more regions you mapped with MMU off, the more > work > you have to do to ensure nothing will clash. Yes, agree here. Then I have to look at what I need now to introduce map_pages_to_xen(). Thanks for clarifying. > > > And the virtual regions are mapped so high so I > > am not sure that loader will put something there. ( FDT in Xen is > > mapped to 0xffffffffc0200000 ) > Never say never :). On Arm, some 64-bit HW (such as ADLink AVA > platform) > has the RAM starting very high and load Xen around 8TB. For Arm, we > still decided to put a limit (10TB) where Xen can be loaded but this > is > mainly done for convenience (otherwise it is a bit more complicated > to > get off the identity mapping). Oh, it is very high. I couldn't even expect. ~ Oleksii > > We still have a check in place to ensure that Xen is not loaded above > 10TB. If you map the FDT within the same L1. > > Cheers, >
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
diff --git a/xen/arch/riscv/riscv64/head.S b/xen/arch/riscv/riscv64/head.S index 3261e9fce8..22fb36a861 100644 --- 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 diff --git a/xen/arch/riscv/setup.c b/xen/arch/riscv/setup.c index 4f06203b46..b346956e06 100644 --- a/xen/arch/riscv/setup.c +++ b/xen/arch/riscv/setup.c @@ -1,7 +1,9 @@ /* SPDX-License-Identifier: GPL-2.0-only */ +#include <xen/bootfdt.h> #include <xen/bug.h> #include <xen/compile.h> +#include <xen/device_tree.h> #include <xen/init.h> #include <xen/mm.h> @@ -33,15 +35,34 @@ static void test_macros_from_bug_h(void) printk("WARN is most likely working\n"); } +void __init fdt_map(paddr_t dtb_addr) +{ + device_tree_flattened = early_fdt_map(dtb_addr); + if ( !device_tree_flattened ) + { + printk("wrong FDT\n"); + die(); + } +} + void __init noreturn start_xen(unsigned long bootcpu_id, paddr_t dtb_addr) { + size_t fdt_size; + const char *cmdline; + remove_identity_mapping(); trap_init(); test_macros_from_bug_h(); + fdt_size = boot_fdt_info(device_tree_flattened, dtb_addr); + + cmdline = boot_fdt_cmdline(device_tree_flattened); + printk("Command line: %s\n", cmdline); + cmdline_parse(cmdline); + printk("All set up\n"); for ( ;; )
Except mapping of FDT, it is also printing command line passed by a DTB and initialize bootinfo from a DTB. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- xen/arch/riscv/riscv64/head.S | 3 +++ xen/arch/riscv/setup.c | 21 +++++++++++++++++++++ 2 files changed, 24 insertions(+)