Re: [PATCH v2 3/5] xen/arm: Move make_resv_memory_node()

[Koichiro Den <den@xxxxxxxxxxxxx>]

On Fri, Jul 11, 2025 at 03:36:56PM -0400, Stewart Hildebrand wrote:
> On 7/9/25 03:58, Koichiro Den wrote:
> > On Tue, Jul 08, 2025 at 04:12:50PM -0400, Stewart Hildebrand wrote:
> >> On 7/5/25 10:27, Koichiro Den wrote:
> >>> The /reserved-memory node is inherently not specific to static-shmem.
> >>> Move it to a more generic domain build context. While at it, add an
> >>> empty kernel_info_get_shm_mem_const() for the CONFIG_STATIC_SHM=n case,
> >>> as it can now be invoked in such case.
> >>>
> >>> No functional change.
> >>>
> >>> Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> >>> ---
> >>>  xen/arch/arm/domain_build.c           | 40 +++++++++++++++++++++++++++
> >>>  xen/common/device-tree/static-shmem.c | 40 ---------------------------
> >>>  xen/include/xen/fdt-domain-build.h    |  2 ++
> >>>  xen/include/xen/static-shmem.h        | 15 ++++------
> >>>  4 files changed, 48 insertions(+), 49 deletions(-)
> >>
> >> make_resv_memory_node() was recently moved from arch/arm to common in:
> >>
> >> 72c5fa220804 ("device-tree: Move Arm's static-shmem feature to common")
> >>
> >> Is there any rationale for moving it back?
> > 
> > I overlooked that commit. So to preserve its intent, and make it usable
> > outside of the static-shmem purpose, which is my original intention, I
> > think I should place it in xen/common/device-tree/dom0less-build.c. What do
> > you think?
> 
> Hm, if only considering its current callers, yes, because STATIC_SHM
> depends on STATIC_MEMORY depends on DOM0LESS_BOOT. However, whether to
> put it in the common domain-build.c or dom0less-build.c would really
> depend on how you intend to use it, but I don't see any new calls to
> make_resv_memory_node() on the hypervisor side in the remainder of the
> series. Given that you do introduce an equivalent call on the toolstack
> side, I wonder if that's an oversight on the hypervisor side?
> 
> Lastly, in the context of the series, your intent looks to be to add the
> pv time region to /reserved-memory, but is that actually necessary?

On second thought, it's not necessary as the region is always supposed to
be recognized by the call (follows SMCCC) only, and it resides in memory
holes that Xen will never use for any other purpose. I'll simplify the
whole patch series while also including the other feedback (ie. fixed
assignment instead of searching for unused range). That means I'll drop
this commit in v3.

Thanks for the review.