Hari Bathini <hbath...@linux.ibm.com> writes:
> diff --git a/arch/powerpc/kexec/file_load_64.c 
> b/arch/powerpc/kexec/file_load_64.c
> index a5c1442590b2..88408b17a7f6 100644
> --- a/arch/powerpc/kexec/file_load_64.c
> +++ b/arch/powerpc/kexec/file_load_64.c
> @@ -697,6 +699,69 @@ static int update_usable_mem_fdt(void *fdt, struct 
> crash_mem *usable_mem)
>       return ret;
>  }
>  
> +/**
> + * load_backup_segment - Locate a memory hole to place the backup region.
> + * @image:               Kexec image.
> + * @kbuf:                Buffer contents and memory parameters.
> + *
> + * Returns 0 on success, negative errno on error.
> + */
> +static int load_backup_segment(struct kimage *image, struct kexec_buf *kbuf)
> +{
> +     void *buf;
> +     int ret;
> +
> +     /* Setup a segment for backup region */
> +     buf = vzalloc(BACKUP_SRC_SIZE);

This worried me initially, because we can't copy from physically
discontiguous pages in real mode.

But as you explained this buffer is not used for copying.

I think if you move the large comment below up here, it would be
clearer.


> diff --git a/arch/powerpc/purgatory/trampoline_64.S 
> b/arch/powerpc/purgatory/trampoline_64.S
> index 464af8e8a4cb..d4b52961f592 100644
> --- a/arch/powerpc/purgatory/trampoline_64.S
> +++ b/arch/powerpc/purgatory/trampoline_64.S
> @@ -43,14 +44,38 @@ master:
>       mr      %r17,%r3        /* save cpu id to r17 */
>       mr      %r15,%r4        /* save physical address in reg15 */
>  
> +     bl      0f              /* Work out where we're running */
> +0:   mflr    %r18

I know you just moved it, but this should use:

        bcl     20, 31, $+4
        mflr    %r18

Which is a special form of branch and link that doesn't unbalance the
link stack in the chip.

> +     /*
> +      * Copy BACKUP_SRC_SIZE bytes from BACKUP_SRC_START to
> +      * backup_start 8 bytes at a time.
> +      *
> +      * Use r3 = dest, r4 = src, r5 = size, r6 = count
> +      */
> +     ld      %r3,(backup_start - 0b)(%r18)
> +     cmpdi   %cr0,%r3,0

I prefer spaces or tabs between arguments, eg:

        cmpdi   %cr0, %r3, 0

> +     beq     80f             /* skip if there is no backup region */

Local labels will make this clearer I think. eg:

        beq     .Lskip_copy

> +     lis     %r5,BACKUP_SRC_SIZE@h
> +     ori     %r5,%r5,BACKUP_SRC_SIZE@l
> +     cmpdi   %cr0,%r5,0
> +     beq     80f             /* skip if copy size is zero */
> +     lis     %r4,BACKUP_SRC_START@h
> +     ori     %r4,%r4,BACKUP_SRC_START@l
> +     li      %r6,0
> +70:

.Lcopy_loop:

> +     ldx     %r0,%r6,%r4
> +     stdx    %r0,%r6,%r3
> +     addi    %r6,%r6,8
> +     cmpld   %cr0,%r6,%r5
> +     blt     70b

        blt     .Lcopy_loop

> +

.Lskip_copy:

> +80:
>       or      %r3,%r3,%r3     /* ok now to high priority, lets boot */
>       lis     %r6,0x1
>       mtctr   %r6             /* delay a bit for slaves to catch up */
>       bdnz    .               /* before we overwrite 0-100 again */


cheers

Reply via email to