[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/arm: Skip memory nodes if not enabled
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
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |