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@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?
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
