November 2023 - flashrom-gerrit

[L] Change in flashrom[main]: tests: Unit tests for erase function selection algo
by Anastasia Klimchuk (Code Review) 12 Nov '23

12 Nov '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/67535?usp=email ) Change subject: tests: Unit tests for erase function selection algo ...................................................................... tests: Unit tests for erase function selection algo The test checks that algorithm for erase functions selection works and there are no regressions. Specifically, test contains an array of test cases. Each case initialises a given initial state of the memory for the mock chip, and layout regions on the chip, and then performs erase and write operations. At the end of operation, test asserts the following: - the state of mock chip memory is as expected, i.e. properly erased or written - erase blocks are invoked in expected order and expected number of them - chip operation (erase or write) returned 0. Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/67535 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- A tests/erase_func_algo.c M tests/meson.build M tests/tests.c M tests/tests.h 4 files changed, 751 insertions(+), 0 deletions(-) Approvals: Peter Marheine: Looks good to me, approved build bot (Jenkins): Verified diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c new file mode 100644 index 0000000..824a03a --- /dev/null +++ b/tests/erase_func_algo.c @@ -0,0 +1,738 @@ +/* + * This file is part of the flashrom project. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 2 of the License. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <include/test.h> +#include <stdio.h> +#include <string.h> + +#include "tests.h" +#include "chipdrivers.h" +#include "flash.h" +#include "io_mock.h" +#include "libflashrom.h" +#include "programmer.h" + +#define LOG_ERASE_FUNC printf("Eraser called with blockaddr=0x%x, blocklen=0x%x\n", blockaddr, blocklen) +#define LOG_READ_WRITE_FUNC printf("%s called with start=0x%x, len=0x%x\n", __func__, start, len) + +#define ERASE_VALUE 0xff +#define MOCK_CHIP_SIZE 16 +/* How many small chip test cases. */ +#define TEST_CASES_NUM 16 +#define MIN_BUF_SIZE 1024 /* Minimum buffer size flashrom operates for chip operations. */ +#define MIN_REAL_CHIP_SIZE 1024 /* Minimum chip size that can be defined for real chip in flashchips */ + +struct test_region { + const size_t start; + const size_t end; + const char *name; +}; + +struct erase_invoke { + unsigned int blockaddr; + unsigned int blocklen; +}; + +struct test_case { + struct flashchip *chip; /* Chip definition. */ + struct test_region regions[MOCK_CHIP_SIZE]; /* Layout regions. */ + uint8_t initial_buf[MOCK_CHIP_SIZE]; /* Initial state of chip memory. */ + uint8_t erased_buf[MOCK_CHIP_SIZE]; /* Expected content after erase. */ + uint8_t written_buf[MOCK_CHIP_SIZE]; /* Expected content after write. */ + struct erase_invoke eraseblocks_expected[MOCK_CHIP_SIZE]; /* Expected order of eraseblocks invocations. */ + unsigned int eraseblocks_expected_ind; /* Expected number of eraseblocks invocations. */ + char erase_test_name[20]; /* Test case display name for testing erase operation. */ + char write_test_name[20]; /* Test case display name for testing write operation. */ +}; + +struct all_state { + uint8_t buf[MIN_REAL_CHIP_SIZE]; /* Buffer emulating the memory of the mock chip. */ + struct erase_invoke eraseblocks_actual[MOCK_CHIP_SIZE]; /* The actual order of eraseblocks invocations. */ + unsigned int eraseblocks_actual_ind; /* Actual number of eraseblocks invocations. */ + const struct test_case* current_test_case; /* Currently executed test case. */ +} g_state; + +static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) +{ + if (start + len <= MOCK_CHIP_SIZE) + LOG_READ_WRITE_FUNC; + + assert_in_range(start + len, 0, MIN_REAL_CHIP_SIZE); + + memcpy(buf, &g_state.buf[start], len); + return 0; +} + +static int write_chip(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len) +{ + if (start + len <= MOCK_CHIP_SIZE) + LOG_READ_WRITE_FUNC; + + assert_in_range(start + len, 0, MIN_REAL_CHIP_SIZE); + + memcpy(&g_state.buf[start], buf, len); + return 0; +} + +static int block_erase_chip(struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen) +{ + if (blockaddr + blocklen <= MOCK_CHIP_SIZE) { + LOG_ERASE_FUNC; + + /* Register eraseblock invocation. */ + g_state.eraseblocks_actual[g_state.eraseblocks_actual_ind].blocklen + = blocklen; + g_state.eraseblocks_actual[g_state.eraseblocks_actual_ind].blockaddr + = blockaddr; + g_state.eraseblocks_actual_ind++; + } + + assert_in_range(blockaddr + blocklen, 0, MIN_REAL_CHIP_SIZE); + + memset(&g_state.buf[blockaddr], ERASE_VALUE, blocklen); + return 0; +} + +extern write_func_t *g_test_write_injector; +extern read_func_t *g_test_read_injector; +extern erasefunc_t *g_test_erase_injector; + +static struct flashchip chip_1_2_4_8_16 = { + .vendor = "aklm", + /* + * total_size is supposed to be in Kilobytes and this number is multiplied + * by 1024 everywhere in flashrom code. With this, we can't have the chip of + * size MOCK_CHIP_SIZE anyway, because MOCK_CHIP_SIZE is much smaller than + * 1024. + * + * So setting 1 here as this is the smallest possible value of unsigned int. + * Why aim for the smallest value? Because various places in flashrom code + * are allocating buffers of size total_size * 1024, however in these unit + * tests only MOCK_CHIP_SIZE bytes are tracked/logged/asserted. + */ + .total_size = 1, + .tested = TEST_OK_PREW, + .gran = WRITE_GRAN_1BYTE, + .read = TEST_READ_INJECTOR, + .write = TEST_WRITE_INJECTOR, + .block_erasers = + { + { + .eraseblocks = { {1, MIN_REAL_CHIP_SIZE} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {2, MIN_REAL_CHIP_SIZE / 2} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {4, MIN_REAL_CHIP_SIZE / 4} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {8, MIN_REAL_CHIP_SIZE / 8} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {16, MIN_REAL_CHIP_SIZE / 16} }, + .block_erase = TEST_ERASE_INJECTOR, + } + }, +}; + +static struct flashchip chip_1_8_16 = { + .vendor = "aklm", + /* See comment on previous chip. */ + .total_size = 1, + .tested = TEST_OK_PREW, + .gran = WRITE_GRAN_1BYTE, + .read = TEST_READ_INJECTOR, + .write = TEST_WRITE_INJECTOR, + .block_erasers = + { + { + .eraseblocks = { {1, MIN_REAL_CHIP_SIZE} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {8, MIN_REAL_CHIP_SIZE / 8} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {16, MIN_REAL_CHIP_SIZE / 16} }, + .block_erase = TEST_ERASE_INJECTOR, + } + }, +}; + +static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, + const char *programmer_param, struct test_case *current_test_case) +{ + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector = block_erase_chip; + + /* First MOCK_CHIP_SIZE bytes have a meaning and set with given values for this test case. */ + memcpy(g_state.buf, current_test_case->initial_buf, MOCK_CHIP_SIZE); + /* The rest of mock chip memory does not matter. */ + memset(g_state.buf + MOCK_CHIP_SIZE, ERASE_VALUE, MIN_REAL_CHIP_SIZE - MOCK_CHIP_SIZE); + + /* Clear eraseblock invocation records. */ + memset(g_state.eraseblocks_actual, 0, MOCK_CHIP_SIZE * sizeof(struct erase_invoke)); + g_state.eraseblocks_actual_ind = 0; + + flashctx->chip = current_test_case->chip; + + printf("Creating layout ... "); + assert_int_equal(0, flashrom_layout_new(layout)); + + /* Adding regions from test case. */ + int i = 0; + while (current_test_case->regions[i].name != NULL) { + assert_int_equal(0, flashrom_layout_add_region(*layout, + current_test_case->regions[i].start, + current_test_case->regions[i].end, + current_test_case->regions[i].name)); + assert_int_equal(0, flashrom_layout_include_region(*layout, current_test_case->regions[i].name)); + i++; + } + + flashrom_layout_set(flashctx, *layout); + printf("done\n"); + + /* + * We need some programmer (any), and dummy is a very good one, + * because it doesn't need any mocking. So no extra complexity + * from a programmer side, and test can focus on working with the chip. + */ + printf("Dummyflasher initialising with param=\"%s\"... ", programmer_param); + assert_int_equal(0, programmer_init(&programmer_dummy, programmer_param)); + /* Assignment below normally happens while probing, but this test is not probing. */ + flashctx->mst = &registered_masters[0]; + printf("done\n"); +} + +static void teardown_chip(struct flashrom_layout **layout) +{ + printf("Dummyflasher shutdown... "); + assert_int_equal(0, programmer_shutdown()); + printf("done\n"); + + printf("Releasing layout... "); + flashrom_layout_release(*layout); + printf("done\n"); +} + +/* + * Setup all test cases. + * + * First half of test cases (0 - (size(testcases)/2-1)) set up for a chip with erasers: 1, 2, 4, 8, 16 bytes. + * Second half (size(testcases)/2 - size(testcases)) repeates the same test cases for a chip with erasers: 1, 8, 16 bytes. + */ +static struct test_case test_cases[] = { + { + /* + * Test case #0 + * + * Initial vs written: all 16 bytes are different. + * One layout region for the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x8, + 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18, + 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #0", + .write_test_name = "Write test case #0", + }, { + /* + * Test case #1 + * + * Initial vs written: 9 bytes the same, 7 bytes different. + * Two layout regions each one 8 bytes, which is 1/2 size of chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MOCK_CHIP_SIZE/2 - 1, "part1"}, {MOCK_CHIP_SIZE/2, MIN_REAL_CHIP_SIZE - 1, "part2"}}, + .initial_buf = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, + 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, 0xf8, + 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f}, + .eraseblocks_expected = {{0x8, 0x8}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 2, + .erase_test_name = "Erase test case #1", + .write_test_name = "Write test case #1", + }, { + /* + * Test case #2 + * + * Initial vs written: 6 bytes the same, 4 bytes different, 4 bytes the same, 2 bytes different. + * Two layout regions 11 and 5 bytes each. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, 10, "odd1"}, {11, 15, "odd2"}, {MOCK_CHIP_SIZE, MIN_REAL_CHIP_SIZE - 1, "longtail"}}, + .initial_buf = {0xff, 0xff, 0x0, 0xff, 0x0, 0xff, 0x0, 0xff, + 0x0, 0xff, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xff, 0xff, 0x0, 0xff, 0x0, 0xff, 0x20, 0x2f, + 0x20, 0x2f, 0x0, 0xff, 0xff, 0xff, 0x2f, 0x2f}, + .eraseblocks_expected = {{0xb, 0x1}, {0xc, 0x4}, {0xa, 0x1}, {0x8, 0x2}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 5, + .erase_test_name = "Erase test case #2", + .write_test_name = "Write test case #2", + }, { + /* + * Test case #3 + * + * Initial vs written: 4 bytes the same, 4 bytes different, 8 bytes the same. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0xff, 0xff, 0xff, 0xff, 0x11, 0x22, 0x33, 0x44, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xff, 0xff, 0xff, 0xff, 0x1, 0x2, 0x3, 0x4, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #3", + .write_test_name = "Write test case #3", + }, { + /* + * Test case #4 + * + * Initial vs written: 4 bytes different, 4 bytes the same, 8 bytes different. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0xff, 0xff, 0xff, 0xff, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x11, 0x22, 0x33, 0x44, 0xff, 0xff, 0xff, 0xff, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #4", + .write_test_name = "Write test case #4", + }, { + /* + * Test case #5 + * + * Initial vs written: 7 bytes different, 1 bytes the same, 8 bytes different. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xff, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0xff, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #5", + .write_test_name = "Write test case #5", + }, { + /* + * Test case #6 + * + * Initial vs written: 7 bytes the same, 1 bytes different, 8 bytes the same. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x1d, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xdd, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #6", + .write_test_name = "Write test case #6", + }, { + /* + * Test case #7 + * + * Initial vs written: all 16 bytes are different. + * Layout with irregular regions unaligned with eraseblocks. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, 2, "reg3"}, {3, 7, "reg5"}, {8, 14, "reg7"}, {15, MIN_REAL_CHIP_SIZE - 1, "reg1"}}, + .initial_buf = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, + 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f}, + .eraseblocks_expected = {{0xf, 0x1}, {0xe, 0x1}, {0xc, 0x2}, {0x8, 0x4}, + {0x3, 0x1}, {0x4, 0x4}, {0x2, 0x1}, {0x0, 0x2}}, + .eraseblocks_expected_ind = 8, + .erase_test_name = "Erase test case #7", + .write_test_name = "Write test case #7", + }, { + /* + * Test case #8 + * + * Initial vs written: all 16 bytes are different. + * One layout region for the whole chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, + 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #8", + .write_test_name = "Write test case #8", + }, { + /* + * Test case #9 + * + * Initial vs written: 9 bytes the same, 7 bytes different. + * Two layout regions each one 8 bytes, which is 1/2 size of chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MOCK_CHIP_SIZE/2 - 1, "part1"}, {MOCK_CHIP_SIZE/2, MOCK_CHIP_SIZE - 1, "part2"}, + {MOCK_CHIP_SIZE, MIN_REAL_CHIP_SIZE - 1, "longtail"}}, + .initial_buf = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, + 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, + 0xf8, 0x29, 0x2a, 0x2b, 0x2c, 0x2d, 0x2e, 0x2f}, + .eraseblocks_expected = {{0x8, 0x8}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 2, + .erase_test_name = "Erase test case #9", + .write_test_name = "Write test case #9", + }, { + /* + * Test case #10 + * + * Initial vs written: 6 bytes the same, 4 bytes different, 4 bytes the same, 2 bytes different. + * Two layout regions 11 and 5 bytes each. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, 10, "odd1"}, {11, 15, "odd2"}, {MOCK_CHIP_SIZE, MIN_REAL_CHIP_SIZE - 1, "longtail"}}, + .initial_buf = {0xff, 0xff, 0x0, 0xff, 0x0, 0xff, 0x0, 0xff, + 0x0, 0xff, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xff, 0xff, 0x0, 0xff, 0x0, 0xff, 0x20, 0x2f, + 0x20, 0x2f, 0x0, 0xff, 0xff, 0xff, 0x2f, 0x2f}, + .eraseblocks_expected = {{0xb, 0x1}, {0xc, 0x1}, {0xd, 0x1}, {0xe, 0x1}, + {0xf, 0x1}, {0x8, 0x1}, {0x9, 0x1}, {0xa, 0x1}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 9, + .erase_test_name = "Erase test case #10", + .write_test_name = "Write test case #10", + }, { + /* + * Test case #11 + * + * Initial vs written: 4 bytes the same, 4 bytes different, 8 bytes the same. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0xff, 0xff, 0xff, 0xff, 0x11, 0x22, 0x33, 0x44, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0xff, 0xff, 0xff, 0xff, 0x1, 0x2, 0x3, 0x4, + 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #11", + .write_test_name = "Write test case #11", + }, { + /* + * Test case #12 + * + * Initial vs written: 4 bytes different, 4 bytes the same, 8 bytes different. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0xff, 0xff, 0xff, 0xff, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x11, 0x22, 0x33, 0x44, 0xff, 0xff, 0xff, 0xff, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #12", + .write_test_name = "Write test case #12", + }, { + /* + * Test case #13 + * + * Initial vs written: 7 bytes different, 1 bytes the same, 8 bytes different. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xff, + 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, 0x18}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x11, 0x22, 0x33, 0x44, 0x55, 0x66, 0x77, 0xff, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #13", + .write_test_name = "Write test case #13", + }, { + /* + * Test case #14 + * + * Initial vs written: 7 bytes the same, 1 bytes different, 8 bytes the same. + * One layout region covering the whole chip. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x1d, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xdd, + 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, + .eraseblocks_expected = {{0x0, 0x10}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #14", + .write_test_name = "Write test case #14", + }, { + /* + * Test case #15 + * + * Initial vs written: all 16 bytes are different. + * Layout with irregular regions unaligned with eraseblocks. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, 2, "reg3"}, {3, 7, "reg5"}, {8, 14, "reg7"}, + {15, MIN_REAL_CHIP_SIZE - 1, "reg1"}}, + .initial_buf = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, + 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x10, 0x11, 0x12, 0x13, 0x14, 0x15, 0x16, 0x17, + 0x18, 0x19, 0x1a, 0x1b, 0x1c, 0x1d, 0x1e, 0x1f}, + .eraseblocks_expected = {{0xf, 0x1}, {0x8, 0x1}, {0x9, 0x1}, {0xa, 0x1}, + {0xb, 0x1}, {0xc, 0x1}, {0xd, 0x1}, {0xe, 0x1}, + {0x3, 0x1}, {0x4, 0x1}, {0x5, 0x1}, {0x6, 0x1}, + {0x7, 0x1}, {0x0, 0x1}, {0x1, 0x1}, {0x2, 0x1}}, + .eraseblocks_expected_ind = 16, + .erase_test_name = "Erase test case #15", + .write_test_name = "Write test case #15", + }, +}; + +static int setup(void **state) { + struct test_case *current_test_case = *state; + g_state.current_test_case = current_test_case; + return 0; +} + +static int teardown(void **state) { + return 0; +} + +/* + * Creates the array of tests for each test case in test_cases[]. + * Each test_case produces two tests: one for erase and one for write operation. + * The caller needs to free the allocated memory. + */ +struct CMUnitTest *get_erase_func_algo_tests(size_t *num_tests) { + // Twice the number of test cases, because each test case is run twice: for erase and write. + struct CMUnitTest *all_cases = calloc(TEST_CASES_NUM * 2, sizeof(struct CMUnitTest)); + *num_tests = TEST_CASES_NUM * 2; + + for (size_t i = 0; i < TEST_CASES_NUM; i++) { + all_cases[i] = (struct CMUnitTest) { + .name = test_cases[i].erase_test_name, + .setup_func = setup, + .teardown_func = teardown, + .initial_state = &test_cases[i], + .test_func = erase_function_algo_test_success, + }; + all_cases[i + TEST_CASES_NUM] = (struct CMUnitTest) { + .name = test_cases[i].write_test_name, + .setup_func = setup, + .teardown_func = teardown, + .initial_state = &test_cases[i], + .test_func = write_function_algo_test_success, + }; + } + + return all_cases; +} + +/* + * This function is invoked for every test case in test_cases[], + * current test case is passed as an argument. + */ +void erase_function_algo_test_success(void **state) +{ + struct test_case* current_test_case = *state; + + int all_erase_tests_result = 0; + struct flashrom_flashctx flashctx = { 0 }; + const char *param = ""; /* Default values for all params. */ + + struct flashrom_layout *layout; + + setup_chip(&flashctx, &layout, param, current_test_case); + + printf("%s started.\n", current_test_case->erase_test_name); + int ret = flashrom_flash_erase(&flashctx); + printf("%s returned %d.\n", current_test_case->erase_test_name, ret); + + int chip_erased = !memcmp(g_state.buf, current_test_case->erased_buf, MOCK_CHIP_SIZE); + + int eraseblocks_in_order = !memcmp(g_state.eraseblocks_actual, + current_test_case->eraseblocks_expected, + current_test_case->eraseblocks_expected_ind * sizeof(struct erase_invoke)); + + int eraseblocks_invocations = (g_state.eraseblocks_actual_ind == + current_test_case->eraseblocks_expected_ind); + + if (chip_erased) + printf("Erased chip memory state for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Erased chip memory state for %s is WRONG\n", + current_test_case->erase_test_name); + + if (eraseblocks_in_order) + printf("Eraseblocks order of invocation for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Eraseblocks order of invocation for %s is WRONG\n", + current_test_case->erase_test_name); + + if (eraseblocks_invocations) + printf("Eraseblocks number of invocations for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Eraseblocks number of invocations for %s is WRONG, expected %d actual %d\n", + current_test_case->erase_test_name, + current_test_case->eraseblocks_expected_ind, + g_state.eraseblocks_actual_ind); + + all_erase_tests_result |= ret; + all_erase_tests_result |= !chip_erased; + all_erase_tests_result |= !eraseblocks_in_order; + all_erase_tests_result |= !eraseblocks_invocations; + + teardown_chip(&layout); + + assert_int_equal(0, all_erase_tests_result); +} + +/* + * This function is invoked for every test case in test_cases[], + * current test case is passed as an argument. + */ +void write_function_algo_test_success(void **state) +{ + struct test_case* current_test_case = *state; + + int all_write_test_result = 0; + struct flashrom_flashctx flashctx = { 0 }; + uint8_t newcontents[MIN_BUF_SIZE]; + const char *param = ""; /* Default values for all params. */ + + struct flashrom_layout *layout; + + setup_chip(&flashctx, &layout, param, current_test_case); + memcpy(&newcontents, current_test_case->written_buf, MOCK_CHIP_SIZE); + + printf("%s started.\n", current_test_case->write_test_name); + int ret = flashrom_image_write(&flashctx, &newcontents, MIN_BUF_SIZE, NULL); + printf("%s returned %d.\n", current_test_case->write_test_name, ret); + + int chip_written = !memcmp(g_state.buf, current_test_case->written_buf, MOCK_CHIP_SIZE); + + if (chip_written) + printf("Written chip memory state for %s is CORRECT\n", + current_test_case->write_test_name); + else + printf("Written chip memory state for %s is WRONG\n", + current_test_case->write_test_name); + + all_write_test_result |= ret; + all_write_test_result |= !chip_written; + + teardown_chip(&layout); + + assert_int_equal(0, all_write_test_result); +} diff --git a/tests/meson.build b/tests/meson.build index 94f60e2..fd05166 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -24,6 +24,7 @@ 'chip_wp.c', 'selfcheck.c', 'io_real.c', + 'erase_func_algo.c', ) if not programmer.get('dummy').get('active') diff --git a/tests/tests.c b/tests/tests.c index 35bd599..c38a66c 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,12 @@ }; ret |= cmocka_run_group_tests_name("chip.c tests", chip_tests, NULL, NULL); + size_t n_erase_tests; + struct CMUnitTest *erase_func_algo_tests = get_erase_func_algo_tests(&n_erase_tests); + ret |= _cmocka_run_group_tests("erase_func_algo.c tests", erase_func_algo_tests, n_erase_tests, NULL, NULL); + free(erase_func_algo_tests); + + // Write-protect group should run last. const struct CMUnitTest chip_wp_tests[] = { cmocka_unit_test(invalid_wp_range_dummyflasher_test_success), cmocka_unit_test(set_wp_range_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index 3841d20..af5f968 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -17,6 +17,7 @@ #define TESTS_H #include <fcntl.h> +#include <include/test.h> /* helpers.c */ void address_to_bits_test_success(void **state); @@ -104,4 +105,9 @@ void selfcheck_eraseblocks(void **state); void selfcheck_board_matches_table(void **state); +/* erase_func_algo.c */ +struct CMUnitTest *get_erase_func_algo_tests(size_t *num_tests); +void erase_function_algo_test_success(void **state); +void write_function_algo_test_success(void **state); + #endif /* TESTS_H */ -- To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Gerrit-Change-Number: 67535 Gerrit-PatchSet: 22 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: merged

1 0

[S] Change in flashrom[main]: erasure_layout: Add region boundaries to printed info message
by Anastasia Klimchuk (Code Review) 12 Nov '23

12 Nov '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/78492?usp=email ) Change subject: erasure_layout: Add region boundaries to printed info message ...................................................................... erasure_layout: Add region boundaries to printed info message Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/78492 Reviewed-by: Alexander Goncharov <chat(a)joursoir.net> Reviewed-by: Aarya <aarya.chaumal(a)gmail.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M erasure_layout.c 1 file changed, 8 insertions(+), 2 deletions(-) Approvals: Alexander Goncharov: Looks good to me, approved build bot (Jenkins): Verified Aarya: Looks good to me, approved diff --git a/erasure_layout.c b/erasure_layout.c index b43e52e..0c31911 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -174,11 +174,17 @@ } if (start_diff) { - msg_cinfo("Region start not sector aligned! Extending start boundaries...\n"); + msg_cinfo("Region [0x%08x - 0x%08x] is not sector aligned! " + "Extending start boundaries by 0x%08x bytes, from 0x%08x -> 0x%08x\n", + *region_start, *region_end, + start_diff, *region_start, *region_start - start_diff); *region_start = *region_start - start_diff; } if (end_diff) { - msg_cinfo("Region end not sector aligned! Extending end boundaries...\n"); + msg_cinfo("Region [0x%08x - 0x%08x] is not sector aligned! " + "Extending end boundaries by 0x%08x bytes, from 0x%08x -> 0x%08x\n", + *region_start, *region_end, + end_diff, *region_end, *region_end + end_diff); *region_end = *region_end + end_diff; } } -- To view, visit https://review.coreboot.org/c/flashrom/+/78492?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I511a79754cff15e7dba26f5313d7015830780f60 Gerrit-Change-Number: 78492 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix double invocation of erasers
by Anastasia Klimchuk (Code Review) 12 Nov '23

12 Nov '23

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/77747?usp=email ) Change subject: erasure_layout: Fix double invocation of erasers ...................................................................... erasure_layout: Fix double invocation of erasers Erasefn was invoked over the same block of memory twice. This patch removes the second redundant invokation. It was accidentally introduced during earlier refactoring of the code. Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/77747 Reviewed-by: Alexander Goncharov <chat(a)joursoir.net> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c 1 file changed, 0 insertions(+), 9 deletions(-) Approvals: Peter Marheine: Looks good to me, approved build bot (Jenkins): Verified Alexander Goncharov: Looks good to me, approved diff --git a/erasure_layout.c b/erasure_layout.c index c9ac44b..b43e52e 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -339,15 +339,6 @@ } } - ret = erasefn(flashctx, start_addr, block_len); - if (ret) { - msg_cerr("Failed to execute erase command " - "for offset %#"PRIx32" to %#"PRIx32".\n", - start_addr, start_addr + block_len); - ret = -1; - goto _end; - } - // adjust curcontents memset(curcontents+start_addr, erased_value, block_len); // after erase make it unselected again -- To view, visit https://review.coreboot.org/c/flashrom/+/77747?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I92351eba0fd29114ce98b4a839358e92d176af28 Gerrit-Change-Number: 77747 Gerrit-PatchSet: 8 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[main]: tests: Add test cases for erasing/writing unaligned layout regions
by Anastasia Klimchuk (Code Review) 10 Nov '23

10 Nov '23

Attention is currently required from: Aarya, Alexander Goncharov, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78985?usp=email ) Change subject: tests: Add test cases for erasing/writing unaligned layout regions ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: Added one more test case when logical layout does not cover the whole chip memory. File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/78985/comment/f9da4339_a73373ff : PS1, Line 31: #define TEST_CASES_NUM 19 > Any reason not to make this `(sizeof(test_cases) / sizeof(*test_cases))` instead? That way it doesn' […] Actually yes, no reason. I think this was from the very first version of the test, maybe for creating array, and just stayed. I removed the macro. -- To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Gerrit-Change-Number: 78985 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 10 Nov 2023 10:40:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: tests: Add test cases for erasing/writing unaligned layout regions
by Anastasia Klimchuk (Code Review) 10 Nov '23

10 Nov '23

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk. Hello Aarya, Alexander Goncharov, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/78985?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: tests: Add test cases for erasing/writing unaligned layout regions ...................................................................... tests: Add test cases for erasing/writing unaligned layout regions The test cases from #16 onwards cover the case when layout region is not aligned with eraseblock region, and therefore either layout start boundary or end boundary needs to be extended to align with eraseblock. Ticket: https://ticket.coreboot.org/issues/494 Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M tests/erase_func_algo.c 1 file changed, 118 insertions(+), 8 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/78985/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Gerrit-Change-Number: 78985 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[main]: tests: Add test cases for erasing/writing unaligned layout regions
by Peter Marheine (Code Review) 09 Nov '23

09 Nov '23

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78985?usp=email ) Change subject: tests: Add test cases for erasing/writing unaligned layout regions ...................................................................... Patch Set 1: (1 comment) File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/78985/comment/350ae07f_6cc0b6de : PS1, Line 31: #define TEST_CASES_NUM 19 Any reason not to make this `(sizeof(test_cases) / sizeof(*test_cases))` instead? That way it doesn't need to be manually updated. -- To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Gerrit-Change-Number: 78985 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 09 Nov 2023 22:57:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: tests: Unit tests for erase function selection algo
by Peter Marheine (Code Review) 09 Nov '23

09 Nov '23

Attention is currently required from: Aarya, Anastasia Klimchuk, Edward O'Callaghan, Nikolai Artemiev, Simon Buhrow, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67535?usp=email ) Change subject: tests: Unit tests for erase function selection algo ...................................................................... Patch Set 21: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/67535?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I8f3fdefb76e71f6f8dc295d9dead5f38642aace7 Gerrit-Change-Number: 67535 Gerrit-PatchSet: 21 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Simon Buhrow Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Alexander Goncharov <chat(a)joursoir.net> Gerrit-CC: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-CC: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Simon Buhrow Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Thu, 09 Nov 2023 22:56:24 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix unaligned region end offset by 1
by Peter Marheine (Code Review) 09 Nov '23

09 Nov '23

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78984?usp=email ) Change subject: erasure_layout: Fix unaligned region end offset by 1 ...................................................................... Patch Set 1: Code-Review+2 (1 comment) Patchset: PS1: I had to study this a bit to understand what's happening, but I see now that the issue stems from how bounds for an operation are inclusive so starting from `old_end` will start from the last byte of the region to be erased. -- To view, visit https://review.coreboot.org/c/flashrom/+/78984?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7f78a0090065cd2a952cba1a5d28179483ba4c55 Gerrit-Change-Number: 78984 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 09 Nov 2023 22:55:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix unaligned region end offset by 1
by Anastasia Klimchuk (Code Review) 09 Nov '23

09 Nov '23

Attention is currently required from: Aarya, Alexander Goncharov, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/78984?usp=email ) Change subject: erasure_layout: Fix unaligned region end offset by 1 ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: The open question here is whether anything is needed for start region branch. I made new test cases in CB:78985 and I was able to repro the issue with end region boundary expansion (the test in CB:78985 fails on head and passes on this patch). However I was not yet able to come up with test case to repro the issue with start region boundaries. It worries me that the patch is assymmetric, and ideally I would have a test case which fails for start boundary and the fix for it. If anyone can help to come up with such test case, that would be so great! -- To view, visit https://review.coreboot.org/c/flashrom/+/78984?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I7f78a0090065cd2a952cba1a5d28179483ba4c55 Gerrit-Change-Number: 78984 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 09 Nov 2023 13:01:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: tests: Add test cases for erasing/writing unaligned layout regions
by Anastasia Klimchuk (Code Review) 09 Nov '23

09 Nov '23

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/78985?usp=email ) Change subject: tests: Add test cases for erasing/writing unaligned layout regions ...................................................................... tests: Add test cases for erasing/writing unaligned layout regions The test cases from #16 onwards cover the case when layout region is not aligned with eraseblock region, and therefore either layout start boundary or end boundary needs to be extended to align with eraseblock. Ticket: https://ticket.coreboot.org/issues/494 Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M tests/erase_func_algo.c 1 file changed, 91 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/78985/1 diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index 824a03a..029f4a8 100644 --- a/tests/erase_func_algo.c +++ b/tests/erase_func_algo.c @@ -28,7 +28,7 @@ #define ERASE_VALUE 0xff #define MOCK_CHIP_SIZE 16 /* How many small chip test cases. */ -#define TEST_CASES_NUM 16 +#define TEST_CASES_NUM 19 #define MIN_BUF_SIZE 1024 /* Minimum buffer size flashrom operates for chip operations. */ #define MIN_REAL_CHIP_SIZE 1024 /* Minimum chip size that can be defined for real chip in flashchips */ @@ -169,6 +169,26 @@ }, }; +static struct flashchip chip_8_16 = { + .vendor = "aklm", + /* See comment on previous chip. */ + .total_size = 1, + .tested = TEST_OK_PREW, + .gran = WRITE_GRAN_1BYTE, + .read = TEST_READ_INJECTOR, + .write = TEST_WRITE_INJECTOR, + .block_erasers = + { + { + .eraseblocks = { {8, MIN_REAL_CHIP_SIZE / 8} }, + .block_erase = TEST_ERASE_INJECTOR, + }, { + .eraseblocks = { {16, MIN_REAL_CHIP_SIZE / 16} }, + .block_erase = TEST_ERASE_INJECTOR, + } + }, +}; + static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, const char *programmer_param, struct test_case *current_test_case) { @@ -230,8 +250,9 @@ /* * Setup all test cases. * - * First half of test cases (0 - (size(testcases)/2-1)) set up for a chip with erasers: 1, 2, 4, 8, 16 bytes. - * Second half (size(testcases)/2 - size(testcases)) repeates the same test cases for a chip with erasers: 1, 8, 16 bytes. + * First half of test cases is set up for a chip with erasers: 1, 2, 4, 8, 16 bytes. + * Second half repeates the same test cases for a chip with erasers: 1, 8, 16 bytes. + * Last three test cases use the chip with erasers: 8, 16 bytes, to test unaligned layout regions. */ static struct test_case test_cases[] = { { @@ -593,6 +614,73 @@ .eraseblocks_expected_ind = 16, .erase_test_name = "Erase test case #15", .write_test_name = "Write test case #15", + }, { + /* + * Test case #16 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned regions 8+4+4b which are smaller than the smallest eraseblock. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_8_16, + .regions = {{0, 7, "reg8"}, {8, 11, "reg4_a"}, + {12, MIN_REAL_CHIP_SIZE - 1, "reg4_b"}}, + .initial_buf = {0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, + 0x5, 0x5, 0x5, 0x5, 0x6, 0x6, 0x6, 0x6}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x14, 0x14, 0x14, 0x14, 0x14, 0x14, 0x14, 0x14, + 0x15, 0x15, 0x15, 0x15, 0x16, 0x16, 0x16, 0x16}, + .eraseblocks_expected = {{0x8, 0x8}, {0x8, 0x8}, {0x0, 0x8}}, // TODO setup correct ones! these are temporary + .eraseblocks_expected_ind = 3, // TODO setup correct ones! these are temporary + .erase_test_name = "Erase test case #16", + .write_test_name = "Write test case #16", + }, { + /* + * Test case #17 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned region 3+13b which are smaller than the smallest eraseblock. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_8_16, + .regions = {{0, 2, "reg3"}, {3, MIN_REAL_CHIP_SIZE - 1, "tail"}}, + .initial_buf = {0x4, 0x4, 0x4, 0x6, 0x6, 0x6, 0x6, 0x6, + 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x14, 0x14, 0x14, 0x16, 0x16, 0x16, 0x16, 0x16, + 0x16, 0x16, 0x16, 0x16, 0x16, 0x16, 0x16, 0x16}, + .eraseblocks_expected = {{0x0, 0x10}, {0x0, 0x8}}, // TODO setup correct ones! these are temporary + .eraseblocks_expected_ind = 2, // TODO setup correct ones! these are temporary + .erase_test_name = "Erase test case #17", + .write_test_name = "Write test case #17", + }, { + /* + * Test case #18 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned region 9+7b. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_8_16, + .regions = {{0, 8, "reg9"}, {9, MIN_REAL_CHIP_SIZE - 1, "tail"}}, + .initial_buf = {0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, + 0x4, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, + .written_buf = {0x14, 0x14, 0x14, 0x14, 0x14, 0x14, 0x14, 0x14, + 0x14, 0x16, 0x16, 0x16, 0x16, 0x16, 0x16, 0x16}, + .eraseblocks_expected = {{0x8, 0x8}, {0x0, 0x10}}, // TODO setup correct ones! these are temporary + .eraseblocks_expected_ind = 2, // TODO setup correct ones! these are temporary + .erase_test_name = "Erase test case #18", + .write_test_name = "Write test case #18", }, }; -- To view, visit https://review.coreboot.org/c/flashrom/+/78985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I726a30b0e47a966e8093ddc19abf4a65fb1d70ce Gerrit-Change-Number: 78985 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2023