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

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

[Usama Arif]

 __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);
+
 	name = of_flat_dt_get_machine_name();
 	if (!name)
 		return;
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index 4d528c10df3a..9e312b7c246e 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -457,6 +457,7 @@ int __initdata dt_root_addr_cells;
 int __initdata dt_root_size_cells;
 
 void *initial_boot_params __ro_after_init;
+phys_addr_t initial_boot_params_pa __ro_after_init;
 
 #ifdef CONFIG_OF_EARLY_FLATTREE
 
@@ -1185,6 +1186,11 @@ bool __init early_init_dt_scan(void *params)
 	return true;
 }
 
+void __init set_initial_boot_params_pa(phys_addr_t params)
+{
+	initial_boot_params_pa = params;
+}
+
 static void *__init copy_device_tree(void *fdt)
 {
 	int size;
diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
index 9ccde2fd77cb..ca9f27b27f71 100644
--- a/drivers/of/kexec.c
+++ b/drivers/of/kexec.c
@@ -300,8 +300,12 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
 		goto out;
 	}
 
-	/* Remove memory reservation for the current device tree. */
-	ret = fdt_find_and_del_mem_rsv(fdt, __pa(initial_boot_params),
+	/* Remove memory reservation for the current device tree.
+	 * For arm64, initial_boot_params is a fixmap address, hence __pa(),
+	 * can't be used to get the physical address.
+	 */
+	ret = fdt_find_and_del_mem_rsv(fdt, IS_ENABLED(CONFIG_ARM64) ?
+				       initial_boot_params_pa : __pa(initial_boot_params),
 				       fdt_totalsize(initial_boot_params));
 	if (ret == -EINVAL) {
 		pr_err("Error removing memory reservation.\n");
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index d69ad5bb1eb1..dbd99bf21ac8 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -31,6 +31,7 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
 extern int __initdata dt_root_addr_cells;
 extern int __initdata dt_root_size_cells;
 extern void *initial_boot_params;
+extern phys_addr_t initial_boot_params_pa;
 
 extern char __dtb_start[];
 extern char __dtb_end[];
@@ -73,6 +74,7 @@ extern int early_init_dt_scan_root(void);
 extern bool early_init_dt_scan(void *params);
 extern bool early_init_dt_verify(void *params);
 extern void early_init_dt_scan_nodes(void);
+extern void set_initial_boot_params_pa(phys_addr_t params);
 
 extern const char *of_flat_dt_get_machine_name(void);
 extern const void *of_flat_dt_match_machine(const void *default_match,
-- 
2.43.5

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

[Saravana Kannan]

On Thu, Oct 3, 2024 at 4:38 AM Usama Arif <usamaarif642@gmail.com> 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);
> +
>         name = of_flat_dt_get_machine_name();
>         if (!name)
>                 return;
> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
> index 4d528c10df3a..9e312b7c246e 100644
> --- a/drivers/of/fdt.c
> +++ b/drivers/of/fdt.c
> @@ -457,6 +457,7 @@ int __initdata dt_root_addr_cells;
>  int __initdata dt_root_size_cells;
>
>  void *initial_boot_params __ro_after_init;
> +phys_addr_t initial_boot_params_pa __ro_after_init;
>
>  #ifdef CONFIG_OF_EARLY_FLATTREE
>
> @@ -1185,6 +1186,11 @@ bool __init early_init_dt_scan(void *params)
>         return true;
>  }
>
> +void __init set_initial_boot_params_pa(phys_addr_t params)
> +{
> +       initial_boot_params_pa = params;
> +}
> +
>  static void *__init copy_device_tree(void *fdt)
>  {
>         int size;
> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
> index 9ccde2fd77cb..ca9f27b27f71 100644
> --- a/drivers/of/kexec.c
> +++ b/drivers/of/kexec.c
> @@ -300,8 +300,12 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>                 goto out;
>         }
>
> -       /* Remove memory reservation for the current device tree. */
> -       ret = fdt_find_and_del_mem_rsv(fdt, __pa(initial_boot_params),
> +       /* Remove memory reservation for the current device tree.
> +        * For arm64, initial_boot_params is a fixmap address, hence __pa(),
> +        * can't be used to get the physical address.
> +        */
> +       ret = fdt_find_and_del_mem_rsv(fdt, IS_ENABLED(CONFIG_ARM64) ?
> +                                      initial_boot_params_pa : __pa(initial_boot_params),
>                                        fdt_totalsize(initial_boot_params));

Not sure about the correctness of the patch (not a kexec expert) but
no need to do all of this inside a function parameter. Just create a
variable and use it here.

-Saravana

>         if (ret == -EINVAL) {
>                 pr_err("Error removing memory reservation.\n");
> diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
> index d69ad5bb1eb1..dbd99bf21ac8 100644
> --- a/include/linux/of_fdt.h
> +++ b/include/linux/of_fdt.h
> @@ -31,6 +31,7 @@ extern void *of_fdt_unflatten_tree(const unsigned long *blob,
>  extern int __initdata dt_root_addr_cells;
>  extern int __initdata dt_root_size_cells;
>  extern void *initial_boot_params;
> +extern phys_addr_t initial_boot_params_pa;
>
>  extern char __dtb_start[];
>  extern char __dtb_end[];
> @@ -73,6 +74,7 @@ extern int early_init_dt_scan_root(void);
>  extern bool early_init_dt_scan(void *params);
>  extern bool early_init_dt_verify(void *params);
>  extern void early_init_dt_scan_nodes(void);
> +extern void set_initial_boot_params_pa(phys_addr_t params);
>
>  extern const char *of_flat_dt_get_machine_name(void);
>  extern const void *of_flat_dt_match_machine(const void *default_match,
> --
> 2.43.5
>

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

[Usama Arif]

On 03/10/2024 21:20, Saravana Kannan wrote:
> On Thu, Oct 3, 2024 at 4:38 AM Usama Arif <usamaarif642@gmail.com> 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);
>> +
>>         name = of_flat_dt_get_machine_name();
>>         if (!name)
>>                 return;
>> diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
>> index 4d528c10df3a..9e312b7c246e 100644
>> --- a/drivers/of/fdt.c
>> +++ b/drivers/of/fdt.c
>> @@ -457,6 +457,7 @@ int __initdata dt_root_addr_cells;
>>  int __initdata dt_root_size_cells;
>>
>>  void *initial_boot_params __ro_after_init;
>> +phys_addr_t initial_boot_params_pa __ro_after_init;
>>
>>  #ifdef CONFIG_OF_EARLY_FLATTREE
>>
>> @@ -1185,6 +1186,11 @@ bool __init early_init_dt_scan(void *params)
>>         return true;
>>  }
>>
>> +void __init set_initial_boot_params_pa(phys_addr_t params)
>> +{
>> +       initial_boot_params_pa = params;
>> +}
>> +
>>  static void *__init copy_device_tree(void *fdt)
>>  {
>>         int size;
>> diff --git a/drivers/of/kexec.c b/drivers/of/kexec.c
>> index 9ccde2fd77cb..ca9f27b27f71 100644
>> --- a/drivers/of/kexec.c
>> +++ b/drivers/of/kexec.c
>> @@ -300,8 +300,12 @@ void *of_kexec_alloc_and_setup_fdt(const struct kimage *image,
>>                 goto out;
>>         }
>>
>> -       /* Remove memory reservation for the current device tree. */
>> -       ret = fdt_find_and_del_mem_rsv(fdt, __pa(initial_boot_params),
>> +       /* Remove memory reservation for the current device tree.
>> +        * For arm64, initial_boot_params is a fixmap address, hence __pa(),
>> +        * can't be used to get the physical address.
>> +        */
>> +       ret = fdt_find_and_del_mem_rsv(fdt, IS_ENABLED(CONFIG_ARM64) ?
>> +                                      initial_boot_params_pa : __pa(initial_boot_params),
>>                                        fdt_totalsize(initial_boot_params));
> 
> Not sure about the correctness of the patch (not a kexec expert) but
> no need to do all of this inside a function parameter. Just create a
> variable and use it here.
> 
> -Saravana
> 

Thanks, will do in the next revision.

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

[Rob Herring]

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

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

[Usama Arif]

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

Thanks,
Usama

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

[Rob Herring]

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.

> 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

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

[Usama Arif]

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?

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

[Usama Arif]

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

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

[Rob Herring]

On Mon, Oct 7, 2024 at 10:30 AM Usama Arif <usamaarif642@gmail.com> 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?

Yes, and I don't want more APIs if they can be avoided. When is
set_initial_boot_params_pa() supposed to be called? Is it before or
after early_init_dt_scan()? Can subsequent OF functions assume the PA
is valid? If an arch doesn't call set_initial_boot_params_pa() is
__pa() valid or did they just forget to call it? Requiring the PA to
be set at the same time as initial_boot_params avoids all those issues
with any period of time having the PA incorrect.

Rob

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

[Usama Arif]

On 23/10/2024 14:40, Rob Herring wrote:
> On Mon, Oct 7, 2024 at 10:30 AM Usama Arif <usamaarif642@gmail.com> 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?
> 
> Yes, and I don't want more APIs if they can be avoided. When is
> set_initial_boot_params_pa() supposed to be called? Is it before or
> after early_init_dt_scan()? 

Its only needed in arm64, and can be either before or after, as long as its
somewhere in setup_machine_fdt, where dt_phys is available.

> Can subsequent OF functions assume the PA
> is valid?

After set_initial_boot_params_pa has been called, yes.

> If an arch doesn't call set_initial_boot_params_pa() is
> __pa() valid or did they just forget to call it? 

Only arm64 seems to do the fixmap as discussed in
https://lore.kernel.org/all/1ea5538f-7e96-4034-9af9-e2d5fd72e069@gmail.com/,
so __pa should work in others.

Requiring the PA to
> be set at the same time as initial_boot_params avoids all those issues
> with any period of time having the PA incorrect.
> 

Are you recommending I send a patch which changes all archs to call
early_init_dt_scan(dt_virt, NULL)?
or maybe early_init_dt_scan(dt_virt, __pa(dt_virt))?
and arm to call early_init_dt_scan(dt_virt, dt_phys).

Happy to do send a v2 with that if its the way forward, although I feel
set_initial_boot_params_pa() in just one arch might be better than
changing this for all archs.

Thanks
Usama

> Rob

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

[Rob Herring]

On Wed, Oct 23, 2024 at 9:43 AM Usama Arif <usamaarif642@gmail.com> wrote:
>
>
>
> On 23/10/2024 14:40, Rob Herring wrote:
> > On Mon, Oct 7, 2024 at 10:30 AM Usama Arif <usamaarif642@gmail.com> 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?
> >
> > Yes, and I don't want more APIs if they can be avoided. When is
> > set_initial_boot_params_pa() supposed to be called? Is it before or
> > after early_init_dt_scan()?
>
> Its only needed in arm64, and can be either before or after, as long as its
> somewhere in setup_machine_fdt, where dt_phys is available.

Maybe only arm64 today. What happens when riscv decides they too want
to support the DT anywhere in memory including outside the linear
address map and then they need the same thing.

> > Can subsequent OF functions assume the PA
> > is valid?
>
> After set_initial_boot_params_pa has been called, yes.

How do I know it has been called? Do I have to go wade thru every arch
to see? You could document the requirement to be immediately after
early_init_dt_scan(), but then how do you enforce that? You can't
unless you design the interface to just avoid the problem in the first
place.

> > If an arch doesn't call set_initial_boot_params_pa() is
> > __pa() valid or did they just forget to call it?
>
> Only arm64 seems to do the fixmap as discussed in
> https://lore.kernel.org/all/1ea5538f-7e96-4034-9af9-e2d5fd72e069@gmail.com/,
> so __pa should work in others.
>
> Requiring the PA to
> > be set at the same time as initial_boot_params avoids all those issues
> > with any period of time having the PA incorrect.
> >
>
> Are you recommending I send a patch which changes all archs to call
> early_init_dt_scan(dt_virt, NULL)?
> or maybe early_init_dt_scan(dt_virt, __pa(dt_virt))?
> and arm to call early_init_dt_scan(dt_virt, dt_phys).

I believe that's what I suggested already, so yes. Whether NULL or
__pa(dt_virt))? __pa() would be better because then the arch has to
think about whether that is right or not.

> Happy to do send a v2 with that if its the way forward, although I feel
> set_initial_boot_params_pa() in just one arch might be better than
> changing this for all archs.

We don't work around kernel APIs if they don't meet changing needs. We
change them.

Rob

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

[Usama Arif]

On 23/10/2024 16:17, Rob Herring wrote:
> On Wed, Oct 23, 2024 at 9:43 AM Usama Arif <usamaarif642@gmail.com> wrote:
>>
>>
>>
>> On 23/10/2024 14:40, Rob Herring wrote:
>>> On Mon, Oct 7, 2024 at 10:30 AM Usama Arif <usamaarif642@gmail.com> 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?
>>>
>>> Yes, and I don't want more APIs if they can be avoided. When is
>>> set_initial_boot_params_pa() supposed to be called? Is it before or
>>> after early_init_dt_scan()?
>>
>> Its only needed in arm64, and can be either before or after, as long as its
>> somewhere in setup_machine_fdt, where dt_phys is available.
> 
> Maybe only arm64 today. What happens when riscv decides they too want
> to support the DT anywhere in memory including outside the linear
> address map and then they need the same thing.
> 
>>> Can subsequent OF functions assume the PA
>>> is valid?
>>
>> After set_initial_boot_params_pa has been called, yes.
> 
> How do I know it has been called? Do I have to go wade thru every arch
> to see? You could document the requirement to be immediately after
> early_init_dt_scan(), but then how do you enforce that? You can't
> unless you design the interface to just avoid the problem in the first
> place.
> 
>>> If an arch doesn't call set_initial_boot_params_pa() is
>>> __pa() valid or did they just forget to call it?
>>
>> Only arm64 seems to do the fixmap as discussed in
>> https://lore.kernel.org/all/1ea5538f-7e96-4034-9af9-e2d5fd72e069@gmail.com/,
>> so __pa should work in others.
>>
>> Requiring the PA to
>>> be set at the same time as initial_boot_params avoids all those issues
>>> with any period of time having the PA incorrect.
>>>
>>
>> Are you recommending I send a patch which changes all archs to call
>> early_init_dt_scan(dt_virt, NULL)?
>> or maybe early_init_dt_scan(dt_virt, __pa(dt_virt))?
>> and arm to call early_init_dt_scan(dt_virt, dt_phys).
> 
> I believe that's what I suggested already, so yes. Whether NULL or
> __pa(dt_virt))? __pa() would be better because then the arch has to
> think about whether that is right or not.


Sounds good! Will send a v2 with the change.

Thanks
> 
>> Happy to do send a v2 with that if its the way forward, although I feel
>> set_initial_boot_params_pa() in just one arch might be better than
>> changing this for all archs.
> 
> We don't work around kernel APIs if they don't meet changing needs. We
> change them.
> 
> Rob