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

From: Julien Grall <julien@xen.org>
To: Shawn Anastasio <sanastasio@raptorengineering.com>,
	xen-devel@lists.xenproject.org
Cc: tpearson@raptorengineering.com, Jan Beulich <jbeulich@suse.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	George Dunlap <george.dunlap@citrix.com>,
	Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>,
	Bertrand Marquis <bertrand.marquis@arm.com>,
	Michal Orzel <michal.orzel@amd.com>,
	Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
Subject: Re: [PATCH v3 6/9] xen/common: Move Arm's bootfdt.c to common
Date: Wed, 17 Apr 2024 18:24:14 +0100	[thread overview]
Message-ID: <38b06b07-179d-4483-b79b-aea8c68f3c34@xen.org> (raw)
In-Reply-To: <c7de32e6-67d5-4bb8-9cc8-68753c657fc7@raptorengineering.com>

Hi Shawn,

On 12/04/2024 03:53, Shawn Anastasio wrote:
> Hi Julien,
> 
> On 3/21/24 12:50 PM, Julien Grall wrote:
>> Hi Shawn,
>>
>> On 14/03/2024 22:15, Shawn Anastasio wrote:
>>> 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@xen.org>
>>> Signed-off-by: Shawn Anastasio <sanastasio@raptorengineering.com>
>>> Acked-by: Julien Grall <julien@xen.org>
>>> ---
>>> Changes in v2:
>>>     - Drop #if defined(CONFIG_ARM_EFI) now that efi_enabled is stubbed
>>>
>>>    MAINTAINERS                                    | 1 +
>>>    xen/arch/arm/Makefile                          | 1 -
>>>    xen/common/device-tree/Makefile                | 1 +
>>>    xen/{arch/arm => common/device-tree}/bootfdt.c | 0
>>>    4 files changed, 2 insertions(+), 1 deletion(-)
>>>    rename xen/{arch/arm => common/device-tree}/bootfdt.c (100%)
>>>
>>> diff --git a/MAINTAINERS b/MAINTAINERS
>>> index e85fbe6737..20fdec9ffa 100644
>>> --- a/MAINTAINERS
>>> +++ b/MAINTAINERS
>>> @@ -251,6 +251,7 @@ S:    Supported
>>>    L:    xen-devel@lists.xenproject.org
>>>    F:    docs/misc/arm/
>>>    F:    xen/arch/arm/
>>> +F:    xen/common/device-tree/bootfdt.c
>>>    F:    xen/drivers/char/arm-uart.c
>>>    F:    xen/drivers/char/cadence-uart.c
>>>    F:    xen/drivers/char/exynos4210-uart.c
>>> diff --git a/xen/arch/arm/Makefile b/xen/arch/arm/Makefile
>>> index 7b1350e2ef..9e1548378c 100644
>>> --- a/xen/arch/arm/Makefile
>>> +++ b/xen/arch/arm/Makefile
>>> @@ -10,7 +10,6 @@ obj-$(CONFIG_TEE) += tee/
>>>    obj-$(CONFIG_HAS_VPCI) += vpci.o
>>>
>>>    obj-$(CONFIG_HAS_ALTERNATIVE) += alternative.o
>>> -obj-y += bootfdt.init.o
>>>    obj-y += cpuerrata.o
>>>    obj-y += cpufeature.o
>>>    obj-y += decode.o
>>> diff --git a/xen/common/device-tree/Makefile
>>> b/xen/common/device-tree/Makefile
>>> index c97b2bd88c..fa5beafd65 100644
>>> --- a/xen/common/device-tree/Makefile
>>> +++ b/xen/common/device-tree/Makefile
>>> @@ -1 +1,2 @@
>>> +obj-y += bootfdt.init.o
>>>    obj-y += bootinfo.o
>>
>> Looking at the names, it is not entirely clear what would be the
>> differences between bootfdt and bootinfo. Should they just be one file?
>>
> 
> With the current split I've chosen, all functions pertaining to managing
> the `struct bootinfo` data structure are contained within bootinfo.c and
> all functions responsible for parsing the FDT on boot are in bootfdt.c.
> 
> This separation exists currently in the ARM tree, but the bootinfo
> functions are contained in setup.c rather than a separate self-contained
> bootinfo.c.
> 
> If you feel strongly that we would be better off with everything in a
> single file, I'm not necessarily opposed to that, but I do think that
> this split at least makes sense.

I am fine with the split. It wasn't originally clear how this was done 
because the first comment in bootinfo contains:

  Early device tree parsing and bookkeeping routines.

But from what you wrote, it seems this is only meant to cover the latter 
part. Did I understand it correctly?

Cheers,

-- 
Julien Grall