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

[Paul Menzel]

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