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 16:30, Usama Arif wrote:
> 
> 
> 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?

Hi,

Just wanted to check in on above. I feel what I am doing here is similar to
early_init_dt_scan so should be ok? There is just a small comment from Saravana
that I need to address which I can in v2, but wanted to check on above before
sending it.

Thanks,
Usama