Re: [PATCH v2 2/2] efi: SPI NOR flash command line

[Paul Menzel <pmenzel@molgen.mpg.de>]

Dear Michael,


Am 12.02.21 um 10:20 schrieb Michael Lawnick:

> Note: There is a bug in code below which will let this code fail on
> BIOS-UEFI and requires a v3 as soon as basic discussion whether this
> sort of functionality is accepted at all is done.
> I don't want to disturb thread by a v3 thread.

Thank you for preparing and upstreaming the patches.

I think, I read, that the maintainers should be added to Cc for patches, 
but as written, and as seen from the thread *RFC: Grub project 
management*, GRUB definitely has some resource problems. So, if you 
could send the v3 with the maintainers in Cc, that would be great.

[…]

> Am 05.02.2021 um 10:02 schrieb Michael Lawnick:
>> Add SPI NOR flash to command line
>> Based on patch '[PATCH 1/2] efi: SPI NOR flash support'
>> add command line functionality for interactive access
>> to SPI NOR flash.
>>
>> Supported commands:
>> spi_nor
>>        init  - establish communication to a flash part
>>        read  - read from flash part to memory or print hexdump
>>        write - write to flash part from memory
>>        erase - erase some erase blocks

Please name the system (board and firmware) you tested this with, and 
maybe give an example, how to test this. Is QEMU able to emulate a SPI 
NOR flash chip?

>> Signed-off-by: Michael Lawnick <michael.lawnick@nokia.com>
>> ---
>> [Patch v2 2/2]: no change
>> ---
>> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
>> index 4d775e5f6..403a5432f 100644
>> --- a/grub-core/Makefile.core.def
>> +++ b/grub-core/Makefile.core.def
>> @@ -659,6 +659,7 @@ module = {
>>    module = {
>>      name = efi_spi_nor;
>>      common = bus/spi/efi_spi_nor.c;
>> +  common = commands/efi/spinorcmd.c;
>>      enable = efi;
>>    };
>>
>> diff --git a/grub-core/commands/efi/spinorcmd.c
>> b/grub-core/commands/efi/spinorcmd.c
>> new file mode 100644
>> index 000000000..c55a900aa
>> --- /dev/null
>> +++ b/grub-core/commands/efi/spinorcmd.c
>> @@ -0,0 +1,253 @@
>> +/*  spinorcmd.c  - Give access to SPI NOR flash on command line.
>> + *  Copyright 2021 Nokia
>> + *  Licensed under the GNU General Public License v3.0 only
>> + *  SPDX-License-Identifier: GPL-3.0-only
>> + *
>> + *  GRUB  --  GRand Unified Bootloader
>> + *  Copyright (C) 2008  Free Software Foundation, Inc.
>> + *
>> + *  GRUB 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, either version 3 of the License, or
>> + *  (at your option) any later version.
>> + *
>> + *  GRUB 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.
>> + *
>> + *  You should have received a copy of the GNU General Public License
>> + *  along with GRUB.  If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <grub/types.h>
>> +#include <grub/misc.h>
>> +#include <grub/mm.h>
>> +#include <grub/efi/api.h>
>> +#include <grub/efi/efi.h>
>> +#include <grub/efi/efi_spi_nor.h>
>> +#include <grub/command.h>
>> +
>> +GRUB_MOD_LICENSE ("GPLv3+");
>> +
>> +static void *cli_spi_flash_device = NULL;
>> +
>> +static void
>> +usage (void)
>> +{
>> +    grub_printf("spi_nor - access SPI NOR flash through UEFI API\n");
>> +    grub_printf("Usage:\n");
>> +    grub_printf("  spi_nor init|read|write|format <args>\n");
>> +    grub_printf("      init [-id <id0 id1 id2>] [-cnt <count>]\n");
>> +    grub_printf("             to be called once before operation on a device.\n");
>> +    grub_printf("             <id>    : optional up to 3 bytes flash identifier\n");
>> +    grub_printf("                       to match against\n");
>> +    grub_printf("             <count> : use n-th occurance of  device\n");
>> +    grub_printf("                       (can be combined with <id>)\n");
>> +    grub_printf("      read <offset> <bytes> [<addr>]\n");
>> +    grub_printf("             read and dump/save bytes\n");
>> +    grub_printf("      write <offset> <bytes> <addr>\n");
>> +    grub_printf("             write bytes from <addr> to flash\n");
>> +    grub_printf("      erase <offset> <bytes>\n");
>> +    grub_printf("             format area \n");
>> +    grub_printf("             (<offset> and <bytes> must be erase block alligned)\n");

aligned

>> +    grub_printf("\n");
>> +}
>> +
>> +/*  get_init_args - return index and number of -id and/or -cnt arguments
>> +    handle 4 possible inputs:
>> +    - spi_nor init -id <id0> <id1> -cnt <count>
>> +    - spi_nor init -cnt <count> -id <id0>
>> +    - spi_nor init -cnt <count>
>> +    - spi_nor init -id <id0> <id1> <id2>
>> +*/
>> +static int get_init_args(int argc, char **args, int *id_idx, int
>> *id_cnt, int *cnt_idx)
>> +{
>> +    int opt_idx = 1;
>> +    *id_idx = 0;
>> +    *cnt_idx = 0;
>> +    *id_cnt = 0;
>> +
>> +    while (opt_idx < argc) {
>> +        if (!grub_strcmp(args[opt_idx],"-id")) {

Please add a space after the comma.

>> +            opt_idx++;
>> +            *id_idx = opt_idx;
>> +
>> +            while ((opt_idx < argc)
>> +                    && (grub_strcmp(args[opt_idx], "-cnt"))
>> +                    && (*id_cnt < 3)) {
>> +                opt_idx++;
>> +                *id_cnt = *id_cnt + 1;
>> +            }
>> +
>> +            if (*id_cnt == 0)
>> +                return 1;
>> +        } else if (!grub_strcmp(args[opt_idx],"-cnt")) {
>> +            if (argc > opt_idx + 1) {
>> +                *cnt_idx = opt_idx + 1;
>> +                opt_idx += 2;
>> +            } else {
>> +                return 1;
>> +            }
>> +        } else {
>> +            return 1;
>> +        }
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static grub_err_t
>> +grub_cmd_spi_nor (grub_command_t cmd __attribute__ ((unused)),
>> +            int argc __attribute__ ((unused)),
>> +            char **args __attribute__ ((unused)))
>> +{
>> +    grub_err_t ret;
>> +    int cnt_idx, id_idx, id_cnt = 0;
>> +    grub_uint8_t *device_id = NULL, devId[3];
>> +    grub_uint32_t instance = 0;
>> +
>> +    if (argc < 1) {
>> +        grub_printf("Missing argument\n");
>> +        usage();
>> +        return GRUB_ERR_BAD_ARGUMENT;
>> +    }
>> +
>> +    if (!grub_strcmp("init", args[0])) {
>> +        if (argc != 1) {
>> +            if (get_init_args(argc, args, &id_idx, &id_cnt, &cnt_idx)) {
>> +                usage();
>> +                return GRUB_ERR_BAD_ARGUMENT;
>> +            } else {
>> +                if (id_idx != 0) {
>> +                    for (int i=0; i<id_cnt; i++)
>> +                        devId[i] = grub_strtoul(args[id_idx + i], NULL, 0);
>> +                    device_id = devId;
>> +                }
>> +                if (cnt_idx != 0) {
>> +                    instance = grub_strtoul(args[cnt_idx], NULL, 0);
>> +                }
>> +            }
>> +        }
>> +
>> +        cli_spi_flash_device = grub_efi_spi_nor_init(device_id, id_cnt, instance);
>> +        if (cli_spi_flash_device == NULL) {
>> +            grub_printf("No SPI NOR flash found\n");
>> +            return GRUB_ERR_UNKNOWN_DEVICE;
>> +        }
>> +        grub_printf("Found dev %x, capacity 0x%x bytes, erase block size 0x%x\n",
>> +                    grub_efi_spi_nor_device_id(cli_spi_flash_device),
>> +                    grub_efi_spi_nor_flash_size(cli_spi_flash_device),
>> +                    grub_efi_spi_nor_erase_block_size(cli_spi_flash_device));
>> +        return GRUB_ERR_NONE;
>> +    }
>> +
>> +    if (cli_spi_flash_device == NULL) {
>> +        grub_printf("No known device. Call 'init' first\n");
>> +        usage();
>> +        return GRUB_ERR_UNKNOWN_DEVICE;
>> +    }
>> +
>> +    if (!grub_strcmp("read", args[0])) {
>> +        grub_uint8_t *data;
>> +        grub_uint32_t offset, num_bytes, i, j;
>> +
>> +        if (argc < 3) {
>> +            grub_printf("Missing parameters\n");
>> +            usage();
>> +            return GRUB_ERR_BAD_ARGUMENT;
>> +        }
>> +
>> +        offset = grub_strtoul(args[1], NULL, 0);
>> +        num_bytes = grub_strtoul(args[2], NULL, 0);
>> +
>> +        if (argc == 4) {
>> +            data = (grub_uint8_t *)grub_strtoul(args[3], NULL, 0);
>> +            if (data == NULL) {
>> +                grub_printf("Bad memory pointer, 0 not supported.\n");
>> +                usage();
>> +                return GRUB_ERR_BAD_ARGUMENT;
>> +            }
>> +        } else {
>> +            data = grub_malloc(num_bytes);
>> +            if (data == NULL) {
>> +                grub_printf("Out of memory.\n");
>> +                usage();
>> +                return GRUB_ERR_OUT_OF_MEMORY;
>> +            }
>> +        }
>> +
>> +        ret = grub_efi_spi_nor_read(cli_spi_flash_device, data, offset, num_bytes);
>> +        if (ret != GRUB_ERR_NONE)
>> +            return ret;
>> +
>> +        if (argc == 3) {
>> +            for (i=0; i<num_bytes; i+=16) {
>> +                grub_printf("0x%06x: ", i + offset);
>> +                for (j=0; (j<16) && (i+j<num_bytes); j++)
>> +                    grub_printf("%02x ", data[i+j]);
>> +                grub_printf("\n");
>> +            }
>> +            grub_free(data);
>> +        }
>> +        return GRUB_ERR_NONE;
>> +    }
>> +
>> +    if (!grub_strcmp("write", args[0])) {
>> +        grub_uint8_t *data;
>> +        grub_uint32_t offset, num_bytes;
>> +
>> +        if (argc != 4) {
>> +            grub_printf("Wrong number of parameters\n");
>> +            usage();
>> +            return GRUB_ERR_BAD_ARGUMENT;
>> +        }
>> +
>> +        offset = grub_strtoul(args[1], NULL, 0);
>> +        num_bytes = grub_strtoul(args[2], NULL, 0);
>> +
>> +        data = (grub_uint8_t *)grub_strtoul(args[3], NULL, 0);
>> +        if (data == NULL) {
>> +            grub_printf("Bad memory pointer, 0 not supported.\n");
>> +            usage();
>> +            return GRUB_ERR_BAD_ARGUMENT;
>> +        }
>> +
>> +        ret = grub_efi_spi_nor_write(cli_spi_flash_device, data, offset, num_bytes);
>> +
>> +        return ret;
>> +    }
>> +
>> +    if (!grub_strcmp("erase", args[0])) {
>> +        grub_uint32_t offset, num_bytes;
>> +
>> +        if (argc != 3) {
>> +            grub_printf("Wrong number of parameters\n");
>> +            usage();
>> +            return GRUB_ERR_BAD_ARGUMENT;
>> +        }
>> +
>> +        offset = grub_strtoul(args[1], NULL, 0);
>> +        num_bytes = grub_strtoul(args[2], NULL, 0);
>> +
>> +        ret = grub_efi_spi_nor_erase(cli_spi_flash_device, offset, num_bytes);
>> +
>> +        return ret;
>> +    }
>> +
>> +    grub_printf("Unknown command \"%s\"\n", args[1]);
>> +    usage();
>> +    return GRUB_ERR_BAD_ARGUMENT;
>> +}
>> +
>> +static grub_command_t cmd;
>> +
>> +GRUB_MOD_INIT(spinorcmd)
>> +{
>> +    cmd = grub_register_command("spi_nor", grub_cmd_spi_nor,
>> +                   "", "access SPI NOR flash");
>> +}
>> +
>> +GRUB_MOD_FINI(spinorcmd)
>> +{
>> +    grub_unregister_command(cmd);
>> +}

Sorry, I just to an superficial review, and the things commented are 
only nits (style issues).


Kind regards,

Paul