Re: [PATCH 1/2] kdump: vmcore: move copy_to() from vmcore.c to uaccess.h

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 10/12/2021 à 17:59, Andrew Morton a écrit :
> On Fri, 10 Dec 2021 21:36:00 +0800 Tiezhu Yang <yangtiezhu@loongson.cn> wrote:
> 
>> In arch/*/kernel/crash_dump*.c, there exist similar code about
>> copy_oldmem_page(), move copy_to() from vmcore.c to uaccess.h,
>> and then we can use copy_to() to simplify the related code.
>>
>> ...
>>
>> --- a/fs/proc/vmcore.c
>> +++ b/fs/proc/vmcore.c
>> @@ -238,20 +238,6 @@ copy_oldmem_page_encrypted(unsigned long pfn, char *buf, size_t csize,
>>   	return copy_oldmem_page(pfn, buf, csize, offset, userbuf);
>>   }
>>   
>> -/*
>> - * Copy to either kernel or user space
>> - */
>> -static int copy_to(void *target, void *src, size_t size, int userbuf)
>> -{
>> -	if (userbuf) {
>> -		if (copy_to_user((char __user *) target, src, size))
>> -			return -EFAULT;
>> -	} else {
>> -		memcpy(target, src, size);
>> -	}
>> -	return 0;
>> -}
>> -
>>   #ifdef CONFIG_PROC_VMCORE_DEVICE_DUMP
>>   static int vmcoredd_copy_dumps(void *dst, u64 start, size_t size, int userbuf)
>>   {
>> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
>> index ac03940..4a6c3e4 100644
>> --- a/include/linux/uaccess.h
>> +++ b/include/linux/uaccess.h
>> @@ -201,6 +201,20 @@ copy_to_user(void __user *to, const void *from, unsigned long n)
>>   	return n;
>>   }
>>   
>> +/*
>> + * Copy to either kernel or user space
>> + */
>> +static inline int copy_to(void *target, void *src, size_t size, int userbuf)
>> +{
>> +	if (userbuf) {
>> +		if (copy_to_user((char __user *) target, src, size))
>> +			return -EFAULT;
>> +	} else {
>> +		memcpy(target, src, size);
>> +	}
>> +	return 0;
>> +}
>> +
> 
> Ordinarily I'd say "this is too large to be inlined".  But the function
> has only a single callsite per architecture so inlining it won't cause
> bloat at present.
> 
> But hopefully copy_to() will get additional callers in the future, in
> which case it shouldn't be inlined.  So I'm thinking it would be best
> to start out with this as a regular non-inlined function, in
> lib/usercopy.c.
> 
> Also, copy_to() is a very poor name for a globally-visible helper
> function.  Better would be copy_to_user_or_kernel(), although that's
> perhaps a bit long.
> 
> And the `userbuf' arg should have type bool, yes?
> 

I think keeping it inlined is better.

copy_oldmem_page() is bigger with v2 (outlined) than with v1 (inlined), 
see both below:

v1:

00000000 <copy_oldmem_page>:
    0:	94 21 ff e0 	stwu    r1,-32(r1)
    4:	93 e1 00 1c 	stw     r31,28(r1)
    8:	7c bf 2b 79 	mr.     r31,r5
    c:	40 82 00 14 	bne     20 <copy_oldmem_page+0x20>
   10:	83 e1 00 1c 	lwz     r31,28(r1)
   14:	38 60 00 00 	li      r3,0
   18:	38 21 00 20 	addi    r1,r1,32
   1c:	4e 80 00 20 	blr
   20:	28 1f 10 00 	cmplwi  r31,4096
   24:	93 61 00 0c 	stw     r27,12(r1)
   28:	7c 08 02 a6 	mflr    r0
   2c:	93 81 00 10 	stw     r28,16(r1)
   30:	93 a1 00 14 	stw     r29,20(r1)
   34:	7c 9b 23 78 	mr      r27,r4
   38:	90 01 00 24 	stw     r0,36(r1)
   3c:	7c dd 33 78 	mr      r29,r6
   40:	93 c1 00 18 	stw     r30,24(r1)
   44:	7c fc 3b 78 	mr      r28,r7
   48:	40 81 00 08 	ble     50 <copy_oldmem_page+0x50>
   4c:	3b e0 10 00 	li      r31,4096
   50:	54 7e 60 26 	rlwinm  r30,r3,12,0,19
   54:	7f c3 f3 78 	mr      r3,r30
   58:	7f e4 fb 78 	mr      r4,r31
   5c:	48 00 00 01 	bl      5c <copy_oldmem_page+0x5c>
			5c: R_PPC_REL24	memblock_is_region_memory
   60:	2c 03 00 00 	cmpwi   r3,0
   64:	41 82 00 30 	beq     94 <copy_oldmem_page+0x94>
   68:	2c 1c 00 00 	cmpwi   r28,0
   6c:	3f de c0 00 	addis   r30,r30,-16384
   70:	7f 63 db 78 	mr      r3,r27
   74:	7f e5 fb 78 	mr      r5,r31
   78:	7c 9e ea 14 	add     r4,r30,r29
   7c:	41 82 00 7c 	beq     f8 <copy_oldmem_page+0xf8>
   80:	48 00 00 01 	bl      80 <copy_oldmem_page+0x80>
			80: R_PPC_REL24	_copy_to_user
   84:	2c 03 00 00 	cmpwi   r3,0
   88:	41 a2 00 48 	beq     d0 <copy_oldmem_page+0xd0>
   8c:	3b e0 ff f2 	li      r31,-14
   90:	48 00 00 40 	b       d0 <copy_oldmem_page+0xd0>
   94:	7f c3 f3 78 	mr      r3,r30
   98:	38 a0 05 91 	li      r5,1425
   9c:	38 80 10 00 	li      r4,4096
   a0:	48 00 00 01 	bl      a0 <copy_oldmem_page+0xa0>
			a0: R_PPC_REL24	ioremap_prot
   a4:	2c 1c 00 00 	cmpwi   r28,0
   a8:	7c 7e 1b 78 	mr      r30,r3
   ac:	7c 83 ea 14 	add     r4,r3,r29
   b0:	7f e5 fb 78 	mr      r5,r31
   b4:	7f 63 db 78 	mr      r3,r27
   b8:	41 82 00 48 	beq     100 <copy_oldmem_page+0x100>
   bc:	48 00 00 01 	bl      bc <copy_oldmem_page+0xbc>
			bc: R_PPC_REL24	_copy_to_user
   c0:	2c 03 00 00 	cmpwi   r3,0
   c4:	40 82 00 44 	bne     108 <copy_oldmem_page+0x108>
   c8:	7f c3 f3 78 	mr      r3,r30
   cc:	48 00 00 01 	bl      cc <copy_oldmem_page+0xcc>
			cc: R_PPC_REL24	iounmap
   d0:	80 01 00 24 	lwz     r0,36(r1)
   d4:	7f e3 fb 78 	mr      r3,r31
   d8:	83 61 00 0c 	lwz     r27,12(r1)
   dc:	83 81 00 10 	lwz     r28,16(r1)
   e0:	7c 08 03 a6 	mtlr    r0
   e4:	83 a1 00 14 	lwz     r29,20(r1)
   e8:	83 c1 00 18 	lwz     r30,24(r1)
   ec:	83 e1 00 1c 	lwz     r31,28(r1)
   f0:	38 21 00 20 	addi    r1,r1,32
   f4:	4e 80 00 20 	blr
   f8:	48 00 00 01 	bl      f8 <copy_oldmem_page+0xf8>
			f8: R_PPC_REL24	memcpy
   fc:	4b ff ff d4 	b       d0 <copy_oldmem_page+0xd0>
  100:	48 00 00 01 	bl      100 <copy_oldmem_page+0x100>
			100: R_PPC_REL24	memcpy
  104:	4b ff ff c4 	b       c8 <copy_oldmem_page+0xc8>
  108:	3b e0 ff f2 	li      r31,-14
  10c:	4b ff ff bc 	b       c8 <copy_oldmem_page+0xc8>


v2:

00000000 <copy_oldmem_page>:
    0:	94 21 ff e0 	stwu    r1,-32(r1)
    4:	93 e1 00 1c 	stw     r31,28(r1)
    8:	7c bf 2b 79 	mr.     r31,r5
    c:	93 c1 00 18 	stw     r30,24(r1)
   10:	3b c0 00 00 	li      r30,0
   14:	40 82 00 18 	bne     2c <copy_oldmem_page+0x2c>
   18:	7f c3 f3 78 	mr      r3,r30
   1c:	83 e1 00 1c 	lwz     r31,28(r1)
   20:	83 c1 00 18 	lwz     r30,24(r1)
   24:	38 21 00 20 	addi    r1,r1,32
   28:	4e 80 00 20 	blr
   2c:	28 1f 10 00 	cmplwi  r31,4096
   30:	93 61 00 0c 	stw     r27,12(r1)
   34:	7c 08 02 a6 	mflr    r0
   38:	93 81 00 10 	stw     r28,16(r1)
   3c:	93 a1 00 14 	stw     r29,20(r1)
   40:	7c db 33 78 	mr      r27,r6
   44:	90 01 00 24 	stw     r0,36(r1)
   48:	7c 9d 23 78 	mr      r29,r4
   4c:	7c fc 3b 78 	mr      r28,r7
   50:	40 81 00 08 	ble     58 <copy_oldmem_page+0x58>
   54:	3b e0 10 00 	li      r31,4096
   58:	54 7e 60 26 	rlwinm  r30,r3,12,0,19
   5c:	7f c3 f3 78 	mr      r3,r30
   60:	7f e4 fb 78 	mr      r4,r31
   64:	48 00 00 01 	bl      64 <copy_oldmem_page+0x64>
			64: R_PPC_REL24	memblock_is_region_memory
   68:	2c 03 00 00 	cmpwi   r3,0
   6c:	41 82 00 54 	beq     c0 <copy_oldmem_page+0xc0>
   70:	3f de c0 00 	addis   r30,r30,-16384
   74:	7c 9e da 14 	add     r4,r30,r27
   78:	7f 86 e3 78 	mr      r6,r28
   7c:	7f a3 eb 78 	mr      r3,r29
   80:	7f e5 fb 78 	mr      r5,r31
   84:	48 00 00 01 	bl      84 <copy_oldmem_page+0x84>
			84: R_PPC_REL24	copy_to_user_or_kernel
   88:	3b c0 ff f2 	li      r30,-14
   8c:	2c 03 00 00 	cmpwi   r3,0
   90:	40 82 00 08 	bne     98 <copy_oldmem_page+0x98>
   94:	7f fe fb 78 	mr      r30,r31
   98:	80 01 00 24 	lwz     r0,36(r1)
   9c:	83 61 00 0c 	lwz     r27,12(r1)
   a0:	83 81 00 10 	lwz     r28,16(r1)
   a4:	7c 08 03 a6 	mtlr    r0
   a8:	83 a1 00 14 	lwz     r29,20(r1)
   ac:	7f c3 f3 78 	mr      r3,r30
   b0:	83 e1 00 1c 	lwz     r31,28(r1)
   b4:	83 c1 00 18 	lwz     r30,24(r1)
   b8:	38 21 00 20 	addi    r1,r1,32
   bc:	4e 80 00 20 	blr
   c0:	7f c3 f3 78 	mr      r3,r30
   c4:	93 41 00 08 	stw     r26,8(r1)
   c8:	38 a0 05 91 	li      r5,1425
   cc:	38 80 10 00 	li      r4,4096
   d0:	48 00 00 01 	bl      d0 <copy_oldmem_page+0xd0>
			d0: R_PPC_REL24	ioremap_prot
   d4:	7f 86 e3 78 	mr      r6,r28
   d8:	7c 83 da 14 	add     r4,r3,r27
   dc:	7c 7a 1b 78 	mr      r26,r3
   e0:	7f e5 fb 78 	mr      r5,r31
   e4:	7f a3 eb 78 	mr      r3,r29
   e8:	48 00 00 01 	bl      e8 <copy_oldmem_page+0xe8>
			e8: R_PPC_REL24	copy_to_user_or_kernel
   ec:	3b c0 ff f2 	li      r30,-14
   f0:	2c 03 00 00 	cmpwi   r3,0
   f4:	40 82 00 08 	bne     fc <copy_oldmem_page+0xfc>
   f8:	7f fe fb 78 	mr      r30,r31
   fc:	7f 43 d3 78 	mr      r3,r26
  100:	48 00 00 01 	bl      100 <copy_oldmem_page+0x100>
			100: R_PPC_REL24	iounmap
  104:	80 01 00 24 	lwz     r0,36(r1)
  108:	83 41 00 08 	lwz     r26,8(r1)
  10c:	83 61 00 0c 	lwz     r27,12(r1)
  110:	7c 08 03 a6 	mtlr    r0
  114:	83 81 00 10 	lwz     r28,16(r1)
  118:	83 a1 00 14 	lwz     r29,20(r1)
  11c:	4b ff ff 90 	b       ac <copy_oldmem_page+0xac>


Christophe