Re: [PATCH v1 5/5] xen/riscv: map FDT

[Jan Beulich <jbeulich@xxxxxxxx>]

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