[PATCH 5/5 v2] mips: octeon: tools: Add update_octeon_header tool
Stefan Roese
sr at denx.de
Mon Nov 30 11:27:43 CET 2020
Hi Daniel,
On 27.11.20 20:01, Daniel Schwierzeck wrote:
> Am Mittwoch, den 28.10.2020, 15:10 +0100 schrieb Stefan Roese:
>> Add a tool to update or insert an Octeon specific header into the U-Boot
>> image. This is needed e.g. for booting via SPI NOR, eMMC and NAND.
>>
>> While working on this, move enum cvmx_board_types_enum and
>> cvmx_board_type_to_string() to cvmx-bootloader.h and remove the
>> unreferenced (unsupported) board definition.
>>
>> Signed-off-by: Stefan Roese <sr at denx.de>
>> Cc: Aaron Williams <awilliams at marvell.com>
>> Cc: Chandrakala Chavva <cchavva at marvell.com>
>> Cc: Daniel Schwierzeck <daniel.schwierzeck at gmail.com>
>> ---
>> v2:
>> - No change
>> - Azure build success here:
>> https://dev.azure.com/sr0718/u-boot/_build/results?buildId=59&view=results
>>
>> .../mach-octeon/include/mach/cvmx-bootinfo.h | 222 ---------
>> .../include/mach/cvmx-bootloader.h | 172 +++++++
>> tools/Makefile | 3 +
>> tools/update_octeon_header.c | 450 ++++++++++++++++++
>> 4 files changed, 625 insertions(+), 222 deletions(-)
>> create mode 100644 arch/mips/mach-octeon/include/mach/cvmx-bootloader.h
>> create mode 100644 tools/update_octeon_header.c
>>
> ...
>
>> diff --git a/tools/Makefile b/tools/Makefile
>> index 51123fd929..253a6b9706 100644
>> --- a/tools/Makefile
>> +++ b/tools/Makefile
>> @@ -206,6 +206,9 @@ hostprogs-y += proftool
>> hostprogs-$(CONFIG_STATIC_RELA) += relocate-rela
>> hostprogs-$(CONFIG_RISCV) += prelink-riscv
>>
>> +hostprogs-$(CONFIG_ARCH_OCTEON) += update_octeon_header
>> +update_octeon_header-objs := update_octeon_header.o lib/crc32.o
>
> entry in tools/.gitignore is missing
>
>> +
>> hostprogs-y += fdtgrep
>> fdtgrep-objs += $(LIBFDT_OBJS) common/fdt_region.o fdtgrep.o
>>
>> diff --git a/tools/update_octeon_header.c b/tools/update_octeon_header.c
>> new file mode 100644
>> index 0000000000..964036216d
>> --- /dev/null
>> +++ b/tools/update_octeon_header.c
>> @@ -0,0 +1,450 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (C) 2020 Marvell International Ltd.
>> + */
>> +
>> +#include <stdio.h>
>> +#include <stdint.h>
>> +#include <stddef.h>
>> +#include <sys/types.h>
>> +#include <sys/stat.h>
>> +#include <fcntl.h>
>> +#include <unistd.h>
>> +#include <stdlib.h>
>> +#include <string.h>
>> +#include <getopt.h>
>> +#include <arpa/inet.h>
>> +
>> +#include <u-boot/crc.h>
>> +
>> +#include "mkimage.h"
>> +
>> +#include "../arch/mips/mach-octeon/include/mach/cvmx-bootloader.h"
>> +
>> +void usage(void);
>
> this needs some code cleanup:
> - drop the forward declarations and reorder the functions when
> necessary
> - make all functions static
> - drop dead code
> - drop debug() macros convert the useful ones to printf
> - maybe use fprintf(STDERR, ...) for error messages
>
> there are also some bugs:
> - tool crashes with segfault when called without arguments
> - tool crashes when called with a long option but no value
> - the bugs above will also be detected by clang-tidy, suggestions for
> fixing below
Many thanks for the extensive code review. I'll try to address all
comments shortly.
Thanks,
Stefan
>> +
>> +/*
>> + * Do some funky stuff here to get a compile time error if the size of the
>> + * bootloader_header structure has changed.
>> + */
>> +#define compile_time_assert(cond, msg) char msg[(cond) ? 1 : -1]
>> +compile_time_assert(sizeof(struct bootloader_header) == 192,
>> + bootloader_header_size_changed);
>
> there is BUILD_BUG_ON_MSG() in linux/build_bug.h
> or compiletime_assert() in linux/compiler.h
>
>> +
>> +#define BUF_SIZE (16 * 1024)
>> +#define NAME_LEN 100
>> +
>> +#ifndef WOFFSETOF
>> +/* word offset */
>> +#define WOFFSETOF(type, elem) (offsetof(type, elem) / 4)
>> +#endif
>> +
>> +#define cvmx_cpu_to_be64(x) (x)
>> +
>> +static int stage2_flag;
>> +static int stage_1_5_flag;
>> +static int stage_1_flag;
>> +
>> +int lookup_board_type(char *board_name)
>> +{
>> + int i;
>> + int board_type = 0;
>> + char *substr = NULL;
>> +
>> + /* Detect stage 2 bootloader boards */
>> + if (strcasestr(board_name, "_stage2")) {
>> + debug("Stage 2 bootloader detected from substring %s in name %s\n",
>> + "_stage2", board_name);
>> + stage2_flag = 1;
>> + } else {
>> + debug("Stage 2 bootloader NOT detected from name \"%s\"\n",
>> + board_name);
>> + }
>> +
>> + if (strcasestr(board_name, "_stage1")) {
>> + debug("Stage 1 bootloader detected from substring %s in name %s\n",
>> + "_stage1", board_name);
>> + stage_1_flag = 1;
>> + }
>> +
>> + /* Generic is a special case since there are numerous sub-types */
>> + if (!strncasecmp("generic", board_name, strlen("generic")))
>> + return CVMX_BOARD_TYPE_GENERIC;
>> +
>> + /*
>> + * If we're an eMMC stage 2 bootloader, cut off the _emmc_stage2
>> + * part of the name.
>> + */
>> + substr = strcasestr(board_name, "_emmc_stage2");
>> + if (substr && (substr[strlen("_emmc_stage2")] == '\0')) {
>> + /*return CVMX_BOARD_TYPE_GENERIC;*/
>> +
>> + printf(" Converting board name %s to ", board_name);
>> + *substr = '\0';
>> + printf("%s\n", board_name);
>> + }
>> +
>> + /*
>> + * If we're a NAND stage 2 bootloader, cut off the _nand_stage2
>> + * part of the name.
>> + */
>> + substr = strcasestr(board_name, "_nand_stage2");
>> + if (substr && (substr[strlen("_nand_stage2")] == '\0')) {
>> + /*return CVMX_BOARD_TYPE_GENERIC;*/
>> +
>> + printf(" Converting board name %s to ", board_name);
>> + *substr = '\0';
>> + printf("%s\n", board_name);
>> + }
>> +
>> + /*
>> + * If we're a SPI stage 2 bootloader, cut off the _spi_stage2
>> + * part of the name.
>> + */
>> + substr = strcasestr(board_name, "_spi_stage2");
>> + if (substr && (substr[strlen("_spi_stage2")] == '\0')) {
>> + printf(" Converting board name %s to ", board_name);
>> + *substr = '\0';
>> + printf("%s\n", board_name);
>> + }
>> +
>> + for (i = CVMX_BOARD_TYPE_NULL; i < CVMX_BOARD_TYPE_MAX; i++)
>> + if (!strcasecmp(cvmx_board_type_to_string(i), board_name))
>> + board_type = i;
>> +
>> + for (i = CVMX_BOARD_TYPE_CUST_DEFINED_MIN;
>> + i < CVMX_BOARD_TYPE_CUST_DEFINED_MAX; i++)
>> + if (!strncasecmp(cvmx_board_type_to_string(i), board_name,
>> + strlen(cvmx_board_type_to_string(i))))
>> + board_type = i;
>> +
>> + for (i = CVMX_BOARD_TYPE_CUST_PRIVATE_MIN;
>> + i < CVMX_BOARD_TYPE_CUST_PRIVATE_MAX; i++)
>> + if (!strncasecmp(cvmx_board_type_to_string(i), board_name,
>> + strlen(cvmx_board_type_to_string(i))))
>> + board_type = i;
>> +
>> + return board_type;
>> +}
>> +
>> +/* Getoptions variables must be global */
>> +static int failsafe_flag;
>> +static int pciboot_flag;
>> +static int env_flag;
>> +
>> +int main(int argc, char *argv[])
>> +{
>> + int fd;
>> + uint8_t buf[BUF_SIZE];
>> + uint32_t data_crc = 0;
>> + int len;
>> + int data_len = 0;
>> + struct bootloader_header header;
>> + char filename[NAME_LEN];
>> + int i;
>> + int option_index = 0; /* getopt_long stores the option index here. */
>> + char board_name[NAME_LEN] = { 0 };
>> + char tmp_board_name[NAME_LEN] = { 0 };
>> + int c;
>> + int board_type = 0;
>> + unsigned long long address = 0;
>> + ssize_t ret;
>> + const char *type_str = NULL;
>> + int hdr_size = sizeof(struct bootloader_header);
>
> the tool has two positional arguments, so you should bail out early.
> This will also fix the segfault
>
> if (argc < 3) {
> usage();
> return -1;
> }
>
>> +
>> + debug("header size is: %d bytes\n", hdr_size);
>> +
>> + /* Parse command line options using getopt_long */
>> + while (1) {
>> + static struct option long_options[] = {
>> + /* These options set a flag. */
>> + {"failsafe", no_argument, &failsafe_flag, 1},
>> + {"pciboot", no_argument, &pciboot_flag, 1},
>> + {"nandstage2", no_argument, &stage2_flag, 1},
>> + {"spistage2", no_argument, &stage2_flag, 1},
>> + {"norstage2", no_argument, &stage2_flag, 1},
>> + {"stage2", no_argument, &stage2_flag, 1},
>> + {"stage1.5", no_argument, &stage_1_5_flag, 1},
>> + {"stage1", no_argument, &stage_1_flag, 1},
>> + {"environment", no_argument, &env_flag, 1},
>> + /* These options don't set a flag.
>> + * We distinguish them by their indices.
>> + */
>> + {"board", required_argument, 0, 0},
>> + {"text_base", required_argument, 0, 0},
>> + {0, 0, 0, 0}
>> + };
>
> this should be moved out of main() and marked as static const
>
>> +
>> + c = getopt_long(argc, (char *const *)argv, "h",
>
> cast is not needed
>
>> + long_options, &option_index);
>> +
>> + /* Detect the end of the options. */
>> + if (c == -1)
>> + break;
>> +
>> + switch (c) {
>> + /* All long options handled in case 0 */
>> + case 0:
>> + /* If this option set a flag, do nothing else now. */
>> + if (long_options[option_index].flag != 0)
>> + break;
>> + debug("option(l) %s", long_options[option_index].name);
>> + if (optarg)
>> + debug(" with arg %s", optarg);
>> + debug("\n");
>
> should be
>
> if (!optarg) {
> usage();
> return -1;
> }
> debug(" with arg %s\n", optarg);
>
> the code below depends on optarg and passing a long option without
> value is an error and should be notified to the user
>
>> +
>> + if (!strcmp(long_options[option_index].name, "board")) {
>> + if (strlen(optarg) >= NAME_LEN) {
>> + printf("strncpy() issue detected!");
>> + exit(-1);
>> + }
>> + strncpy(board_name, optarg, NAME_LEN);
>> +
>> + debug("Using user supplied board name: %s\n",
>> + board_name);
>> + } else if (!strcmp(long_options[option_index].name,
>> + "text_base")) {
>> + address = strtoull(optarg, NULL, 0);
>> + debug("Address of image is: 0x%llx\n",
>> + (unsigned long long)address);
>> + if (!(address & 0xFFFFFFFFULL << 32)) {
>> + if (address & 1 << 31) {
>> + address |= 0xFFFFFFFFULL << 32;
>> + debug("Converting address to 64 bit compatibility space: 0x%llx\n",
>> + address);
>> + }
>> + }
>> + }
>> + break;
>> +
>> + case 'h':
>> + case '?':
>> + /* getopt_long already printed an error message. */
>> + usage();
>> + return -1;
>> +
>> + default:
>> + abort();
>> + }
>> + }
>> +
>> + if (optind < argc) {
>> + /*
>> + * We only support one argument - an optional bootloader
>> + * file name
>> + */
>> + if (argc - optind > 2) {
>> + debug("non-option ARGV-elements: ");
>> + while (optind < argc)
>> + debug("%s ", argv[optind++]);
>> + debug("\n");
>> + usage();
>> + return -1;
>> + }
>> + }
>> +
>> + if (strlen(argv[optind]) >= NAME_LEN) {
>> + printf("strncpy() issue detected!");
>> + exit(-1);
>> + }
>> + strncpy(filename, argv[optind], NAME_LEN);
>> +
>> + if (board_name[0] == '\0') {
>> + if (strlen(argv[optind + 1]) >= NAME_LEN) {
>> + printf("strncpy() issue detected!");
>> + exit(-1);
>> + }
>> + strncpy(board_name, argv[optind + 1], NAME_LEN);
>> + }
>> +
>> + if (strlen(board_name) >= NAME_LEN) {
>> + printf("strncpy() issue detected!");
>> + exit(-1);
>> + }
>> + strncpy(tmp_board_name, board_name, NAME_LEN);
>> +
>> + fd = open(filename, O_RDWR);
>> + if (fd < 0) {
>> + printf("Unable to open file: %s\n", filename);
>> + exit(-1);
>> + }
>> +
>> + if (failsafe_flag)
>> + debug("Setting failsafe flag\n");
>> +
>> + if (strlen(board_name)) {
>> + int offset = 0;
>> +
>> + debug("Supplied board name of: %s\n", board_name);
>> +
>> + if (strstr(board_name, "failsafe")) {
>> + failsafe_flag = 1;
>> + debug("Setting failsafe flag based on board name\n");
>> + }
>> + /* Skip leading octeon_ if present. */
>> + if (!strncmp(board_name, "octeon_", 7))
>> + offset = 7;
>> +
>> + /*
>> + * Check to see if 'failsafe' is in the name. If so, set the
>> + * failsafe flag. Also, ignore extra trailing characters on
>> + * passed parameter when comparing against board names.
>> + * We actually use the configuration name from u-boot, so it
>> + * may have some other variant names. Variants other than
>> + * failsafe _must_ be passed to this program explicitly
>> + */
>> +
>> + board_type = lookup_board_type(board_name + offset);
>> + if (!board_type) {
>> + /* Retry with 'cust_' prefix to catch boards that are
>> + * in the customer section (such as nb5)
>> + */
>> + sprintf(tmp_board_name, "cust_%s", board_name + offset);
>> + board_type = lookup_board_type(tmp_board_name);
>> + }
>> +
>> + /* reset to original value */
>> + strncpy(tmp_board_name, board_name, NAME_LEN);
>> + if (!board_type) {
>> + /*
>> + * Retry with 'cust_private_' prefix to catch boards
>> + * that are in the customer private section
>> + */
>> + sprintf(tmp_board_name, "cust_private_%s",
>> + board_name + offset);
>> + board_type = lookup_board_type(tmp_board_name);
>> + }
>> +
>> + if (!board_type) {
>> + printf("ERROR: unable to determine board type\n");
>> + exit(-1);
>> + }
>> + debug("Board type is: %d: %s\n", board_type,
>> + cvmx_board_type_to_string(board_type));
>> + } else {
>> + printf("Board name must be specified!\n");
>> + exit(-1);
>> + }
>> +
>> + /*
>> + * Check to see if there is either an existing header, or that there
>> + * are zero valued bytes where we want to put the header
>> + */
>> + len = read(fd, buf, BUF_SIZE);
>> + if (len > 0) {
>> + /*
>> + * Copy the header, as the first word (jump instruction, needs
>> + * to remain the same.
>> + */
>> + memcpy(&header, buf, hdr_size);
>> + /*
>> + * Check to see if we have zero bytes (excluding first 4, which
>> + * are the jump instruction)
>> + */
>> + for (i = 1; i < hdr_size / 4; i++) {
>> + if (((uint32_t *)buf)[i]) {
>> + printf("ERROR: non-zero word found %x in location %d required for header, aborting\n",
>> + ((uint32_t *)buf)[i], i);
>> + exit(-1);
>> + }
>> + }
>> + debug("Zero bytes found in header location, adding header.\n");
>> +
>> + } else {
>> + printf("Unable to read from file %s\n", filename);
>> + exit(-1);
>> + }
>> +
>> + /* Read data bytes and generate CRC */
>> + lseek(fd, hdr_size, SEEK_SET);
>> +
>> + while ((len = read(fd, buf, BUF_SIZE)) > 0) {
>> + data_crc = crc32(data_crc, buf, len);
>> + data_len += len;
>> + }
>> + printf("CRC of data: 0x%x, length: %d\n", data_crc, data_len);
>> +
>> + /* Now create the new header */
>> + header.magic = htonl(BOOTLOADER_HEADER_MAGIC);
>> + header.maj_rev = htons(BOOTLOADER_HEADER_CURRENT_MAJOR_REV);
>> + header.min_rev = htons(BOOTLOADER_HEADER_CURRENT_MINOR_REV);
>> + header.dlen = htonl(data_len);
>> + header.dcrc = htonl(data_crc);
>> + header.board_type = htons(board_type);
>> + header.address = cvmx_cpu_to_be64(address);
>
> cvmx_cpu_to_be64() is a no-op, is this intentional? If yes, this macro
> can be removed. Otherwise you can use cpu_to_be64(). All cpu_to_be*()
> functions are automatically available in tools/ directory.
>
>
Viele Grüße,
Stefan
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de
More information about the U-Boot
mailing list