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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 11.07.2024 11:00, Oleksii wrote:
> On Wed, 2024-07-10 at 12:23 +0200, Jan Beulich wrote:
>> On 03.07.2024 12:42, 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 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().
>>
>> Just to mention it: This is confusing. The series is tagged "v1". I
>> understand
>> you took Shawn's work, which had already undergone revisions. But
>> then imo you
>> want to at least clarify how your v1 relates to his v4 or v5, i.e.
>> then making
>> clear to the reader whether all of the changes above were actually
>> done by you
>> on top of an earlier v4, or whether you took the earlier v5 verbatim.
> That is why I wrote "Changes in v5" to show that these changes were
> done on top of v4 version of Shawn's series, so what is mentioned in v5
> here it is what was done by me.

Except that what you sent is v1, not v5.

> I'll reword that and probably I shouldn't drop "Changes in v4,3,2,1"
> from Shawn's patch.

I don't think you should drop anything. You want to clarify relationship
of the version of your series with that of Shawn's earlier one. Or you'd
want to continue numbering from what the previous series had, yet that
may then also end up confusing if Shawn resumed work there.

Jan