Hi Michael Ellerman,
Michael Ellerman <m...@ellerman.id.au> writes:
> Jinglin Wen <jinglin....@shingroup.cn> writes:
> > According to the code logic, when the kernel is loaded to address 0,
> > no copying operation should be performed, but it is currently being
> > done.
> >
> > This patch fixes the issue where the kernel code was incorrectly
> > duplicated to address 0 when booting from address 0.
> >
> > Signed-off-by: Jinglin Wen <jinglin....@shingroup.cn>
> > ---
> > arch/powerpc/kernel/head_64.S | 4 +++-
> > 1 file changed, 3 insertions(+), 1 deletion(-)
>
> Thanks for the improved change log.
>
> The subject could probably still be clearer, maybe:
> Fix unnecessary copy to 0 when kernel is booted at address 0
Thanks for your feedback, I will revise my subject.
>
> Looks like this was introduced by:
>
> Fixes: b270bebd34e3 ("powerpc/64s: Run at the kernel virtual address
> earlier in boot")
> Cc: sta...@vger.kernel.org # v6.4+
>
> Let me know if you think otherwise.
>
> Just out of interest, how are you hitting this bug? AFAIK none of our
> "normal" boot loaders will load the kernel at 0.
>
> > diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> > index 4690c219bfa4..6c73551bdc50 100644
> > --- a/arch/powerpc/kernel/head_64.S
> > +++ b/arch/powerpc/kernel/head_64.S
> > @@ -647,7 +647,9 @@ __after_prom_start:
> > * Note: This process overwrites the OF exception vectors.
> > */
> > LOAD_REG_IMMEDIATE(r3, PAGE_OFFSET)
> > - mr. r4,r26 /* In some cases the loader may */
> > + tophys(r4,r26)
> > + cmplwi cr0,r4,0 /* runtime base addr is zero */
> > + mr r4,r26 /* In some cases the loader may */
> > beq 9f /* have already put us at zero */
>
> That is a pretty minimal fix, but I think the code would be clearer if
> we just compared the source and destination addresses.
>
> Something like the diff below. Can you confirm that works for you.
>
> cheers
>
As for how I discovered this bug, we use zImage.epapr for emulation, which
loads vmlinux.bin at address 0. When vmlinux.bin is relatively large, I
found that the boot time of Linux 6.6 is much slower compared to Linux
5.10.108.
I discovered this issue while comparing the code between the two versions.
> diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
> index 4690c219bfa4..6ad1435303f9 100644
> --- a/arch/powerpc/kernel/head_64.S
> +++ b/arch/powerpc/kernel/head_64.S
> @@ -647,8 +647,9 @@ __after_prom_start:
> * Note: This process overwrites the OF exception vectors.
> */
> LOAD_REG_IMMEDIATE(r3, PAGE_OFFSET)
> - mr. r4,r26 /* In some cases the loader may */
> - beq 9f /* have already put us at zero */
> + mr r4, r26 // Load the source address into r4
> + cmpld cr0, r3, r4 // Check if source == dest
> + beq 9f // If so skip the copy
> li r6,0x100 /* Start offset, the first 0x100 */
> /* bytes were copied earlier. */
Indeed, your code looks much clearer. I will make the following modifications
based on your code:
diff --git a/arch/powerpc/kernel/head_64.S b/arch/powerpc/kernel/head_64.S
index 4690c219bfa4..751181dfb897 100644
--- a/arch/powerpc/kernel/head_64.S
+++ b/arch/powerpc/kernel/head_64.S
@@ -647,8 +647,9 @@ __after_prom_start:
* Note: This process overwrites the OF exception vectors.
*/
LOAD_REG_IMMEDIATE(r3, PAGE_OFFSET)
- mr. r4,r26 /* In some cases the loader may */
- beq 9f /* have already put us at zero */
+ mr r4,r26 /* Load the virtual source address into
r4 */
+ cmpd r3,r4 /* Check if source == dest */
+ beq 9f /* If so skip the copy */
li r6,0x100 /* Start offset, the first 0x100 */
/* bytes were copied earlier. */
Thanks,
Jinglin Wen
