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

[Leo Yan <leo.yan@xxxxxxxxxx>]

Hi Michal,

On Tue, Sep 26, 2023 at 10:36:04AM +0200, Michal Orzel wrote:

[...]

> > Essentially, this issue is caused by the Xen hypervisor which misses to
> > handle the status for the memory nodes (for both the normal memory nodes
> > and the reserved memory nodes) and transfers them to the Linux kernel.
> > 
> > This patch introduces a function memory_node_is_available(). If it
> > detects a memory node is not enabled, the hypervisor will not add the
> > memory region into the memory lists. In the end, this avoids to generate
> > the memory nodes from the memory lists sent to the kernel and the kernel
> > cannot use the disabled memory nodes any longer.
> 
> Interesting. So FWICS, we have 2 issues that have a common ground:
>
> 1) If the reserved memory node has a status "disabled", it implies that this 
> region
> is no longer reserved and can be used which is not handled today by Xen and 
> leads
> to the above mentioned problem.

Correct.

> 2) If the memory node has a status "disabled" it implies that it should not 
> be used
> which is not the case in current Xen. This means that at the moment, Xen 
> would try
> to use such a memory region which is incorrect.

Exactly.

> I think the commit msg should be more generic and focus on these two issues.
> Summary:
> Xen does not handle the status property of memory nodes and ends up using 
> them.
> Xen does not handle the status property of reserved memory nodes. If status 
> is disabled
> it means the reserved region shall no longer be treated as reserved. Xen 
> passes the reserved
> memory nodes untouched to dom0 fdt and creates a memory node to cover 
> reserved regions.
> Disabled reserved memory nodes are ignored by the guest which leaves us with 
> the exposed
> memory nodes. Guest can access such a region but it is excluded from the page 
> management in Xen
> which results in Xen being unable to obtain the corresponding MFN.

Yes, thanks a lot for good summary!

In theory, a good practice should use two separate patches to fix two
issues respectively. Given we can use simple change to fix these two
issues in one patch, I will amend the patch's commit log with your
summary for better recording these issues.

> > Signed-off-by: Leo Yan <leo.yan@xxxxxxxxxx>
> > ---
> >  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..b46dde06a9 100644
> > --- a/xen/arch/arm/bootfdt.c
> > +++ b/xen/arch/arm/bootfdt.c
> > @@ -206,11 +206,27 @@ int __init device_tree_for_each_node(const void *fdt, 
> > int node,
> >      return 0;
> >  }
> > 
> > +static bool __init memory_node_is_available(const void *fdt, unsigned long 
> > node)
>
> This function is not strictly related to memory node so it would be better to 
> call it e.g. device_tree_node_is_available.
> This way it can be used in the future for other nodes if needed. Also, I 
> would move it somewhere near the top of the file
> next to other helpers.

Will do.

> Also, node should be of type 'int'

Will fix.

> > +{
> > +    const char *status = fdt_getprop(fdt, node, "status", NULL);
> > +
> > +    if (!status)
> white spaces between brackets and condition
> if ( !status )

Will fix.

> > +        return true;
> > +
> > +    if (!strcmp(status, "ok") || !strcmp(status, "okay"))
> white spaces between brackets and condition
> if ( !strcmp(status, "ok") || !strcmp(status, "okay") )

Will fix.

> > +        return true;
> > +
> > +    return false;
> > +}
> > +
> >  static int __init process_memory_node(const void *fdt, int node,
> >                                        const char *name, int depth,
> >                                        u32 address_cells, u32 size_cells,
> >                                        void *data)
> >  {
> > +    if (!memory_node_is_available(fdt, node))
> > +        return 0;
>
> I would move this check to device_tree_get_meminfo()

Okay, I will do it.

> > +
> >      return device_tree_get_meminfo(fdt, node, "reg", address_cells, 
> > size_cells,
> >                                     data, MEMBANK_DEFAULT);
> >  }
> > --
> > 2.39.2
> > 
> > 
> 
> Also, I think it would be nice to add ASSERT(bootinfo.mem.nr_banks); e.g. in 
> boot_fdt_info().
> Otherwise the panic from Xen when there is no memory bank:
> The frametable cannot cover the physical region ...
> is not really meaningful for normal users.

I'd like to use a separate patch to validate the memory banks.
> 
> This is just my suggestion (@Julien ?)

Thank you a lot for review.

Leo