Hi,
On Tue, Dec 14, 2021 at 11:20:11PM -0800, Stephen Boyd wrote:
> In commit 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove
> already reserved regions") we returned -EBUSY when trying to mark
> regions as no-map when they're in the reserved memory node. This if
> condition will still trigger though if the DT has a /memreserve/ that
> completely subsumes the no-map memory carveouts in the reserved memory
> node. Let's only consider this to be a problem if we're trying to mark a
> region as no-map and it is actually memory. If it isn't memory,
> presumably it was removed from the memory map via /memreserve/ and thus
> can't be mapped anyway.
I have no objections for this patch, but I afraid that this is a never
ending story of reservation vs nomap ordering and this won't be the last
fix in the area.
I was toying with the idea to use flags in memblock.reserved to have
clearer view of how the reserved memory was used and then we won't need
to guess firmware intentions.
Thoughts?
> This silences a warning seen at boot on sc7180-trogdor.dtsi boards that
> have /memreserve/ populated by the bootloader where those reserved
> regions overlap with the reserved-memory carveouts that we have in DT
> for other purposes like communicating with remote processors.
Do you mind adding the relevant pats of the device tree to the changelog?
> For example
>
> OF: fdt: Reserved memory: failed to reserve memory for node 'memory@80900000': base 0x0000000080900000, size 2 MiB
>
> Cc: Mike Rapoport <rppt@xxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Cc: Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
> Cc: Quentin Perret <qperret@xxxxxxxxxx>
> Cc: Jan Kiszka <jan.kiszka@xxxxxxxxxxx>
> Fixes: 8a5a75e5e9e5 ("of/fdt: Make sure no-map does not remove already reserved regions")
> Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> ---
>
> Changes from v1 (https://lore.kernel.org/r/20210520012731.3731314-1-swboyd@xxxxxxxxxxxx):
> * Use memblock_overlaps_region instead of memblock_is_region_memory()
> * Add more details to commit text
>
> drivers/of/fdt.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index bdca35284ceb..c736e5bcc2f6 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -482,9 +482,11 @@ static int __init early_init_dt_reserve_memory_arch(phys_addr_t base,
> if (nomap) {
> /*
> * If the memory is already reserved (by another region), we
> - * should not allow it to be marked nomap.
> + * should not allow it to be marked nomap, but don't worry
> + * if the region isn't memory as it won't be mapped.
> */
> - if (memblock_is_region_reserved(base, size))
> + if (memblock_overlaps_region(&memblock.memory, base, size) &&
> + memblock_is_region_reserved(base, size))
> return -EBUSY;
>
> return memblock_mark_nomap(base, size);
>
> base-commit: 136057256686de39cc3a07c2e39ef6bc43003ff6
> --
> https://chromeos.dev
>
--
Sincerely yours,
Mike.
