Re: [PATCH v7 2/9] xen/common: Move Arm's bootfdt.c to common

[oleksii.kurochko@xxxxxxxxx]

Hi Julien,

On Mon, 2024-08-05 at 10:31 +0100, Julien Grall wrote:
> Hi Oleksii,
> 
> On 24/07/2024 16:31, Oleksii Kurochko wrote:
> > From: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > 
> > Move Arm's bootfdt.c to xen/common so that it can be used by other
> > device tree architectures like PPC and RISCV.
> > 
> > Suggested-by: Julien Grall <julien@xxxxxxx>
> > Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> > Acked-by: Julien Grall <julien@xxxxxxx>
> 
> On Matrix you asked me to review this patch again. This wasn't
> obvious 
> given you kept my ack. If you think a review is needed, then please 
> either drop the ack or explain why you keep it and ask if it is fine.
> 
> Also, I tend to list in the changes where this was acked. In this
> case, 
> you said I acked v4.
> 
> Anyway, before confirming my ack, I would like to ask some
> clarification.
> 
> > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > ---
> > Changes in V7:
> >   - Nothing changed. Only rebase.
> > ---
> > Changes in V6:
> >   - update the version of the patch to v6.
> > ---
> > Changes in V5:
> >   - add xen/include/xen/bootfdt.h to MAINTAINERS file.
> 
> I don't see any change in MAINTAINERS within this patch. Did you
> happen 
> to copy/paste all the changes made in the series?
This change should be mentioned in this patch. It is part of the
previous patch (
https://lore.kernel.org/xen-devel/102f8b60c55cdf2db5890b9fb1c2fb66e61c4a67.1721834549.git.oleksii.kurochko@xxxxxxxxx/
)

> 
> In fact the only change related to this patch doesn't seem to be
> listed.
> 
> [...]
> 
> > +#ifndef CONFIG_STATIC_SHM
> > +static inline int process_shm_node(const void *fdt, int node,
> > +                                   uint32_t address_cells,
> > uint32_t size_cells)
> > +{
> > +    printk("CONFIG_STATIC_SHM must be enabled for parsing static
> > shared"
> > +            " memory nodes\n");
> > +    return -EINVAL;
> > +}
> > +#endif
> 
> I see you duplicated the stub from arch/arm/include/static-shmem.h.
> But 
> the one in static-shmem.h will now be unreachable. I think it needs
> to 
> be removed.
Overlooked that. Originally I added that to make Xen RISC-V and PPC
build happy as early_scan_node() code uses process_shm_node():
   static int __init early_scan_node(const void *fdt,
                                     int node, const char *name, int
   depth,
                                     u32 address_cells, u32 size_cells,
                                     void *data)
   {
   ...
       else if ( depth == 2 && device_tree_node_compatible(fdt, node,
   "xen,domain") )
           rc = process_domain_node(fdt, node, name, address_cells,
   size_cells);
       else if ( depth <= 3 && device_tree_node_compatible(fdt, node,
   "xen,domain-shared-memory-v1") )
           rc = process_shm_node(fdt, node, address_cells, size_cells);
   
       if ( rc < 0 )
           printk("fdt: node `%s': parsing failed\n", name);
       return rc;
   }

Instead of introducing stub for process_shm_node() when
CONFIG_STATIC_SHM I think it would be better to add "#ifdef
CONFIG_STATIC_SHM" to early_scan_node():

   static int __init early_scan_node(const void *fdt,
                                     int node, const char *name, int
   depth,
                                     u32 address_cells, u32 size_cells,
                                     void *data)
   {
   ...
       else if ( depth == 2 && device_tree_node_compatible(fdt, node,
   "xen,domain") )
           rc = process_domain_node(fdt, node, name, address_cells,
   size_cells);
   #ifdef CONFIG_STATIC_SHM
       else if ( depth <= 3 && device_tree_node_compatible(fdt, node,
   "xen,domain-shared-memory-v1") )
           rc = process_shm_node(fdt, node, address_cells, size_cells);
   #endif
   
       if ( rc < 0 )
           printk("fdt: node `%s': parsing failed\n", name);
       return rc;
   }

> 
> Also, I think this change deserve an explanation in the commit
> message.
If the option I mentioned above looks okay then definitely I have to
mentioned that in commit message.

Thanks.

~ Oleksii

> 
> [...]
> 
> > +static void __init early_print_info(void)
> > +{
> > +    const struct membanks *mi = bootinfo_get_mem();
> > +    const struct membanks *mem_resv = bootinfo_get_reserved_mem();
> > +    struct bootmodules *mods = &bootinfo.modules;
> > +    struct bootcmdlines *cmds = &bootinfo.cmdlines;
> > +    unsigned int i;
> > +
> > +    for ( i = 0; i < mi->nr_banks; i++ )
> > +        printk("RAM: %"PRIpaddr" - %"PRIpaddr"\n",
> > +                mi->bank[i].start,
> > +                mi->bank[i].start + mi->bank[i].size - 1);
> > +    printk("\n");
> > +    for ( i = 0 ; i < mods->nr_mods; i++ )
> > +        printk("MODULE[%d]: %"PRIpaddr" - %"PRIpaddr" %-12s\n",
> > +                i,
> > +                mods->module[i].start,
> > +                mods->module[i].start + mods->module[i].size,
> > +                boot_module_kind_as_string(mods->module[i].kind));
> > +
> > +    for ( i = 0; i < mem_resv->nr_banks; i++ )
> > +    {
> > +        printk(" RESVD[%u]: %"PRIpaddr" - %"PRIpaddr"\n", i,
> > +               mem_resv->bank[i].start,
> > +               mem_resv->bank[i].start + mem_resv->bank[i].size -
> > 1);
> > +    }
> > +#ifdef CONFIG_STATIC_SHM
> > +    early_print_info_shmem();
> > +#endif
> 
> Similar remark here.
> 
> Cheers,
>