Re: [PATCH v6 1/8] xen/device-tree: Move Arm's setup.c bootinfo functions to common

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Oleksii,

In general, the patch looks ok (apart from Jan comments). Just a couple of 
remarks.

On 12/07/2024 18:22, Oleksii Kurochko wrote:
> 
> 
> From: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> 
> Arm's setup.c contains a collection of functions for parsing memory map
> and other boot information from a device tree. Since these routines are
> generally useful on any architecture that supports device tree booting,
> move them into xen/common/device-tree.
> 
> Suggested-by: Julien Grall <julien@xxxxxxx>
> Signed-off-by: Shawn Anastasio <sanastasio@xxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> ---
> Changes in V6:
>  - update the version of the patch to v6, to show that it is based
>    on the work done by Shawn in the patch v4.
> ---
> Changes in V5:
>  - add xen/include/xen/bootfdt.h to MAINTAINERS file.
>  - drop message "Early device tree parsing and".
>  - After rebase on top of the current staging the following changes were done:
>    - init bootinfo variable in <common/device-tree/bootinfo.c> with 
> BOOTINFO_INIT;
>    - update the code of dt_unreserved_regions():
>        CONFIG_STATIC_SHM related changes and getting of reserved_mem
>        bootinfo_get_shmem() ??
>    - update the code of meminfo_overlap_check():
>        add check ( INVALID_PADDR == bank_start ) to if case.
>    - update the code of check_reserved_regions_overlap():
>        CONFIG_STATIC_SHM related changes.
>    - struct bootinfo was updated ( CONFIG_STATIC_SHM changes )
>    - add shared_meminfo ( because of CONFIG_STATIC_SHM )
>    - struct struct membanks was update with __struct group so <xen/kernel> is
>      neeeded to be included in bootfdt.h
>    - move BOOTINFO_ACPI_INIT, BOOTINFO_SHMEM_INIT, BOOTINFO_INIT to generic 
> bootfdt.h
>    - bootinfo_get_reserved_mem(), bootinfo_get_mem(), bootinfo_get_acpi(),
>      bootinfo_get_shmem() and bootinfo_get_shmem_extra() were moved to 
> xen/bootfdt.h
>  - s/arm32/CONFIG_SEPARATE_XENHEAP/
>  - add inclusion of <xen/macros.h> because there are function in 
> <xen/bootfdt.h> which
>    are using container_of().
>  ---
> Changes in v4:
>   - create new xen/include/bootinfo.h rather than relying on arch's
>     asm/setup.h to provide required definitions for bootinfo.c
>   - build bootinfo.c as .init.o
>   - clean up and sort bootinfo.c's #includes
>   - use CONFIG_SEPARATE_XENHEAP rather than CONFIG_ARM_32 to guard
>     xenheap-specific behavior of populate_boot_allocator
>   - (MAINTAINERS) include all of common/device-tree rather than just
>     bootinfo.c
> ---
>  MAINTAINERS                       |   2 +
>  xen/arch/arm/include/asm/setup.h  | 187 +-----------
>  xen/arch/arm/setup.c              | 432 ----------------------------
>  xen/common/Makefile               |   1 +
>  xen/common/device-tree/Makefile   |   1 +
>  xen/common/device-tree/bootinfo.c | 459 ++++++++++++++++++++++++++++++
>  xen/include/xen/bootfdt.h         | 196 +++++++++++++
>  7 files changed, 660 insertions(+), 618 deletions(-)
>  create mode 100644 xen/common/device-tree/Makefile
>  create mode 100644 xen/common/device-tree/bootinfo.c
>  create mode 100644 xen/include/xen/bootfdt.h
> 

[...]

> diff --git a/xen/include/xen/bootfdt.h b/xen/include/xen/bootfdt.h
> new file mode 100644
> index 0000000000..7cd45b3d4b
> --- /dev/null
> +++ b/xen/include/xen/bootfdt.h
> @@ -0,0 +1,196 @@
> +#ifndef __XEN_BOOTFDT_H__
AFAIR, to avoid violating MISRA rule 21.1, we should avoid introducing new 
macros with double underscore.

> +#define __XEN_BOOTFDT_H__
> +
> +#include <xen/types.h>
> +#include <xen/kernel.h>
> +#include <xen/macros.h>
> +
> +#define MIN_FDT_ALIGN 8
> +#define MAX_FDT_SIZE SZ_2M
2M blob limit is Arm64 specific. What will be the limit on RISCV? Shouldn't it 
be moved to some arch specific file?

~Michal