[PATCH v1 1/1] Platform: Add support for Arm64 UEFI-based Platform
Geoff Levand
geoff at infradead.org
Fri Mar 30 04:58:50 AEDT 2018
Hi Ge,
Thanks for taking time to add the EFI and arm64 support.
Please add a test program to the test/lib directory for efi
variables.
There is some duplication between platform-powerpc.c and
platform-arm64.c. For example, iface_config_str() is
identical. Please at least move identical code to a common
file. If you feel up to it, maybe rework some routines that
can be shared.
On 03/28/2018 04:29 AM, Ge Song wrote:
Please add a commit message here, maybe the 1st paragraph from
your 0/1 mail.
> Signed-off-by: Ge Song <ge.song at hxt-semitech.com>
> ---
> discover/Makefile.am | 3 +-
> lib/Makefile.am | 2 +
> lib/efi/efivar.h | 42 ++
> discover/platform-arm64.c | 751 ++++++++++++++++++++
> lib/efi/efivar.c | 183 +++++
> 5 files changed, 980 insertions(+), 1 deletion(-)
> diff --git a/lib/efi/efivar.h b/lib/efi/efivar.h
> new file mode 100644
> index 000000000000..b0efcf8aacaf
> --- /dev/null
> +++ b/lib/efi/efivar.h
> @@ -0,0 +1,42 @@
> +/*
> + * Methods to manipulation of EFI variables on UEFI-based platforms
This comment doesn't seem to have any value.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Copyright (C) 2018 Huaxintong Semiconductor Technology Co.,Ltd. All rights
This line and others like it have trailing whitespace.
> + * reserved.
> + * Copyright (C) 2018 Ge Song <ge.song at hxt-semitech.com>
I'm surprised that you could be a copyright holder of this. I've never
heard of a company that doesn't make you sign an agreement that says
that they are the owner of all you create.
> + */
> +#ifndef EFIVAR_H
> +#define EFIVAR_H 1
Other files use just '#define EFIVAR_H'.
> +
> +#include <stdint.h>
> +
> +#define EFI_VARIABLE_NON_VOLATILE 0x00000001
> +#define EFI_VARIABLE_BOOTSERVICE_ACCESS 0x00000002
> +#define EFI_VARIABLE_RUNTIME_ACCESS 0x00000004
> +#define EFI_VARIABLE_HARDWARE_ERROR_RECORD 0x00000008
> +#define EFI_VARIABLE_AUTHENTICATED_WRITE_ACCESS 0x00000010
> +#define EFI_VARIABLE_TIME_BASED_AUTHENTICATED_WRITE_ACCESS 0x00000020
> +#define EFI_VARIABLE_APPEND_WRITE 0x00000040
> +
> +#define EFIVARFS_MAGIC 0xde5e81e4
> +
> +extern char const efivarfs_path[];
Why not just have an inline? Something like:
static inline const char* efivarfs_path(void) {return "/sys/firmware/efi/efivars/";)
> +int vars_get_variable(void *ctx, char *guidstr, char *name,
> + uint8_t **data, size_t *data_size, uint32_t *attributes);
> +int vars_set_variable(void *ctx, char *guidstr, char *name,
> + uint8_t *data, size_t data_size, uint32_t attributes);
Would it be better to have these with an 'efi_' prefix (efi_get_variable)?
> +
> +#endif /* EFIVAR_H */
> diff --git a/discover/platform-arm64.c b/discover/platform-arm64.c
> new file mode 100644
> index 000000000000..4b7204228d95
> --- /dev/null
> +++ b/discover/platform-arm64.c
> @@ -0,0 +1,751 @@
> +/*
> + * Defines an arm64 platform, based on the powerpc platform.
> + *
> + * Actually, this is apply to platforms that adopt UEFI as its underlying
> + * firmware(x86/arm64).
These two comments don't seem to add any value.
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + * Copyright (C) 2018 Huaxintong Semiconductor Technology Co.,Ltd. All rights
> + * reserved.
> + * Copyright (C) 2018 Ge Song <ge.song at hxt-semitech.com>
> + */
> +#include <string.h>
> +#include <stdlib.h>
> +#include <limits.h>
> +#include <errno.h>
> +#include <sys/types.h>
> +#include <sys/fcntl.h>
> +#include <sys/stat.h>
> +#include <sys/statfs.h>
> +#include <file/file.h>
> +#include <talloc/talloc.h>
> +#include <list/list.h>
> +#include <log/log.h>
> +#include <types/types.h>
Put these in sorted order.
> +
> +#include "platform.h"
> +#include "ipmi.h"
> +#include "efi/efivar.h"
> +
> +#define DEF_ATTR EFI_VARIABLE_NON_VOLATILE | \
> + EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_ACCESS
Maybe put parenthesis () around these.
> +#define PETITBOOT_VARS_GUID "fb78ab4b-bd43-41a0-99a2-4e74bef9169b"
Can this be a const char array?
> +
> +static const int ipmi_timeout = 10000; /* milliseconds. */
> +
> +struct param {
> + char *name;
> + char *value;
> + bool modified;
> + struct list_item list;
The alignment of these is off.
> +};
> +
> +struct platform_arm64 {
> + struct list params;
> + struct ipmi *ipmi;
> +};
> +
> +static const char *known_params[] = {
> + "auto-boot?",
> + "network",
> + "timeout",
> + "bootdevs",
> + "language",
> + "debug?",
> + "write?",
> + "snapshots?",
> + "console",
> + "http_proxy",
> + "https_proxy",
> + NULL,
> +};
> +
> +#define to_platform_arm64(p) \
> + (struct platform_arm64 *)(p->platform_data)
Make this a function so type checking is done. The compiler
should inline it.
> +
> +static int parse_nvram(struct platform_arm64 *platform)
The compiler will know that this is static, so no need for
the static keyword. I see other existing code does this though.
> +{
> + int i, rc;
> + size_t size;
> + uint32_t attr;
> + uint8_t *data;
> + char *pb_guid = PETITBOOT_VARS_GUID;
> + char *known_param;
> + struct param *param;
These names are not all aligned, if that is what you were trying for...
> +
> + for (i = 0; known_params[i]; i++) {
> + known_param = known_params[i];
> + rc = vars_get_variable(platform, pb_guid, known_param,
> + &data, &size, &attr);
> + if (!rc) {
> + param = talloc(platform, struct param);
> + param->modified = false;
> + param->name = talloc_strndup(platform, known_param,
> + sizeof(known_param));
> + param->value = talloc_strdup(platform, (char *)data);
> + list_add(&platform->params, ¶m->list);
> + }
> + }
> +
> + return 0;
This will always return zero. Either add error return, or make it a
void return.
> +}
> +
> +static int write_nvram(struct platform_arm64 *platform)
> +{
> + struct param *param;
> + char *pb_guid = PETITBOOT_VARS_GUID;
> + uint32_t attr = DEF_ATTR;
> + size_t data_size;
> +
> + list_for_each_entry(&platform->params, param, list) {
> + if (param->modified) {
> + data_size = sizeof(param->value) + 1;
> + vars_set_variable(platform, pb_guid, param->name,
> + (uint8_t *)param->value, data_size, attr);
> + }
> + }
> +
> + return 0;
Either add error return, or make it a void return.
> +static void populate_network_config(struct platform_arm64 *platform,
> + struct config *config)
> +{
> + char *val, *saveptr = NULL;
> + const char *cval;
> + int i;
Here you use one space between the type and the variable name, other
places you use tabs, and in others you use multiple spaces. For
consistency, please just use a single space for all.
> +static void populate_config(struct platform_arm64 *platform,
> + struct config *config)
> +{
> + const char *val;
> + char *end;
> + unsigned long timeout;
> +
> + /* if the "auto-boot?' property is present and "false", disable auto
> + * boot */
> + val = get_param(platform, "auto-boot?");
> + config->autoboot_enabled = !val || strcmp(val, "false");
> +
> + val = get_param(platform, "timeout");
Is there some reason why we cant use the same config name as powerpc?
For example 'petitboot,timeout' here? Maybe pass in a config names
structure so this can be common to all platforms?
> + if (val) {
> + timeout = strtoul(val, &end, 10);
> + if (end != val) {
> + if (timeout >= INT_MAX)
> + timeout = INT_MAX;
> + config->autoboot_timeout_sec = (int)timeout;
> + }
> + }
> +
> + val = get_param(platform, "language");
> + config->lang = val ? talloc_strdup(config, val) : NULL;
> +
> + populate_network_config(platform, config);
> +
> + populate_bootdev_config(platform, config);
> +
> + if (!config->debug) {
> + val = get_param(platform, "debug?");
> + config->debug = val && !strcmp(val, "true");
> + }
> +
> + val = get_param(platform, "write?");
> + if (val)
> + config->allow_writes = !strcmp(val, "true");
> +
> + val = get_param(platform, "snapshots?");
> + if (val)
> + config->disable_snapshots = !strcmp(val, "false");
> +
> + val = get_param(platform, "console");
> + if (val)
> + config->boot_console = talloc_strdup(config, val);
> + /* If a full path is already set we don't want to override it */
> + config->manual_console = config->boot_console &&
> + !strchr(config->boot_console, '[');
> +
> + val = get_param(platform, "http_proxy");
> + if (val)
> + config->http_proxy = talloc_strdup(config, val);
> + val = get_param(platform, "https_proxy");
> + if (val)
> + config->https_proxy = talloc_strdup(config, val);
> +}
> +static int load_config(struct platform *p, struct config *config)
> +{
> + struct platform_arm64 *platform = to_platform_arm64(p);
> + int rc;
> +
> + rc = parse_nvram(platform);
> + if (rc)
> + pb_log("%s: Failed to parse nvram\n", __func__);
parse_nvram always returns zero, so fix something...
> +
> + populate_config(platform, config);
> +
> + get_active_consoles(config);
> +
> + return 0;
> +}
> diff --git a/lib/efi/efivar.c b/lib/efi/efivar.c
> new file mode 100644
> index 000000000000..59c2b9fe1cdf
> --- /dev/null
> +++ b/lib/efi/efivar.c
> @@ -0,0 +1,183 @@
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +#include <linux/fs.h>
> +#include <stdbool.h>
> +
> +#include <talloc/talloc.h>
Could you put these includes into sorted order?
> +#include "efivar.h"
> +
> +char const efivarfs_path[] = "/sys/firmware/efi/efivars/";
> +
> +static int vars_del_variable(void *ctx, char *guidstr, char *name)
Should those be 'const char *guidstr', 'const char *name'? Please check your
other routines for the same.
> +{
> + int fd, flag, errno_value;
> + int rc = -1;
It doesn't seem this initial value is ever used.
> + char *path;
> +
> + path = talloc_asprintf(ctx, "%s%s-%s",
> + efivarfs_path, name, guidstr);
> + if (!path)
> + return ENOMEM;
> +
> + fd = open(path, O_RDONLY|O_NONBLOCK);
> + if (fd == -1)
> + goto err;
> +
> + rc = ioctl (fd, FS_IOC_GETFLAGS, &flag);
^
Other files don't put a space after the function name, so please
remove them to be consistent.
> + if (rc == -1)
> + goto err;
> +
> + flag &= ~FS_IMMUTABLE_FL;
> + rc = ioctl (fd, FS_IOC_SETFLAGS, &flag);
> + if (rc == -1)
> + goto err;
> +
> + close(fd);
> +
> + rc = unlink(path);
> + if (rc == -1)
> + goto err;
What's happening here? A goto to the next line...
> +int vars_get_variable(void *ctx, char *guidstr, char *name,
> + uint8_t **data, size_t *data_size, uint32_t *attributes)
> +{
> + int fd, errno_value;
> + int rc = -1;
> + void *p, *buf;
> + size_t bufsize = 4096;
> + size_t filesize = 0;
> + ssize_t sz;
> + char *path;
Here you use tabs, above you use spaces. With these the variables are not lined up.
Maybe just put one space between the type and the name.
-Geoff
More information about the Petitboot
mailing list