Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20211002122026.1451269-1-mick@ics.forth.gr>
In-Reply-To: <20211002122026.1451269-1-mick@ics.forth.gr>
From:   Alexandre Ghiti <alexandre.ghiti@canonical.com>
Date:   Wed, 6 Oct 2021 13:13:07 +0200
Message-ID: <CA+zEjCtJqNVrtr_Wkij2c=3+P5kL6jJN5RPULfZj2c5mGzCVjw@mail.gmail.com>
Subject: Re: [PATCH] riscv: Don't use va_pa_offset on kdump
To:     Nick Kossifidis <mick@ics.forth.gr>
Cc:     Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        linux-riscv <linux-riscv@lists.infradead.org>,
        "linux-kernel@vger.kernel.org List" <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sat, Oct 2, 2021 at 2:23 PM Nick Kossifidis <mick@ics.forth.gr> wrote:
>
> On kdump instead of using an intermediate step to relocate the kernel, that
> lives in a "control buffer" outside the current kernel's mapping, we jump
> to the crash kernel directly by calling riscv_kexec_norelocate(). The
> current implementation uses va_pa_offset while switching to physical
> addressing, however since we moved the kernel outside the linear mapping
> this won't work anymore since riscv_kexec_norelocate() is part of the
> kernel mapping and we should use kernel_map.va_kernel_pa_offset, and also
> take XIP kernel into account.
>
> We don't really need to use va_pa_offset on riscv_kexec_norelocate, we can
> just set STVEC to the physical address of the new kernel instead and let
> the hart jump to the new kernel on the next instruction after setting
> SATP to zero. This fixes kdump and is also simpler/cleaner.
>
> Signed-off-by: Nick Kossifidis <mick@ics.forth.gr>
> ---
>  arch/riscv/kernel/kexec_relocate.S | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/arch/riscv/kernel/kexec_relocate.S b/arch/riscv/kernel/kexec_relocate.S
> index a80b52a74..e2f34196e 100644
> --- a/arch/riscv/kernel/kexec_relocate.S
> +++ b/arch/riscv/kernel/kexec_relocate.S
> @@ -159,25 +159,15 @@ SYM_CODE_START(riscv_kexec_norelocate)
>          * s0: (const) Phys address to jump to
>          * s1: (const) Phys address of the FDT image
>          * s2: (const) The hartid of the current hart
> -        * s3: (const) kernel_map.va_pa_offset, used when switching MMU off
>          */
>         mv      s0, a1
>         mv      s1, a2
>         mv      s2, a3
> -       mv      s3, a4
>
>         /* Disable / cleanup interrupts */
>         csrw    CSR_SIE, zero
>         csrw    CSR_SIP, zero
>
> -       /* Switch to physical addressing */
> -       la      s4, 1f
> -       sub     s4, s4, s3
> -       csrw    CSR_STVEC, s4
> -       csrw    CSR_SATP, zero
> -
> -.align 2
> -1:
>         /* Pass the arguments to the next kernel  / Cleanup*/
>         mv      a0, s2
>         mv      a1, s1
> @@ -214,6 +204,11 @@ SYM_CODE_START(riscv_kexec_norelocate)
>         csrw    CSR_SCAUSE, zero
>         csrw    CSR_SSCRATCH, zero
>
> +       /* Switch to physical addressing */
> +       csrw    CSR_STVEC, a2
> +       csrw    CSR_SATP, zero
> +
> +       /* This will trigger a jump to CSR_STVEC anyway */
>         jalr    zero, a2, 0

The last jump to a2 can be removed since the fault will be triggered
before even reaching this instruction.

>  SYM_CODE_END(riscv_kexec_norelocate)
>
> --
> 2.32.0
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

This patch fixes a regression introduced when moving the kernel to the
end of the address space, so we should add:
Fixes: 2bfc6cd81bd1 ("riscv: Move kernel mapping outside of linear mapping")

And it should be backported to 5.13 and 5.14. It seems that the
following tags should be enough:

Cc: <stable@vger.kernel.org> # 5.13
Cc: <stable@vger.kernel.org> # 5.14

And finally, you can add:

Reviewed-by: Alexandre Ghiti <alex@ghiti.fr>

Thanks,

Alex