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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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