Hi, On 07/10/2019 22:30, Stefano Stabellini wrote: > On Mon, 7 Oct 2019, Julien Grall wrote: >> On 05/10/2019 00:09, Stefano Stabellini wrote: >>> When reserved-memory regions are present in the host device tree, dom0 >>> is started with multiple memory nodes. Each memory node should have a >>> unique name, but today they are all called "memory" leading to Linux >>> printing the following warning at boot: >>> >>> OF: Duplicate name in base, renamed to "memory#1" >>> >>> This patch fixes the problem by appending a "@<unit-address>" to the >>> name, as per the Device Tree specification, where <unit-address> matches >>> the base of address of the first region. >>> >>> Reported-by: Oleksandr Tyshchenko <[email protected]> >>> Signed-off-by: Stefano Stabellini <[email protected]> >>> >>> --- >>> >>> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c >>> index 921b054520..a4c07db383 100644 >>> --- a/xen/arch/arm/domain_build.c >>> +++ b/xen/arch/arm/domain_build.c >>> @@ -646,16 +646,22 @@ static int __init make_memory_node(const struct domain >>> *d, >>> int res, i; >>> int reg_size = addrcells + sizecells; >>> int nr_cells = reg_size * mem->nr_banks; >>> + /* Placeholder for memory@ + a 32-bit number + \0 */ >>> + char buf[18]; >>> __be32 reg[NR_MEM_BANKS * 4 /* Worst case addrcells + sizecells */]; >>> __be32 *cells; >>> BUG_ON(nr_cells >= ARRAY_SIZE(reg)); >>> + /* Nothing to do */ >> >> This a departure from the current solution where a node will be created with >> no "reg" property. I think this change of behavior should at least be >> described in the commit message if not implemented in a separate patch. >> But... >> >>> + if ( mem->nr_banks == 0 ) >>> + return 0; >> >> ... I don't think we want to ignore it. The caller most likely messed up the >> banks and we should instead report an error. > > I admit it wasn't my intention to change the current behavior. As I was > looking through the code I noticed that we call make_memory_node for > both normal memory and reserved_memory. Of course, reserved_memory could > have no banks. So I thought it would be good to check whether there are > any banks before continuing because now we are going to access > mem->bank[0].start, which would be a mistake if there are no banks.
Ok, so this not theoritical bug as I first thought but a real bug on platform where DT does not have reserved-regions node. In this case, this should be in a separate patch as this is now 2 different bugs solved in one patch. > > In regards to your comment about returning error, we could return ENOENT, > however we would also have to handle ENOENT especially at the caller > side (handle_node). Or we would have to add a check if ( mem->nr_banks > > 0) to avoid calling make_memory_node when nr_banks is zero. I would much prefer if we check mem->nr_banks > 0 for reserved-regions before hand. Both will need a "Fixes:" to keep track of the original patch. Cheers, -- Julien Grall _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
