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