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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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 ( ;; )
>