Re: [PATCH v2] xen/arm: Skip memory nodes if not enabled

[Michal Orzel <michal.orzel@xxxxxxx>]

Hi Leo,

On 28/09/2023 16:31, Leo Yan wrote:
> 
> 
> Currently, the Xen hypervisor doesn't handle the status, the issue can
> be described from two perspectives: the memory nodes and the reserved
> memory nodes.
> 
> - If a memory node has a status "disabled" it implies that it should
>   not be used. Xen does not handle the status property for the memory
>   node and ends up using it.
> 
> - If a reserved memory node has a status "disabled", it means that this
>   region is no longer reserved and can be used, but the "disabled"
>   status is not handled by Xen.
> 
>   Xen passes the intact device tree binding of the reserved memory nodes
>   to Dom0 and creates a memory node to cover reserved regions. Disabled
>   reserved memory nodes are ignored by the Dom0 Linux kernel, thus the
>   Dom0 Linux kernel will continue to allocate pages from such a region.
> 
>   On the other hand, since the disabled status is not handled by Xen,
>   the disabled reserved memory regions are excluded from the page
>   management in Xen which results in Xen being unable to obtain the
>   corresponding MFN, in the end, Xen reports error like:
> 
>   (XEN) arch/arm/p2m.c:2202: d0v0: Failing to acquire the MFN 0x1a02dc
> 
> This patch introduces a function device_tree_node_is_available(). If it
> detects a memory node is not enabled, Xen will not add the memory region
> into the memory lists. In the end, this avoids to generate the memory
> node for the disabled memory regions sent to the kernel and the kernel
> cannot use the disabled memory nodes any longer.
> 
> Since this patch adds checking device node's status in the
> device_tree_get_meminfo() function, except it checks for memory nodes
> and reserved memory nodes, it also supports status for static memory
> and static heap.
> 
> Suggested-by: Michal Orzel <michal.orzel@xxxxxxx>
> Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> ---
> 
> Changes from v1:
> - Renamed function to device_tree_node_is_available() (Michal Orzel);
> - Polished coding style (Michal Orzel);
> - Refined commit log (Michal Orzel, Julien Grall).
> 
>  xen/arch/arm/bootfdt.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c
> index 2673ad17a1..1b80d2d622 100644
> --- a/xen/arch/arm/bootfdt.c
> +++ b/xen/arch/arm/bootfdt.c
> @@ -16,6 +16,19 @@
>  #include <xsm/xsm.h>
>  #include <asm/setup.h>
> 
> +static bool __init device_tree_node_is_available(const void *fdt, int node)
> +{
> +    const char *status = fdt_getprop(fdt, node, "status", NULL);
Please see Julien's comment for v1. To save some jumps,instructions
we should also check for length of the property to be > 0, just like we do in 
dt_device_is_available().

Apart from that the patch looks good.

~Michal