[PATCH v1 1/1] Platform: Add support for Arm64 UEFI-based Platform
Ge Song
songgebird at gmail.com
Mon Apr 9 12:15:07 AEST 2018
Hi Geoff,
Sorry for the so delayed response and thanks to you for
your kind and patient review!
It's not convenient to use Microsoft outlook for discussion,
Enabling IMAP service in company's mail server is still in progress,
and I don't know when it will be ready. For now maybe gmail is a
good choice for the purpose.
Most parts you indicated will be fixed in V2 patch. Besides,
Maybe it's better to divide the changes into two parts:
One for manipulating efi variables(lib/efi*, test/lib/test-efi.c),
another is for platform related.
After double checking the V2 patch, hope it has no such obvious
mistakes like the previous one.
Thanks again!
On 03/30/2018 01:59 AM, Geoff Levand wrote:
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.
There are several routines can be moved to a common file directly,
but it seems not a good solution that just moving them somewhere.
Since I'm a firmware engineer and for the moment still haven't find a
preferable way to solve it.
Of course if you think currently moving them to a common file is acceptable,
I will do it in next patch.
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>
<mailto: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(-)
It will be added in patch v2.
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 will be removed next time
+ *
+ * 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
<https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g>
02111
<https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g>-1307
USA
+ *
+ * Copyright (C) 2018 Huaxintong Semiconductor Technology
Co.,Ltd. All rights
This line and others like it have trailing whitespace.
Sorry for it……
+ * reserved.
+ * Copyright (C) 2018 Ge Song <ge.song at hxt-semitech.com>
<mailto: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.
Thanks for pointing out it! It will be fixed in V2 patch
+ */
+#ifndef EFIVAR_H
+#define EFIVAR_H 1
Other files use just '#define EFIVAR_H'.
Sure, thanks.
+
+#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/";)
Thanks, this is the better way:-)
+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)?
Sounds like a good a idea.:-)
+
+#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.
In fact, though the file name is platform-arm64.c, petitboot on x86/x64
platforms can works properly when the patch is applied. Just a reminder,
in case people may be misled by its name.
+ *
+ * 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
<https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g>
02111
<https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g>-1307
USA
+ *
+ * Copyright (C) 2018 Huaxintong Semiconductor Technology
Co.,Ltd. All rights
+ * reserved.
+ * Copyright (C) 2018 Ge Song <ge.song at hxt-semitech.com>
<mailto: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.
OK, these two will be revised next time.
+#define PETITBOOT_VARS_GUID "fb78ab4b-bd43-41a0-99a2-4e74bef9169b"
Can this be a const char array?
Sure, const char array is the better way.
+
+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.
This will be revised in patch V2.
+};
+
+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.
Perhaps compiler could handle it properly, I still think indicating it
explicitly is not bad ….
+{
+ 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...
This will be revised in patch V2.
+
+ 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.
OK, all the similar problems will be fixed in V2 patch
+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?
In fact, variables stored in EFI NVRAM will have their own namespace in the
form of Var-GUID. For example, "timeout" will be presented as
"timeout-fb78ab4b-bd43-41a0-99a2-4e74bef9169b" in efivarfs. Just to save
the effort to tackle the redundant "petitboot," and NVRAM space to store it.
Besides, I think config options may differ between platforms in future, so
maybe it's reasonable for platform to hold and handle its own config
options.
+ 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...
OK, about all the similar problem you mentioned, I just want to
keep the same form with functions in this platform-powerpc.c,
will fix them in V2 patch.
+
+ 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?
Of course.
+#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.
These will be revised in patch v2
+{
+ int fd, flag, errno_value;
+ int rc = -1;
It doesn't seem this initial value is ever used.
I think if "open" or "malloc" operation fails, this initial value will
return to its caller.
+ 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.
Thanks for pointing out it.
+ 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...
These will be removed next time...
+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.
In fact "tab" was used in this file for separation between the type and
the name.
May be option "expandtab" in vim changed occasionally.
-Geoff
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.ozlabs.org/pipermail/petitboot/attachments/20180409/024d7d62/attachment-0001.html>
More information about the Petitboot
mailing list