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

[Balbir Singh]

On 02/09/16 23:02, Balbir Singh wrote:
> 
> 
> On 11/08/16 15:23, Claudio Carvalho wrote:
>> This adds memcpy_ci, it's a cache inhibited version of memcpy.
>>
>> We use memcpy_ci to copy the verification code from the secure ROM
>> to memory. Due to performance issues we opted to run it from memory.
>>
>> Signed-off-by: Claudio Carvalho <cclaudio at linux.vnet.ibm.com>
>> ---
>>  libc/include/string.h    |  3 ++-
>>  libc/string/Makefile.inc |  9 +++++----
>>  libc/string/memcpy_ci.c  | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 57 insertions(+), 5 deletions(-)
>>  create mode 100644 libc/string/memcpy_ci.c
>>
>> diff --git a/libc/include/string.h b/libc/include/string.h
>> index 890ffc2..a53e4f4 100644
>> --- a/libc/include/string.h
>> +++ b/libc/include/string.h
>> @@ -1,5 +1,5 @@
>>  /******************************************************************************
>> - * Copyright (c) 2004, 2008 IBM Corporation
>> + * Copyright (c) 2004, 2016 IBM Corporation
>>   * All rights reserved.
>>   * This program and the accompanying materials
>>   * are made available under the terms of the BSD License
>> @@ -32,6 +32,7 @@ char *strdup(const char *src);
>>  void *memset(void *s, int c, size_t n);
>>  void *memchr(const void *s, int c, size_t n);
>>  void *memcpy(void *dest, const void *src, size_t n);
>> +const void *memcpy_ci(const void *destpp, const void *srcpp, size_t len);
>>  void *memmove(void *dest, const void *src, size_t n);
>>  int memcmp(const void *s1, const void *s2, size_t n);
>>  
>> diff --git a/libc/string/Makefile.inc b/libc/string/Makefile.inc
>> index 3b7c8ce..5f7fa27 100644
>> --- a/libc/string/Makefile.inc
>> +++ b/libc/string/Makefile.inc
>> @@ -1,5 +1,5 @@
>>  # *****************************************************************************
>> -# * Copyright (c) 2004, 2008 IBM Corporation
>> +# * Copyright (c) 2004, 2016 IBM Corporation
>>  # * All rights reserved.
>>  # * This program and the accompanying materials
>>  # * are made available under the terms of the BSD License
>> @@ -12,9 +12,10 @@
>>  
>>  SUBDIRS += $(LIBCDIR)/string
>>  
>> -STRING_OBJS = strcat.o strchr.o strcmp.o strcpy.o strlen.o strncmp.o \
>> -	      strncpy.o strstr.o memset.o memcpy.o memmove.o memchr.o \
>> -	      memcmp.o strcasecmp.o strncasecmp.o strtok.o strdup.o
>> +STRING_OBJS = strcat.o strchr.o strcmp.o strcpy.o strlen.o \
>> +	      strncmp.o strncpy.o strstr.o memset.o memcpy.o memcpy_ci.o \
>> +	      memmove.o memchr.o memcmp.o strcasecmp.o strncasecmp.o \
>> +	      strtok.o strdup.o
>>  STRING = $(LIBCDIR)/string/built-in.o
>>  $(STRING): $(STRING_OBJS:%=$(LIBCDIR)/string/%)
>>  
>> diff --git a/libc/string/memcpy_ci.c b/libc/string/memcpy_ci.c
>> new file mode 100644
>> index 0000000..9af45dc
>> --- /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)
> 
> Why isn't len also a const?

Ignore this comment, I missed the len modifications below

> 
>> +{
>> +	size_t i;
>> +	const size_t block = sizeof(uint32_t);
> 
> Why not 64 bit alignment?
> 
>> +
>> +	unsigned long int destp = (long int) destpp;
>> +	unsigned long int srcp = (long int) srcpp;
>> +
>> +	/* copy just a few bytes to make dst aligned */
>> +	i = len % block;
> 
> 
> I would do some basic sanity tests around len, like
> 
> is len > RAM size?
> 
>> +	while (i > 0)
>> +	{
>> +		*((uint8_t*) destp) = in_8((uint8_t*)srcp);
>> +		srcp += 1;
>> +		destp += 1;
>> +		i--;
>> +	}
>> +
> 
> 
> Why not align to an 8 byte boundary and use in_be64 instead?
> 
>> +	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;
>> +}
>>