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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Michal,

On Mon, 2024-07-15 at 12:15 +0200, Michal Orzel wrote:
> 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.
I will update that in the next patch version.

> 
> > +#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?
Agree, good point.

I should drop that definition here and make it arch specific. I have to
update my patch series where I am introducing BOOTFDT_MAX_SIZE ( or
something similar ) in riscv/config.h.

Thanks.

~ Oleksii