[Skiboot] [PATCH V2 2/7] libflash/blocklevel: Extend the block level to be able to do ecc
Cyril Bur
cyril.bur at au1.ibm.com
Tue Jun 23 09:23:15 AEST 2015
On Mon, 22 Jun 2015 19:10:41 +1000
Alistair Popple <alistair at popple.id.au> wrote:
> 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 :-)
>
Hi Stewart ^ :)
I'll send a v3.
> 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?
>
Probably...
> > + 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?
>
Ah, so my thought here was (and please point our where the code
contradicts what I'm about to say), that it would return real errno
values in errno, and libflash.h errors as a positive return value. It
is probably overkill but it does pass back the most information.
> > + }
> > +
> > + 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)...
Yep
>
> > + }
> >
> > 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...
Yeah, I decided though that to avoid potential stuffups it will only
merge ranged that are exactly adjacent and where the new range is
entirely unprotected. Yes it does simplify my life here but also
because the amount of corner cases if you can add with overlaping
ranges gets super messy. In fact, even this way I just realised theres
a possibility that I missed, will fix.
>
> > + 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.
>
Sure.
> > };
> >
> > 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