[Skiboot] [PATCH 2/7] libflash/blocklevel: Extend the block level to be able to do ecc
Cyril Bur
cyrilbur at gmail.com
Tue Jun 16 17:31:24 AEST 2015
On Fri, 12 Jun 2015 12:06:03 +1000
Alistair Popple <alistair at popple.id.au> wrote:
> Cyril,
>
> Comments below. Thanks.
>
> - Alistair
>
> On Fri, 5 Jun 2015 14:11:26 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 | 150
> > ++++++++++++++++++++++++++++++++++++++++++++++++--
> > libflash/blocklevel.h | 19 +++++++ 3 files changed, 167
> > insertions(+), 4 deletions(-)
> >
> > 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..c3ca95e 100644
> > --- a/libflash/blocklevel.c
> > +++ b/libflash/blocklevel.c
> > @@ -14,16 +14,76 @@
> > * 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"
> > +
> > +static int ecc_protected(struct blocklevel_device *bl, uint32_t
> > pos, uint32_t len) +{
> > + int i;
> > +
> > + for (i = 0; i < bl->n_ecc_prot; i++) {
> > + 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))
>
> It took me a while but this looks correct. This would be a really good
> function to write a test case for though.
>
Yep, Added, in the process rewrote all of this though. Sorry.
> > + return 1;
> > + }
> > + return 0;
> > +}
> > +
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wunused-parameter"
>
> Why not use __unused? Given these are just stub functions they should
> be removed or the actual functionality implemented. They can always
> be added in a patch series to implement read/write protections.
>
> > +static int read_protected(struct blocklevel_device *bl, uint32_t
> > pos, uint32_t len) +{
> > + return 0;
> > +}
> > +
> > +static int write_protected(struct blocklevel_device *bl, uint32_t
> > pos, uint32_t len) +{
> > + return 0;
> > +}
> > +#pragma GCC diagnostic pop
> >
> > int blocklevel_read(struct blocklevel_device *bl, uint32_t pos,
> > void *buf, uint32_t len) {
> > if (!bl || !bl->read || !buf)
> > - return -1;
> > + return -EPERM;
> > +
> > + if (read_protected(bl, pos, len))
> > + return -EACCES;
> > +
> > + 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)
> > + return -ENOMEM;
> > +
> > + /*
> > + * Some backends may pass back information with
> > errno, try to
> > + * capture that
> > + */
>
> These are library like functions so arguably we could just use errno
> to return errors, including the ones above. But it's not really
> necessary so happy to stick with returning the errno directly.
>
I like that if say we are a library and we can use errno, error
values defined in libflash.h can be returned nicely and errno can
supplement the information.
> > + errno = 0;
> > + rc = bl->read(bl, pos, buffer, ecc_len);
> > + if (rc) {
> > + rc = errno ? errno : rc;
> > + goto out;
> > + }
> > +
> > + rc = memcpy_from_ecc(buf, buffer, len);
> > +
> > + out:
> > + free(buffer);
> > + return rc;
> > + }
> >
> > return bl->read(bl, pos, buf, len);
> > }
> > @@ -31,7 +91,31 @@ int blocklevel_read(struct blocklevel_device
> > *bl, uint32_t pos, void *buf, uint3 int blocklevel_write(struct
> > blocklevel_device *bl, uint32_t pos, const void *buf, uint32_t len)
> > { if (!bl || !bl->write || !buf)
> > - return -1;
> > + return -EPERM;
> > +
> > + if (write_protected(bl, pos, len))
> > + return -EACCES;
> > +
> > + 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)
> > + return -ENOMEM;
> > +
> > + if (memcpy_to_ecc(buffer, buf, len)) {
> > + rc = -EBADF;
> > + goto out;
> > + }
> > +
> > + rc = bl->write(bl, pos, buffer, ecc_len);
>
> Do backends return things in errno if this fails? We should at least
> be consistent with blocklevel_read.
Consistency is best.
>
> > + out:
> > + free(buffer);
> > + return rc;
> > + }
> >
> > return bl->write(bl, pos, buf, len);
> > }
> > @@ -39,7 +123,10 @@ 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;
> > +
> > + if (write_protected(bl, pos, len))
> > + return -EACCES;
> >
> > /* 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,58 @@ 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,
> > + struct bl_prot_range range)
> > +{
> > + struct bl_prot_range *new_ranges;
> > +
> > + new_ranges = realloc(*ranges, sizeof(range) * ((*n_ranges)
> > + 1));
>
> Frequent reallocs can fragment the heap, especially in skiboot where
> our allocator is not so smart. I wonder if there is someway the ffs
> layer could provide a hint to the total number of ranges needed? It
> knows the total number of partitions and the number of protection
> ranges is unlikely to exceed that.
A simple way could be to have a bulk add function but it's not the best
solution.
There's no reason this code can't preallocate for it's self so as to
reduce the number of reallocs. And if that preallocation happens to
match the amount of ffs partitions... wouldn't that be useful...
>
> > + 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 };
> > + return insert_bl_prot_range(&bl->ecc_prot,
> > &bl->n_ecc_prot, range); +}
> > +
> > +#pragma GCC diagnostic push
> > +#pragma GCC diagnostic ignored "-Wunused-parameter"
>
> __unused?
>
> > +#pragma GCC diagnostic ignored "-Wsuggest-attribute=const"
>
> At the very least you need a comment to explain why the compiler is
> wrong before wilfully ignoring its good advice :)
>
> > +int blocklevel_read_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
> > + */
> > +
> > + /* TODO */
> > + return -ENOSYS;
> > +}
> > +
> > +int blocklevel_write_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
> > + */
> > +
> > + /* TODO */
> > + return -ENOSYS;
> > +}
>
> Just remove these and leave them for a latter series were we implement
> read/write protection.
Yeah I'll keep all that in a local branch as a reminder...
>
> > +#pragma GCC diagnostic pop
> > diff --git a/libflash/blocklevel.h b/libflash/blocklevel.h
> > index a22ecb4..3a20677 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,15 @@ 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;
> > +
> > + struct bl_prot_range *read_prot;
> > + int n_read_prot;
> > +
> > + struct bl_prot_range *write_prot;
> > + int n_write_prot;
> > };
> >
> > int blocklevel_read(struct blocklevel_device *bl, uint32_t pos,
> > void *buf, uint32_t len); @@ -42,4 +56,9 @@ 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); +int blocklevel_read_protect(struct
> > blocklevel_device *bl, uint32_t start, uint32_t len); +int
> > blocklevel_write_protect(struct blocklevel_device *bl, uint32_t
> > start, uint32_t len); + #endif /* __LIBFLASH_BLOCKLEVEL_H */
> >
>
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot
More information about the Skiboot
mailing list