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

[Julien Grall <julien@xxxxxxx>]

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?
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.
Also, I think this change deserve an explanation in the commit message.

[...]

> +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,

--
Julien Grall