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

[Balbir Singh]

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?

> +{
> +	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;
> +}
>