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

[Jan Beulich <jbeulich@xxxxxxxx>]

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