[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