Re: [PATCH] of/kexec: save pa of initial_boot_params for arm64 and use it at kexec

[Usama Arif <usamaarif642@gmail.com>]

On 07/10/2024 15:39, Rob Herring wrote:
> On Mon, Oct 7, 2024 at 9:06 AM Usama Arif <usamaarif642@gmail.com> 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@debian.org>
>>>> Suggested-by: Mark Rutland <mark.rutland@arm.com>
>>>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
>>>> 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?