[Skiboot] [PATCH 2/7] libflash/blocklevel: Extend the block level to be able to do ecc
Alistair Popple
alistair at popple.id.au
Fri Jun 12 12:06:03 AEST 2015
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.
> + 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.
> + 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.
> + 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.
> + 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.
> +#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 */
>
More information about the Skiboot
mailing list