On 07/10/2024 15:39, Rob Herring wrote: > On Mon, Oct 7, 2024 at 9:06 AM Usama Arif <usamaarif642@xxxxxxxxx> wrote: >> >> >> >> On 04/10/2024 01:03, Rob Herring wrote: >>> On Thu, Oct 03, 2024 at 12:38:40PM +0100, Usama Arif wrote: >>>> __pa() is only intended to be used for linear map addresses and using >>>> it for initial_boot_params which is in fixmap for arm64 will give an >>>> incorrect value. Hence stash the physical address when it is known at >>>> boot time and use it at kexec time instead of converting the virtual >>>> address using __pa(). >>>> >>>> Reported-by: Breno Leitao <leitao@xxxxxxxxxx> >>>> Suggested-by: Mark Rutland <mark.rutland@xxxxxxx> >>>> Signed-off-by: Usama Arif <usamaarif642@xxxxxxxxx> >>>> Fixes: ac10be5cdbfa ("arm64: Use common of_kexec_alloc_and_setup_fdt()") >>>> --- >>>> arch/arm64/kernel/setup.c | 8 ++++++++ >>>> drivers/of/fdt.c | 6 ++++++ >>>> drivers/of/kexec.c | 8 ++++++-- >>>> include/linux/of_fdt.h | 2 ++ >>>> 4 files changed, 22 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c >>>> index b22d28ec8028..a4d96f5e2e05 100644 >>>> --- a/arch/arm64/kernel/setup.c >>>> +++ b/arch/arm64/kernel/setup.c >>>> @@ -194,6 +194,14 @@ static void __init setup_machine_fdt(phys_addr_t dt_phys) >>>> /* Early fixups are done, map the FDT as read-only now */ >>>> fixmap_remap_fdt(dt_phys, &size, PAGE_KERNEL_RO); >>>> >>>> + /* >>>> + * Save dt_phys address so that it can be used later for kexec. This >>>> + * is done as __pa() is only intended to be used for linear map addresses >>>> + * and using it for initial_boot_params which is in fixmap will give an >>>> + * incorrect value. >>>> + */ >>>> + set_initial_boot_params_pa(dt_phys); >>> >>> No new arch->dt functions please. If we need to save off the PA, then do >>> that when we set initial_boot_params. >>> >>> Rob >> >> >> initial_boot_params is set in early_init_dt_verify, called by early_init_dt_scan. >> This is done in setup_machine_fdt in arm64 where the PA is available, >> but in other functions in other architectures, where the PA is not available. > > Doesn't __pa() work for all the other architectures? That's what your > patch indicates. > Yes, __pa() works for all other architectures. But we would need to add initial_boot_params_pa of type phys_addr_t as an argument for early_init_dt_scan, which is called by all other archs, and we technically cant use 0 as an invalid value. We could convert initial_boot_params_pa to void *, and pass NULL for all other archs. But again, I don't really think we should be changing the early_init_dt_scan(dt_virt) call in all other archs to early_init_dt_scan(dt_virt, NULL) just to save initial_boot_params_pa in arm64? >> So it makes it quite messy to set it in the same place as initial_boot_params. >> Its only needed for arm64 and making a change in all archs probably isnt a good idea? >> >> Any reason to not add a new function to make arch -> of/fdt call? > > Yes. It is the opposite direction I have reworked the interfaces to. > We don't want each arch calling various early DT functions at random > times and order. That's fragile when the DT functions make assumptions > about when they are called or what's been initialized already. > > Another option is to make arm64 copy the DT as some arches do. > > Rob Ah maybe I didn't understand this properly, but isnt early_init_dt_scan an arch -> of/fdt interfaces. set_initial_boot_params_pa is a similar interface to early_init_dt_scan? _______________________________________________ kexec mailing list kexec@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/kexec