[Skiboot] [PATCH v2] libflash/blocklevel: Allow double ecc protecting a region
Cyril Bur
cyril.bur at au1.ibm.com
Tue Jul 26 11:45:56 AEST 2016
On Fri, 15 Jul 2016 15:34:04 +1000
Cyril Bur <cyril.bur at au1.ibm.com> wrote:
Please disregard, I'll resend it as part of a series to get all of the
blocklevel for skiboot working.
> Currently the policy for calling ECC protecting a range at the
> blocklevel layer is that the requested region be completely
> unprotected otherwise the call will return an error. It turns out that
> duplicate calls to ffs_init() with true as the last parameter (for the
> same blocklevel structure) will cause duplicate attempts to
> ecc_protect() ranges.
>
> Change the policy within blocklevel to allow duplicate protecting.
> In fact the new policy almost guarantees no failure (baring something
> odd like malloc() failing). It will detect that the range is currently
> already fully protected and do nothing, detect that part of the range
> is or is not and extend the existing range or detect that a range fits
> perfectly between two ranges in which case it will merge the ranges.
>
> Also adjust tests to match the new policy.
>
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
> V2: Significant rewrite, v1 was full of holes. This one should be
> better
> Fixup the tests
>
>
> libflash/blocklevel.c | 114 ++++++++++++++++++++++++++--------------
> libflash/libffs.c | 2 +-
> libflash/test/test-blocklevel.c | 54 ++++++++++++++-----
> 3 files changed, 119 insertions(+), 51 deletions(-)
>
> diff --git a/libflash/blocklevel.c b/libflash/blocklevel.c
> index 9591194..cbb9937 100644
> --- a/libflash/blocklevel.c
> +++ b/libflash/blocklevel.c
> @@ -327,53 +327,95 @@ out_free:
> return rc;
> }
>
> -static int insert_bl_prot_range(struct blocklevel_range *ranges, struct bl_prot_range range)
> +static bool insert_bl_prot_range(struct blocklevel_range *ranges, struct bl_prot_range range)
> {
> - struct bl_prot_range *new_ranges;
> - struct bl_prot_range *old_ranges = ranges->prot;
> - int i, count = ranges->n_prot;
> + int i;
> + uint32_t pos, len;
> + struct bl_prot_range *prot = ranges->prot;
>
> - /* 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;
> + pos = range.start;
> + len = range.len;
>
> - if (range.start + range.len == old_ranges[i].start)
> - old_ranges[i].start = range.start;
> + if (len == 0)
> + return true;
>
> - old_ranges[i].len += range.len;
> + /* Check for overflow */
> + if (pos + len < len)
> + return false;
>
> - /*
> - * Check the inserted range isn't wedged between two ranges, if it
> - * is, merge as well
> - */
> - i++;
> - if (i < count && range.start + range.len == old_ranges[i].start) {
> - old_ranges[i - 1].len += old_ranges[i].len;
> + for (i = 0; i < ranges->n_prot && len > 0; i++) {
> + if (prot[i].start <= pos && prot[i].start + prot[i].len >= pos + len) {
> + len = 0;
> + break; /* Might as well, the next two conditions can't be true */
> + }
>
> - for (; i + 1 < count; i++)
> - old_ranges[i] = old_ranges[i + 1];
> - ranges->n_prot--;
> + /* Can easily extend this down just by adjusting start */
> + if (pos <= prot[i].start && pos + len >= prot[i].start) {
> + prot[i].len += prot[i].start - pos;
> + prot[i].start = pos;
> + pos += prot[i].len;
> + if (prot[i].len >= len)
> + len = 0;
> + else
> + len -= prot[i].len;
> }
>
> - return 0;
> + /*
> + * Jump over this range but the new range might be so big that
> + * theres a chunk after
> + */
> + if (pos >= prot[i].start && pos < prot[i].start + prot[i].len) {
> + if (prot[i].start + prot[i].len - pos > len) {
> + len -= prot[i].start + prot[i].len - pos;
> + pos = prot[i].start + prot[i].len;
> + } else {
> + len = 0;
> + }
> + }
> + /*
> + * This condition will be true if the range is smaller than
> + * the current range, therefore it should go here!
> + */
> + if (pos < prot[i].start && pos + len <= prot[i].start)
> + break;
> }
>
> - if (ranges->n_prot == ranges->total_prot) {
> - new_ranges = realloc(ranges->prot, sizeof(range) * ((ranges->n_prot) + PROT_REALLOC_NUM));
> - if (new_ranges)
> + if (len) {
> + int insert_pos = i;
> + struct bl_prot_range *new_ranges = ranges->prot;
> + if (ranges->n_prot == ranges->total_prot) {
> + new_ranges = realloc(ranges->prot,
> + sizeof(range) * ((ranges->n_prot) + PROT_REALLOC_NUM));
> + if (!new_ranges)
> + return false;
> ranges->total_prot += PROT_REALLOC_NUM;
> - } else {
> - new_ranges = old_ranges;
> - }
> - if (new_ranges) {
> - memcpy(new_ranges + ranges->n_prot, &range, sizeof(range));
> + }
> + if (insert_pos != ranges->n_prot)
> + for (i = ranges->n_prot; i > insert_pos; i--)
> + memcpy(&new_ranges[i], &new_ranges[i - 1], sizeof(range));
> + range.start = pos;
> + range.len = len;
> + memcpy(&new_ranges[insert_pos], &range, sizeof(range));
> ranges->prot = new_ranges;
> ranges->n_prot++;
> }
>
> - return !new_ranges;
> + /* Probably only worth mergeing when we're low on space */
> + if (ranges->n_prot + 1 == ranges->total_prot) {
> + /* Check to see if we can merge ranges */
> + for (i = 0; i < ranges->n_prot - 1; i++) {
> + if (prot[i].start + prot[i].len == prot[i + 1].start) {
> + int j;
> + prot[i].len += prot[i + 1].len;
> + for (j = i + 1; j < ranges->n_prot - 1; j++)
> + memcpy(&prot[j] , &prot[j + 1], sizeof(range));
> + ranges->n_prot--;
> + i--; /* Maybe the next one can merge too */
> + }
> + }
> + }
> +
> + return true;
> }
>
> int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_t len)
> @@ -385,11 +427,7 @@ int blocklevel_ecc_protect(struct blocklevel_device *bl, uint32_t start, uint32_
> */
> struct bl_prot_range range = { .start = start, .len = len };
>
> - /*
> - * Refuse to add regions that are already protected or are partially
> - * protected
> - */
> - if (len < BYTES_PER_ECC || ecc_protected(bl, start, len))
> + if (len < BYTES_PER_ECC)
> return -1;
> - return insert_bl_prot_range(&bl->ecc_prot, range);
> + return !insert_bl_prot_range(&bl->ecc_prot, range);
> }
> diff --git a/libflash/libffs.c b/libflash/libffs.c
> index 8134962..97b48c4 100644
> --- a/libflash/libffs.c
> +++ b/libflash/libffs.c
> @@ -194,7 +194,7 @@ int ffs_init(uint32_t offset, uint32_t max_size, struct blocklevel_device *bl,
> if (ecc) {
> rc = blocklevel_ecc_protect(bl, start, total_size);
> if (rc) {
> - FL_ERR("Failed to blocklevel_ecc_protect(0x%08x, 0x%08x)\n",
> + FL_ERR("FFS: Failed to blocklevel_ecc_protect(0x%08x, 0x%08x)\n",
> start, total_size);
> goto out;
> }
> diff --git a/libflash/test/test-blocklevel.c b/libflash/test/test-blocklevel.c
> index ef5d9b5..9630939 100644
> --- a/libflash/test/test-blocklevel.c
> +++ b/libflash/test/test-blocklevel.c
> @@ -44,6 +44,10 @@ int main(void)
> ERR("Failed to blocklevel_ecc_protect(0x3000, 0x1000)\n");
> return 1;
> }
> + if (blocklevel_ecc_protect(bl, 0x2f00, 0x1100)) {
> + ERR("Failed to blocklevel_ecc_protect(0x2f00, 0x1100)\n");
> + return 1;
> + }
>
> /* Zero length protection */
> if (!blocklevel_ecc_protect(bl, 0x4000, 0)) {
> @@ -58,14 +62,20 @@ int main(void)
> }
>
> /* Deal with overlapping protections */
> - if (!blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> - ERR("Shouldn't have succeeded blocklevel_ecc_protect(0x100, 0x1000)\n");
> + if (blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> + ERR("Failed to protect overlaping region blocklevel_ecc_protect(0x100, 0x1000)\n");
> + return 1;
> + }
> +
> + /* Deal with overflow */
> + if (!blocklevel_ecc_protect(bl, 1, 0xFFFFFFFF)) {
> + ERR("Added an 'overflow' protection blocklevel_ecc_protect(1, 0xFFFFFFFF)\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");
> + /* Protect everything */
> + if (blocklevel_ecc_protect(bl, 0, 0xFFFFFFFF)) {
> + ERR("Couldn't protect everything blocklevel_ecc_protect(0, 0xFFFFFFFF)\n");
> return 1;
> }
>
> @@ -84,17 +94,30 @@ int main(void)
> return 1;
> }
>
> - if (ecc_protected(bl, 0x1000, 0) != 0) {
> + /* Clear the protections */
> + bl->ecc_prot.n_prot = 0;
> + /* Reprotect */
> + if (blocklevel_ecc_protect(bl, 0x3000, 0x1000)) {
> + ERR("Failed to blocklevel_ecc_protect(0x3000, 0x1000)\n");
> + return 1;
> + }
> + /* Deal with overlapping protections */
> + if (blocklevel_ecc_protect(bl, 0x100, 0x1000)) {
> + ERR("Failed to protect overlaping region blocklevel_ecc_protect(0x100, 0x1000)\n");
> + return 1;
> + }
> +
> + if (ecc_protected(bl, 0x1000, 0) != 1) {
> ERR("Invalid result for ecc_protected(0x1000, 0)\n");
> return 1;
> }
>
> - if (ecc_protected(bl, 0x1000, 0x1000) != 0) {
> + if (ecc_protected(bl, 0x1000, 0x1000) != -1) {
> ERR("Invalid result for ecc_protected(0x1000, 0x1000)\n");
> return 1;
> }
>
> - if (ecc_protected(bl, 0x1000, 0x100) != 0) {
> + if (ecc_protected(bl, 0x1000, 0x100) != 1) {
> ERR("Invalid result for ecc_protected(0x1000, 0x100)\n");
> return 1;
> }
> @@ -104,7 +127,7 @@ int main(void)
> return 1;
> }
>
> - if (ecc_protected(bl, 0x4000, 1) != 1) {
> + if (ecc_protected(bl, 0x4000, 1) != 0) {
> ERR("Invalid result for ecc_protected(0x4000, 1)\n");
> return 1;
> }
> @@ -136,6 +159,11 @@ int main(void)
> return 1;
> }
>
> + if (blocklevel_ecc_protect(bl, 0x4f00, 0x100)) {
> + ERR("Failed to blocklevel_ecc_protected(0x4900, 0x100)\n");
> + return 1;
> + }
> +
> if (blocklevel_ecc_protect(bl, 0x4900, 0x100)) {
> ERR("Failed to blocklevel_ecc_protected(0x4900, 0x100)\n");
> return 1;
> @@ -146,8 +174,8 @@ int main(void)
> return 1;
> }
>
> - if (!blocklevel_ecc_protect(bl, 0x5290, 0x10)) {
> - ERR("Shouldn't have been able to blocklevel_ecc_protect(0x5290, 0x10)\n");
> + if (blocklevel_ecc_protect(bl, 0x5290, 0x10)) {
> + ERR("Failed to blocklevel_ecc_protect(0x5290, 0x10)\n");
> return 1;
> }
>
> @@ -166,7 +194,9 @@ int main(void)
> ERR("Failed to blocklevel_ecc_protect(0x6100, 0x100)\n");
> return 1;
> }
> -
> + /* Make sure we trigger the merging code */
> + for (i = bl->ecc_prot.n_prot; i < bl->ecc_prot.total_prot; i++)
> + blocklevel_ecc_protect(bl, 0x10000 + i * 0x200, 0x10);
> /* Check that the region merging works */
> for (i = 0; i < bl->ecc_prot.n_prot - 1; i++) {
> if (bl->ecc_prot.prot[i].start + bl->ecc_prot.prot[i].len == bl->ecc_prot.prot[i + 1].start ||
More information about the Skiboot
mailing list