Re: [Qemu-devel] [RFC PATCH v2 1/8] pc-bios/s390-ccw: Move libc functions to separate header

[Thomas Huth]

On 07.07.2017 15:46, Christian Borntraeger wrote:
> On 07/07/2017 12:26 PM, Thomas Huth wrote:
>> The upcoming netboot code will use the libc from SLOF. To be able
>> to still use s390-ccw.h there, the libc related functions in this
>> header have to be moved to a different location.
>> And while we're at it, remove the duplicate memcpy() function from
>> sclp.c.
>>
>> Signed-off-by: Thomas Huth <address@hidden>
> 
> one suggestion below, but 
> 
> Reviewed-by: Christian Borntraeger <address@hidden>
> 
> for change and no change
> 
> [...]
>> +static inline void *memcpy(void *s1, const void *s2, size_t n)
>> +{
>> +    uint8_t *p1 = s1;
>> +    const uint8_t *p2 = s2;
>> +
>> +    while (n--) {
>> +        p1[n] = p2[n];
>> +    }
>> +    return s1;
>> +}
> 
> Not that it matters, and no idea if moving forward like in the for loop below
> might allow gcc to generate better code, but a for loop like below will be 
> the 
> same direction as the MVC instruction. Can you double check if this generates
> better code?

At least with my compiler version (old 4.8.1), the code looks pretty
similar. Backward loop:

 1d8:   a7 19 04 d1             lghi    %r1,1233
 1dc:   e3 40 50 00 00 04       lg      %r4,0(%r5)
 1e2:   e3 30 70 00 00 04       lg      %r3,0(%r7)
 1e8:   a7 29 04 d2             lghi    %r2,1234
 1ec:   43 01 30 00             ic      %r0,0(%r1,%r3)
 1f0:   a7 1b ff ff             aghi    %r1,-1
 1f4:   42 01 40 01             stc     %r0,1(%r1,%r4)
 1f8:   a7 27 ff fa             brctg   %r2,1ec <main+0x1ec>

Forward loop:

 238:   a7 19 00 00             lghi    %r1,0
 23c:   e3 40 50 00 00 04       lg      %r4,0(%r5)
 242:   e3 30 70 00 00 04       lg      %r3,0(%r7)
 248:   a7 29 04 d2             lghi    %r2,1234
 24c:   43 01 30 00             ic      %r0,0(%r1,%r3)
 250:   42 01 40 00             stc     %r0,0(%r1,%r4)
 254:   a7 1b 00 01             aghi    %r1,1
 258:   a7 27 ff fa             brctg   %r2,24c <main+0x24c>

But I can switch to the for-loop version in my patch anyway, if you
prefer that.

 Thomas