[Skiboot] [PATCH 05/15] libc/string: Add memcpy_ci

[Claudio Carvalho]

On 09/01/2016 06:34 AM, Stewart Smith wrote:
> Claudio Carvalho <cclaudio at linux.vnet.ibm.com> writes:
>> --- /dev/null
>> +++ b/libc/string/memcpy_ci.c
>> @@ -0,0 +1,50 @@
>> +/* Copyright 2013-2016 IBM Corp
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at
>> + *
>> + *      http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or
>> + * implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <string.h>
>> +#include <io.h>
>> +
>> +const void* memcpy_ci(const void *destpp, const void *srcpp, size_t len)
>> +{
>> +	size_t i;
>> +	const size_t block = sizeof(uint32_t);
>> +
>> +	unsigned long int destp = (long int) destpp;
>> +	unsigned long int srcp = (long int) srcpp;
> 
> Here you're casting away const which isn't good.
> 
> The const for destpp is incorrect as what destpp points to certainly
> isn't untouched by this function.
> 
> Same with return value, shouldn't be const.
> 

Right. I will apply this in V2:
- remove const from the destpp parameter
- remove const from the return value
- update the memcpy_ci callers

>> +
>> +	/* copy just a few bytes to make dst aligned */
>> +	i = len % block;
>> +	while (i > 0)
>> +	{
>> +		*((uint8_t*) destp) = in_8((uint8_t*)srcp);
>> +		srcp += 1;
>> +		destp += 1;
>> +		i--;
>> +	}
>> +
>> +	len -= len % block;
>> +
>> +	/* copy the aligned bytes */
>> +	while(len > 0)
>> +	{
>> +		*((uint32_t*) destp) = in_be32((uint32_t*)srcp);
>> +		srcp += block;
>> +		destp += block;
>> +		len -= block;
>> +	}
>> +
>> +	return destpp;
>> +}
> 
> I think it would be good to add a unit test for this function.
> 

Makes sense. I will figure out how to add a unit test in skiboot for
this function.


> It also (currently) assumes that src is a multiple of uint32_t, although
> nothing checks that and so would silently fail if called for, say 33 bytes.
> 

Not sure if I understand your suggestion here.

This function doesn't check if srcpp and dstpp are multiple of uint32_t,
but it assumes that the processor is able to read a char and a uint32_t
from src. Thus, the first loop copies at most len % sizeof(uint32_t) to
optimize the number of reads