[Skiboot] [PATCH V2 2/7] libflash/blocklevel: Extend the block level to be able to do ecc

Alistair Popple alistair at popple.id.au
Mon Jun 22 19:10:41 AEST 2015


Looks pretty good. Couple of very minor code style comments which are mostly
a matter of preference and a few questions which I feel like we've already
discussed off-line so probably not an issue.

Also test cases are good ... I'm reasonably sure Stewart pays beer for test
cases :-)

Reviewed-By: Alistair Popple <alistair at popple.id.au>

On Tue, 16 Jun 2015 19:01:05 Cyril Bur wrote:
> At the moment ECC reads and writes are still being handled by the low level
> core of libflash. ECC should be none of libflashes concern, it is primarily
> a hardware access backend.
> 
> It makes sense for blocklevel to take care of ECC but currently it has no
> way of knowing. With some simple modifications (which are rudimentary at
> the moment and will need a performance improvement) blocklevel can handle
> ECC, and with a little more effort this can be extended to provide read and
> write protection in blocklevel.
> 
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
>  libc/include/errno.h            |   2 +
>  libflash/blocklevel.c           | 152 +++++++++++++++++++++++++++++++++++++--
>  libflash/blocklevel.h           |  12 ++++
>  libflash/test/Makefile.check    |   4 +-
>  libflash/test/test-blocklevel.c | 153 ++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 315 insertions(+), 8 deletions(-)
>  create mode 100644 libflash/test/test-blocklevel.c
> 
> diff --git a/libc/include/errno.h b/libc/include/errno.h
> index d585934..c2bd987 100644
> --- a/libc/include/errno.h
> +++ b/libc/include/errno.h
> @@ -21,6 +21,7 @@ extern int errno;
>  #define EPERM		1	/* not permitted */
>  #define ENOENT		2	/* file or directory not found */
>  #define EIO		5	/* input/output error */
> +#define EBADF        9  /* Bad file number */
>  #define ENOMEM		12	/* not enough space */
>  #define EACCES		13	/* permission denied */
>  #define EFAULT		14	/* bad address */
> @@ -30,5 +31,6 @@ extern int errno;
>  #define EINVAL		22	/* invalid argument */
>  #define EDOM		33	/* math argument out of domain of func */
>  #define ERANGE		34	/* math result not representable */
> +#define ENOSYS      38  /* Function not implemented */
>  
>  #endif
> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
> index bc03b9b..8df9c3d 100644
> --- a/libflash/blocklevel.c
> +++ b/libflash/blocklevel.c
> @@ -14,24 +14,111 @@
>   * limitations under the License.
>   */
>  
> +#include <stdlib.h>
>  #include <unistd.h>
>  #include <stdio.h>
> +#include <errno.h>
> +#include <string.h>
>  
>  #include <libflash/libflash.h>
> +
>  #include "blocklevel.h"
> +#include "ecc.h"
> +
> +#define PROT_REALLOC_NUM 25
> +
> +/* This function returns tristate values.
> + * 1  - The region is ECC protected
> + * 0  - The region is not ECC protected
> + * -1 - Partially protected
> + */
> +static int ecc_protected(struct blocklevel_device *bl, uint32_t pos, uint32_t len)
> +{
> +	int i;
> +
> +	/* Length of 0 is nonsensical so add 1 */
> +	if (len == 0)
> +		len = 1;
> +
> +	for (i = 0; i < bl->n_ecc_prot; i++) {
> +		/* Fits entirely within the range */
> +		if (bl->ecc_prot[i].start <= pos && bl->ecc_prot[i].start + bl->ecc_prot[i].len >= pos + len)
> +			return 1;
> +
> +		/*
> +		 * Since we merge regions on inserting we can be sure that a
> +		 * partial fit means that the non fitting region won't fit in another ecc
> +		 * region
> +		 */
> +		if ((bl->ecc_prot[i].start >= pos && bl->ecc_prot[i].start < pos + len) ||
> +		   (bl->ecc_prot[i].start <= pos && bl->ecc_prot[i].start + bl->ecc_prot[i].len > pos))
> +			return -1;
> +	}
> +	return 0;
> +}
>  
>  int blocklevel_read(struct blocklevel_device *bl, uint32_t pos, void *buf, uint32_t len)
>  {
> -	if (!bl || !bl->read || !buf)
> -		return -1;
> +	if (!bl || !bl->read || !buf) {
> +		errno = EPERM;

A bit pedantic but EINVAL might make more sense?

> +		return FLASH_ERR_PARM_ERROR;
> +	}
> +
> +	if (ecc_protected(bl, pos, len)) {
> +		int rc;
> +		struct ecc64 *buffer;
> +		uint32_t ecc_len = ecc_buffer_size(len);
> +
> +		buffer = malloc(ecc_len);
> +		if (!buffer) {
> +			errno = ENOMEM;
> +			return FLASH_ERR_MALLOC_FAILED;

You're kind of doubling up on error numbers here. Given
FLASH_ERR_MALLOC_FAILED is defined in libflash (which we shouldn't need to
run this code) maybe it would be better if you just defined all blocklevel
functions to return -1 on error with more information in errno?

> +		}
> +
> +		rc = bl->read(bl, pos, buffer, ecc_len);
> +		if (rc)
> +			goto out;
> +
> +		rc = memcpy_from_ecc(buf, buffer, len);
> +
> +	out:
> +		free(buffer);
> +		return rc;

The "convention" is that this would go at the end of the function. I can see
why you put it here but if you're redoing the patch would you mind putting it
at the end? Perhaps put the "bl->read" in the else clause of the if(eec_protect)...

> +	}
>  
>  	return bl->read(bl, pos, buf, len);
>  }
>  
>  int blocklevel_write(struct blocklevel_device *bl, uint32_t pos, const void *buf, uint32_t len)
>  {
> -	if (!bl || !bl->write || !buf)
> -		return -1;
> +	if (!bl || !bl->write || !buf) {
> +		errno = EPERM;
> +		return FLASH_ERR_PARM_ERROR;
> +	}
> +
> +	if (ecc_protected(bl, pos, len)) {
> +		int rc;
> +		struct ecc64 *buffer;
> +		uint32_t ecc_len = ecc_buffer_size(len);
> +
> +		buffer = malloc(ecc_len);
> +		if (!buffer) {
> +			errno = ENOMEM;
> +			return FLASH_ERR_MALLOC_FAILED;
> +		}
> +
> +		if (memcpy_to_ecc(buffer, buf, len)) {
> +			errno = EBADF;
> +			rc = FLASH_ERR_ECC_INVALID;
> +			goto out;
> +		}
> +
> +		rc = bl->write(bl, pos, buffer, ecc_len);
> +
> +	out:
> +		free(buffer);
> +		return rc;
> +	}
>  
>  	return bl->write(bl, pos, buf, len);
>  }
> @@ -39,7 +126,7 @@ int blocklevel_write(struct blocklevel_device *bl, uint32_t pos, const void *buf
>  int blocklevel_erase(struct blocklevel_device *bl, uint32_t pos, uint32_t len)
>  {
>  	if (!bl || !bl->erase)
> -		return -1;
> +		return -EPERM;
>  
>  	/* Programmer may be making a horrible mistake without knowing it */
>  	if (len & bl->erase_mask) {
> @@ -57,7 +144,7 @@ int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint32_
>  	int rc;
>  
>  	if (!bl || !bl->get_info)
> -		return -1;
> +		return -EPERM;
>  
>  	rc = bl->get_info(bl, name, total_size, erase_granule);
>  
> @@ -68,3 +155,56 @@ int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint32_
>  
>  	return rc;
>  }
> +
> +static int insert_bl_prot_range(struct bl_prot_range **ranges, int *n_ranges,
> +		int *total_n_ranges, struct bl_prot_range range)
> +{
> +	struct bl_prot_range *new_ranges;
> +	struct bl_prot_range *old_ranges = *ranges;
> +	int i, count = *n_ranges;
> +
> +	/* Try to merge into an existing range */
> +	for (i = 0; i < count; i++) {
> +		if (!(range.start + range.len == old_ranges[i].start || old_ranges[i].start + old_ranges[i].len == range.start))
> +			continue;
> +
> +		if (range.start + range.len == old_ranges[i].start)
> +			old_ranges[i].start = range.start;
> +
> +		old_ranges[i].len += range.len;
> +		return 0;
> +	}

This looks ok at first glance, but I'll assume there's test cases to check the dusty corner cases here...

> +	if (*n_ranges == *total_n_ranges) {
> +		new_ranges = realloc(*ranges, sizeof(range) * ((*n_ranges) + PROT_REALLOC_NUM));
> +		if (new_ranges)
> +			(*total_n_ranges) += PROT_REALLOC_NUM;
> +	} else {
> +		new_ranges = old_ranges;
> +	}
> +	if (new_ranges) {
> +		memcpy(new_ranges + *n_ranges, &range, sizeof(range));
> +		*ranges = new_ranges;
> +		(*n_ranges)++;
> +	}
> +
> +	return !new_ranges;
> +}
> +
> +int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_t len)
> +{
> +	/*
> +	 * Could implement this at hardware level by having an accessor to the
> +	 * backend in struct blocklevel_device and as a result do nothing at
> +	 * this level (although probably not for ecc!)
> +	 */
> +	struct bl_prot_range range = { .start = start, .len = len };
> +
> +	/*
> +	 * Refuse to add regions that are already protected or are partially
> +	 * protected
> +	 */

I vaguely remember talking about this but now that you're merging ranges does
it matter if they overlap? Shouldn't it just extend the range? Although I have
the distinct feeling that captain obvious could come to my rescue and point out
what I'm missing here...

> +	if (len < BYTES_PER_ECC || ecc_protected(bl, start, len))
> +		return -1;
> +	return insert_bl_prot_range(&bl->ecc_prot, &bl->n_ecc_prot, &bl->total_ecc_prot, range);
> +}
> diff --git a/libflash/blocklevel.h b/libflash/blocklevel.h
> index a22ecb4..fac05cd 100644
> --- a/libflash/blocklevel.h
> +++ b/libflash/blocklevel.h
> @@ -18,6 +18,11 @@
>  
>  #include <stdint.h>
>  
> +struct bl_prot_range {
> +	uint32_t start;
> +	uint32_t len;
> +};
> +
>  /*
>   * libffs may be used with different backends, all should provide these for
>   * libflash to get the information it needs
> @@ -34,6 +39,10 @@ struct blocklevel_device {
>  	 * Keep the erase mask so that blocklevel_erase() can do sanity checking
>  	 */
>  	uint32_t erase_mask;
> +
> +	struct bl_prot_range *ecc_prot;
> +	int n_ecc_prot;
> +	int total_ecc_prot;

I'm going to nit pick again here but it would be neater if you defined
something like this:

struct ranges {
	struct bl_prot_range *prot;
	int n_prot;
	int total_prot;
};

Then you could pass one parameter to insert_bl_prot_range(...) instead of three.
It might also make it a bit nicer when implementing read/write protection.

>  };
>  
>  int blocklevel_read(struct blocklevel_device *bl, uint32_t pos, void *buf, uint32_t len);
> @@ -42,4 +51,7 @@ int blocklevel_erase(struct blocklevel_device *bl, uint32_t pos, uint32_t len);
>  int blocklevel_get_info(struct blocklevel_device *bl, const char **name, uint32_t *total_size,
>  		uint32_t *erase_granule);
>  
> +/* Implemented in software at this level */
> +int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_t len);
> +
>  #endif /* __LIBFLASH_BLOCKLEVEL_H */
> diff --git a/libflash/test/Makefile.check b/libflash/test/Makefile.check
> index a852f29..05453b2 100644
> --- a/libflash/test/Makefile.check
> +++ b/libflash/test/Makefile.check
> @@ -1,5 +1,5 @@
>  # -*-Makefile-*-
> -LIBFLASH_TEST := libflash/test/test-flash libflash/test/test-ecc
> +LIBFLASH_TEST := libflash/test/test-flash libflash/test/test-ecc libflash/test/test-blocklevel
>  
>  LCOV_EXCLUDE += $(LIBFLASH_TEST:%=%.c)
>  
> @@ -16,7 +16,7 @@ $(LIBFLASH_TEST:%=%-check) : %-check: %
>  libflash/test/stubs.o: libflash/test/stubs.c
>  	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) -g -c -o $@ $<, $<)
>  
> -$(LIBFLASH_TEST) : libflash/test/stubs.o libflash/libflash.c libflash/ecc.c
> +$(LIBFLASH_TEST) : libflash/test/stubs.o libflash/libflash.c libflash/ecc.c libflash/blocklevel.c
>  
>  $(LIBFLASH_TEST) : % : %.c
>  	$(call Q, HOSTCC ,$(HOSTCC) $(HOSTCFLAGS) -O0 -g -I include -I . -o $@ $< libflash/test/stubs.o, $<)
> diff --git a/libflash/test/test-blocklevel.c b/libflash/test/test-blocklevel.c
> new file mode 100644
> index 0000000..d4af1ae
> --- /dev/null
> +++ b/libflash/test/test-blocklevel.c
> @@ -0,0 +1,153 @@
> +/* Copyright 2013-2014 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 <stdio.h>
> +#include <stdlib.h>
> +#include <stdint.h>
> +#include <string.h>
> +
> +#include <libflash/blocklevel.h>
> +
> +#include "../ecc.c"
> +#include "../blocklevel.c"
> +
> +#define __unused		__attribute__((unused))
> +
> +#define ERR(fmt...) fprintf(stderr, fmt)
> +
> +int main(void)
> +{
> +	struct blocklevel_device bl_mem = { 0 };
> +	struct blocklevel_device *bl = &bl_mem;
> +
> +	if (blocklevel_ecc_protect(bl, 0, 0x1000)) {
> +		ERR("Failed to blocklevel_ecc_protect!\n");
> +		return 1;
> +	}
> +
> +	/* 0x1000 -> 0x3000 should remain unprotected */
> +
> +	if (blocklevel_ecc_protect(bl, 0x3000, 0x1000)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x3000, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	/* Zero length protection */
> +	if (!blocklevel_ecc_protect(bl, 0x4000, 0)) {
> +		ERR("Shouldn't have succeeded blocklevel_ecc_protect(0x4000, 0)\n");
> +		return 1;
> +	}
> +
> +	/* Minimum creatable size */
> +	if (blocklevel_ecc_protect(bl, 0x4000, BYTES_PER_ECC)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x4000, BYTES_PER_ECC)\n");
> +		return 1;
> +	}
> +
> +	/* Deal with overlapping protections */
> +	if (!blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> +		ERR("Shouldn't have succeeded blocklevel_ecc_protect(0x100, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	/* Deal with protections greater than max size */
> +	if (!blocklevel_ecc_protect(bl, 0, 0xFFFFFFFF)) {
> +		ERR("Failed to blocklevel_ecc_protect(0, 0xFFFFFFFF)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0, 1) != 1) {
> +		ERR("Invaid result for ecc_protected(0, 1)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0, 0x1000) != 1) {
> +		ERR("Invalid result for ecc_protected(0, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x100, 0x100) != 1) {
> +		ERR("Invalid result for ecc_protected(0x0100, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x1000, 0) != 0) {
> +		ERR("Invalid result for ecc_protected(0x1000, 0)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x1000, 0x1000) != 0) {
> +		ERR("Invalid result for ecc_protected(0x1000, 0x1000)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x1000, 0x100) != 0) {
> +		ERR("Invalid result for ecc_protected(0x1000, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x2000, 0) != 0) {
> +		ERR("Invalid result for ecc_protected(0x2000, 0)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x4000, 1) != 1) {
> +		ERR("Invalid result for ecc_protected(0x4000, 1)\n");
> +		return 1;
> +	}
> +
> +	/* Check for asking for a region with mixed protection */
> +	if (ecc_protected(bl, 0x100, 0x2000) != -1) {
> +		ERR("Invalid result for ecc_protected(0x100, 0x2000)\n");
> +		return 1;
> +	}
> +
> +	/* Test the auto extending of regions */
> +	if (blocklevel_ecc_protect(bl, 0x5000, 0x100)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x5000, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (blocklevel_ecc_protect(bl, 0x5100, 0x100)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x5100, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (blocklevel_ecc_protect(bl, 0x5200, 0x100)) {
> +		ERR("Failed to blocklevel_ecc_protect(0x5200, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x5120, 0x10) != 1) {
> +		ERR("Invalid result for ecc_protected(0x5120, 0x10)\n");
> +		return 1;
> +	}
> +
> +	if (blocklevel_ecc_protect(bl, 0x4900, 0x100)) {
> +		ERR("Failed to blocklevel_ecc_protected(0x4900, 0x100)\n");
> +		return 1;
> +	}
> +
> +	if (ecc_protected(bl, 0x4920, 0x10) != 1) {
> +		ERR("Invalid result for ecc_protected(0x4920, 0x10)\n");
> +		return 1;
> +	}
> +
> +	if (!blocklevel_ecc_protect(bl, 0x5290, 0x10)) {
> +		ERR("Shouldn't have been able to blocklevel_ecc_protect(0x5290, 0x10)\n");
> +		return 1;
> +	}
> +	return 0;
> +}
> 



More information about the Skiboot mailing list