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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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