<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=utf-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<p><span lang="EN-US">Hi Geoff,</span><br>
</p>
<span lang="EN-US">Sorry for the so delayed response and thanks to
you for</span><span lang="EN-US"><br>
your kind and patient review!</span><span lang="EN-US"><br>
<br>
It's not convenient to use Microsoft outlook for discussion, <br>
</span><span lang="EN-US"><span lang="EN-US">Enabling IMAP service
in company's mail server is still in progress,<br>
and I don't know when it will be ready. For now maybe gmail is a<br>
good choice for the purpose.<br>
</span> <br>
Most parts you indicated will be fixed in V2 patch. Besides,</span><span
lang="EN-US"><br>
Maybe it's better to divide the changes into two parts:</span><span
lang="EN-US"><br>
One for manipulating efi variables(lib/efi*, test/lib/test-efi.c),</span><span
lang="EN-US"><br>
another is for platform related.</span><span lang="EN-US"><br>
<br>
After double checking the V2 patch, hope it has no such obvious<br>
mistakes like the previous one.<br>
<br>
Thanks again!</span>
<p><span lang="EN-US"></span></p>
<p><span lang="EN-US">On 03/30/2018 01:59 AM, Geoff Levand wrote:</span>
</p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">Hi Ge,</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Thanks for taking time to add the EFI and arm64 support.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Please add a test program to the test/lib directory for efi</span></pre>
<pre><span lang="EN-US">variables.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">There is some duplication between platform-powerpc.c and</span></pre>
<pre><span lang="EN-US">platform-arm64.c. For example, iface_config_str() is</span></pre>
<pre><span lang="EN-US">identical. Please at least move identical code to a common</span></pre>
<pre><span lang="EN-US">file. If you feel up to it, maybe rework some routines that</span></pre>
<pre><span lang="EN-US">can be shared.</span></pre>
</blockquote>
<span lang="EN-US">There are several routines can be moved to a
common file directly,<br>
but it seems not a good solution that just moving them somewhere.<br>
<br>
Since I'm a firmware engineer and for the moment still haven't
find a<br>
preferable way to solve it.<br>
<br>
Of course if you think </span><span lang="EN-US"><span
lang="EN-US"><span lang="EN-US"> currently </span>moving them</span>
to a common file is acceptable,<br>
I will do it in next patch.<br>
</span>
<blockquote style="margin-top:5pt;margin-bottom:5pt"><span
lang="EN-US"></span>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">On 03/28/2018 04:29 AM, Ge Song wrote:</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Please add a commit message here, maybe the 1st paragraph from</span></pre>
<pre><span lang="EN-US">your 0/1 mail.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">Signed-off-by: Ge Song <a href="mailto:ge.song@hxt-semitech.com" target="_blank"><ge.song@hxt-semitech.com></a></span></pre>
<pre><span lang="EN-US">---</span></pre>
<pre><span lang="EN-US"> discover/Makefile.am | 3 +-</span></pre>
<pre><span lang="EN-US"> lib/Makefile.am | 2 +</span></pre>
<pre><span lang="EN-US"> lib/efi/efivar.h | 42 ++</span></pre>
<pre><span lang="EN-US"> discover/platform-arm64.c | 751 ++++++++++++++++++++</span></pre>
<pre><span lang="EN-US"> lib/efi/efivar.c | 183 +++++</span></pre>
<pre><span lang="EN-US"> 5 files changed, 980 insertions(+), 1 deletion(-)</span></pre>
</blockquote>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">It will be added in patch
v2.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">diff --git a/lib/efi/efivar.h b/lib/efi/efivar.h</span></pre>
<pre><span lang="EN-US">new file mode 100644</span></pre>
<pre><span lang="EN-US">index 000000000000..b0efcf8aacaf</span></pre>
<pre><span lang="EN-US">--- /dev/null</span></pre>
<pre><span lang="EN-US">+++ b/lib/efi/efivar.h</span></pre>
<pre><span lang="EN-US">@@ -0,0 +1,42 @@</span></pre>
<pre><span lang="EN-US">+/*</span></pre>
<pre><span lang="EN-US">+ * Methods to manipulation of EFI variables on UEFI-based platforms</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">This comment doesn't seem to have any value.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">This will be removed next
time<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * This program is free software; you can redistribute it and/or modify</span></pre>
<pre><span lang="EN-US">+ * it under the terms of the GNU General Public License as published by</span></pre>
<pre><span lang="EN-US">+ * the Free Software Foundation; version 2 of the License.</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * This program is distributed in the hope that it will be useful,</span></pre>
<pre><span lang="EN-US">+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</span></pre>
<pre><span lang="EN-US">+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the</span></pre>
<pre><span lang="EN-US">+ * GNU General Public License for more details.</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * You should have received a copy of the GNU General Public License</span></pre>
<pre><span lang="EN-US">+ * along with this program; if not, write to the Free Software</span></pre>
<pre><span lang="EN-US">+ * Foundation, Inc., <a href="https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g">59 Temple Place, Suite 330, Boston, MA</a> <a href="https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g">02111</a>-1307 USA</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * Copyright (C) 2018 Huaxintong Semiconductor Technology Co.,Ltd. All rights </span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">This line and others like it have trailing whitespace.</span></pre>
<pre><span lang="EN-US"> </span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Sorry for it……<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ * reserved.</span></pre>
<pre><span lang="EN-US">+ * Copyright (C) 2018 Ge Song <a href="mailto:ge.song@hxt-semitech.com" target="_blank"><ge.song@hxt-semitech.com></a></span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">I'm surprised that you could be a copyright holder of this. I've never</span></pre>
<pre><span lang="EN-US">heard of a company that doesn't make you sign an agreement that says</span></pre>
<pre><span lang="EN-US">that they are the owner of all you create.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Thanks for pointing out it!
It will be fixed in V2 patch<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ */</span></pre>
<pre><span lang="EN-US">+#ifndef EFIVAR_H</span></pre>
<pre><span lang="EN-US">+#define EFIVAR_H 1</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Other files use just '#define EFIVAR_H'.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Sure, thanks.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#include <stdint.h></span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_NON_VOLATILE <wbr> 0x00000001</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_BOOTSERVICE_<wbr>ACCESS 0x00000002</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_RUNTIME_ACCESS <wbr> 0x00000004</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_HARDWARE_ERROR_<wbr>RECORD 0x00000008</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_AUTHENTICATED_<wbr>WRITE_ACCESS 0x00000010</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_TIME_BASED_<wbr>AUTHENTICATED_WRITE_ACCESS 0x00000020</span></pre>
<pre><span lang="EN-US">+#define EFI_VARIABLE_APPEND_WRITE <wbr> 0x00000040</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#define EFIVARFS_MAGIC 0xde5e81e4</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+extern char const efivarfs_path[];</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Why not just have an inline? Something like:</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">static inline const char* efivarfs_path(void) {return "/sys/firmware/efi/efivars/";)</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Thanks, this is the better
way:-)<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+int vars_get_variable(void *ctx, char *guidstr, char *name,</span></pre>
<pre><span lang="EN-US">+ uint8_t **data, size_t *data_size, uint32_t *attributes);</span></pre>
<pre><span lang="EN-US">+int vars_set_variable(void *ctx, char *guidstr, char *name,</span></pre>
<pre><span lang="EN-US">+ uint8_t *data, size_t data_size, uint32_t attributes);</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Would it be better to have these with an 'efi_' prefix (efi_get_variable)?</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Sounds like a good a
idea.:-)<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#endif /* EFIVAR_H */</span></pre>
<pre><span lang="EN-US">diff --git a/discover/platform-arm64.c b/discover/platform-arm64.c</span></pre>
<pre><span lang="EN-US">new file mode 100644</span></pre>
<pre><span lang="EN-US">index 000000000000..4b7204228d95</span></pre>
<pre><span lang="EN-US">--- /dev/null</span></pre>
<pre><span lang="EN-US">+++ b/discover/platform-arm64.c</span></pre>
<pre><span lang="EN-US">@@ -0,0 +1,751 @@</span></pre>
<pre><span lang="EN-US">+/*</span></pre>
<pre><span lang="EN-US">+ * Defines an arm64 platform, based on the powerpc platform.</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * Actually, this is apply to platforms that adopt UEFI as its underlying</span></pre>
<pre><span lang="EN-US">+ * firmware(x86/arm64).</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">These two comments don't seem to add any value.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">In fact, though the file
name is platform-arm64.c, petitboot on x86/x64<br>
platforms can works properly when the patch is applied. Just a
reminder,<br>
in case people may be misled by its name.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * This program is free software; you can redistribute it and/or modify</span></pre>
<pre><span lang="EN-US">+ * it under the terms of the GNU General Public License as published by</span></pre>
<pre><span lang="EN-US">+ * the Free Software Foundation; version 2 of the License.</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * This program is distributed in the hope that it will be useful,</span></pre>
<pre><span lang="EN-US">+ * but WITHOUT ANY WARRANTY; without even the implied warranty of</span></pre>
<pre><span lang="EN-US">+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the</span></pre>
<pre><span lang="EN-US">+ * GNU General Public License for more details.</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * You should have received a copy of the GNU General Public License</span></pre>
<pre><span lang="EN-US">+ * along with this program; if not, write to the Free Software</span></pre>
<pre><span lang="EN-US">+ * Foundation, Inc., <a href="https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g">59 Temple Place, Suite 330, Boston, MA</a> <a href="https://maps.google.com/?q=59+Temple+Place,+Suite+330,+Boston,+MA+02111&entry=gmail&source=g">02111</a>-1307 USA</span></pre>
<pre><span lang="EN-US">+ *</span></pre>
<pre><span lang="EN-US">+ * Copyright (C) 2018 Huaxintong Semiconductor Technology Co.,Ltd. All rights</span></pre>
<pre><span lang="EN-US">+ * reserved.</span></pre>
<pre><span lang="EN-US">+ * Copyright (C) 2018 Ge Song <a href="mailto:ge.song@hxt-semitech.com" target="_blank"><ge.song@hxt-semitech.com></a></span></pre>
<pre><span lang="EN-US">+ */</span></pre>
<pre><span lang="EN-US">+#include <string.h></span></pre>
<pre><span lang="EN-US">+#include <stdlib.h></span></pre>
<pre><span lang="EN-US">+#include <limits.h></span></pre>
<pre><span lang="EN-US">+#include <errno.h></span></pre>
<pre><span lang="EN-US">+#include <sys/types.h></span></pre>
<pre><span lang="EN-US">+#include <sys/fcntl.h></span></pre>
<pre><span lang="EN-US">+#include <sys/stat.h></span></pre>
<pre><span lang="EN-US">+#include <sys/statfs.h></span></pre>
<pre><span lang="EN-US">+#include <file/file.h></span></pre>
<pre><span lang="EN-US">+#include <talloc/talloc.h></span></pre>
<pre><span lang="EN-US">+#include <list/list.h></span></pre>
<pre><span lang="EN-US">+#include <log/log.h></span></pre>
<pre><span lang="EN-US">+#include <types/types.h></span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Put these in sorted order.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#include "platform.h"</span></pre>
<pre><span lang="EN-US">+#include "ipmi.h"</span></pre>
<pre><span lang="EN-US">+#include "efi/efivar.h"</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#define DEF_ATTR EFI_VARIABLE_NON_VOLATILE | \</span></pre>
<pre><span lang="EN-US">+ EFI_VARIABLE_RUNTIME_ACCESS | EFI_VARIABLE_BOOTSERVICE_<wbr>ACCESS</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Maybe put parenthesis () around these.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">OK, these two will be
revised next time.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+#define PETITBOOT_VARS_GUID "fb78ab4b-bd43-41a0-99a2-<wbr>4e74bef9169b"</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Can this be a const char array?</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Sure, const char array is
the better way.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+static const int ipmi_timeout = 10000; /* milliseconds. */</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+struct param {</span></pre>
<pre><span lang="EN-US">+ char *name;</span></pre>
<pre><span lang="EN-US">+ char *value;</span></pre>
<pre><span lang="EN-US">+ bool modified;</span></pre>
<pre><span lang="EN-US">+ struct list_item list;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">The alignment of these is off.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">This will be revised in
patch V2. </span>
</p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+};</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+struct platform_arm64 {</span></pre>
<pre><span lang="EN-US">+ struct list params;</span></pre>
<pre><span lang="EN-US">+ struct ipmi *ipmi;</span></pre>
<pre><span lang="EN-US">+};</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+static const char *known_params[] = {</span></pre>
<pre><span lang="EN-US">+ "auto-boot?",</span></pre>
<pre><span lang="EN-US">+ "network",</span></pre>
<pre><span lang="EN-US">+ "timeout",</span></pre>
<pre><span lang="EN-US">+ "bootdevs",</span></pre>
<pre><span lang="EN-US">+ "language",</span></pre>
<pre><span lang="EN-US">+ "debug?",</span></pre>
<pre><span lang="EN-US">+ "write?",</span></pre>
<pre><span lang="EN-US">+ "snapshots?",</span></pre>
<pre><span lang="EN-US">+ "console",</span></pre>
<pre><span lang="EN-US">+ "http_proxy",</span></pre>
<pre><span lang="EN-US">+ "https_proxy",</span></pre>
<pre><span lang="EN-US">+ NULL,</span></pre>
<pre><span lang="EN-US">+};</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#define to_platform_arm64(p) \</span></pre>
<pre><span lang="EN-US">+ (struct platform_arm64 *)(p->platform_data)</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Make this a function so type checking is done. The compiler</span></pre>
<pre><span lang="EN-US">should inline it.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+static int parse_nvram(struct platform_arm64 *platform)</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">The compiler will know that this is static, so no need for</span></pre>
<pre><span lang="EN-US">the static keyword. I see other existing code does this though.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Perhaps compiler could
handle it properly, I still think indicating it</span></p>
<p class="MsoNormal"><span lang="EN-US">explicitly is not bad ….<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ int i, rc;</span></pre>
<pre><span lang="EN-US">+ size_t size;</span></pre>
<pre><span lang="EN-US">+ uint32_t attr;</span></pre>
<pre><span lang="EN-US">+ uint8_t *data;</span></pre>
<pre><span lang="EN-US">+ char *pb_guid = PETITBOOT_VARS_GUID;</span></pre>
<pre><span lang="EN-US">+ char *known_param;</span></pre>
<pre><span lang="EN-US">+ struct param *param;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">These names are not all aligned, if that is what you were trying for...</span></pre>
<pre><span lang="EN-US"> </span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">This will be revised in
patch V2. </span>
</p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ for (i = 0; known_params[i]; i++) {</span></pre>
<pre><span lang="EN-US">+ known_param = known_params[i];</span></pre>
<pre><span lang="EN-US">+ rc = vars_get_variable(platform, pb_guid, known_param,</span></pre>
<pre><span lang="EN-US">+ <wbr> &data, &size, &attr);</span></pre>
<pre><span lang="EN-US">+ if (!rc) {</span></pre>
<pre><span lang="EN-US">+ param = talloc(platform, struct param);</span></pre>
<pre><span lang="EN-US">+ param->modified = false;</span></pre>
<pre><span lang="EN-US">+ param->name = talloc_strndup(platform, known_param,</span></pre>
<pre><span lang="EN-US">+ <wbr> sizeof(known_param));</span></pre>
<pre><span lang="EN-US">+ param->value = talloc_strdup(platform, (char *)data);</span></pre>
<pre><span lang="EN-US">+ list_add(&platform->params, ¶m->list);</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ return 0;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">This will always return zero. Either add error return, or make it a</span></pre>
<pre><span lang="EN-US">void return.</span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+}</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+static int write_nvram(struct platform_arm64 *platform)</span></pre>
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ struct param *param;</span></pre>
<pre><span lang="EN-US">+ char *pb_guid = PETITBOOT_VARS_GUID;</span></pre>
<pre><span lang="EN-US">+ uint32_t attr = DEF_ATTR;</span></pre>
<pre><span lang="EN-US">+ size_t data_size;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ list_for_each_entry(&platform-<wbr>>params, param, list) {</span></pre>
<pre><span lang="EN-US">+ if (param->modified) {</span></pre>
<pre><span lang="EN-US">+ data_size = sizeof(param->value) + 1;</span></pre>
<pre><span lang="EN-US">+ vars_set_variable(platform, pb_guid, param->name,</span></pre>
<pre><span lang="EN-US">+ (uint8_t *)param->value, data_size, attr);</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ return 0;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Either add error return, or make it a void return.</span></pre>
</blockquote>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+static void populate_network_config(struct platform_arm64 *platform,</span></pre>
<pre><span lang="EN-US">+ struct config *config)</span></pre>
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ char *val, *saveptr = NULL;</span></pre>
<pre><span lang="EN-US">+ const char *cval;</span></pre>
<pre><span lang="EN-US">+ int i;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Here you use one space between the type and the variable name, other</span></pre>
<pre><span lang="EN-US">places you use tabs, and in others you use multiple spaces. For</span></pre>
<pre><span lang="EN-US">consistency, please just use a single space for all.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">OK, all the similar problems
will be fixed in V2 patch
<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+static void populate_config(struct platform_arm64 *platform,</span></pre>
<pre><span lang="EN-US">+ struct config *config)</span></pre>
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ const char *val;</span></pre>
<pre><span lang="EN-US">+ char *end;</span></pre>
<pre><span lang="EN-US">+ unsigned long timeout;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ /* if the "auto-boot?' property is present and "false", disable auto</span></pre>
<pre><span lang="EN-US">+ * boot */</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "auto-boot?");</span></pre>
<pre><span lang="EN-US">+ config->autoboot_enabled = !val || strcmp(val, "false");</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "timeout");</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Is there some reason why we cant use the same config name as powerpc?</span></pre>
<pre><span lang="EN-US">For example 'petitboot,timeout' here? Maybe pass in a config names</span></pre>
<pre><span lang="EN-US">structure so this can be common to all platforms?</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">In fact, variables stored in
EFI NVRAM will have their own namespace in the<br>
form of Var-GUID. For example, "timeout" will be presented as <br>
"timeout-fb78ab4b-bd43-41a0-<wbr>99a2-4e74bef9169b" in efivarfs.
Just to save<br>
the effort to tackle the redundant "petitboot," and NVRAM space
to store it.<br>
Besides, I think config options may differ between platforms in
future, so<br>
maybe it's reasonable for platform to hold and handle its own
config options. <br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ if (val) {</span></pre>
<pre><span lang="EN-US">+ timeout = strtoul(val, &end, 10);</span></pre>
<pre><span lang="EN-US">+ if (end != val) {</span></pre>
<pre><span lang="EN-US">+ if (timeout >= INT_MAX)</span></pre>
<pre><span lang="EN-US">+ timeout = INT_MAX;</span></pre>
<pre><span lang="EN-US">+ config->autoboot_timeout_sec = (int)timeout;</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "language");</span></pre>
<pre><span lang="EN-US">+ config->lang = val ? talloc_strdup(config, val) : NULL;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ populate_network_config(<wbr>platform, config);</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ populate_bootdev_config(<wbr>platform, config);</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ if (!config->debug) {</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "debug?");</span></pre>
<pre><span lang="EN-US">+ config->debug = val && !strcmp(val, "true");</span></pre>
<pre><span lang="EN-US">+ }</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "write?");</span></pre>
<pre><span lang="EN-US">+ if (val)</span></pre>
<pre><span lang="EN-US">+ config->allow_writes = !strcmp(val, "true");</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "snapshots?");</span></pre>
<pre><span lang="EN-US">+ if (val)</span></pre>
<pre><span lang="EN-US">+ config->disable_snapshots = !strcmp(val, "false");</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "console");</span></pre>
<pre><span lang="EN-US">+ if (val)</span></pre>
<pre><span lang="EN-US">+ config->boot_console = talloc_strdup(config, val);</span></pre>
<pre><span lang="EN-US">+ /* If a full path is already set we don't want to override it */</span></pre>
<pre><span lang="EN-US">+ config->manual_console = config->boot_console &&</span></pre>
<pre><span lang="EN-US">+ <wbr> !strchr(config->boot_console, '[');</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "http_proxy");</span></pre>
<pre><span lang="EN-US">+ if (val)</span></pre>
<pre><span lang="EN-US">+ config->http_proxy = talloc_strdup(config, val);</span></pre>
<pre><span lang="EN-US">+ val = get_param(platform, "https_proxy");</span></pre>
<pre><span lang="EN-US">+ if (val)</span></pre>
<pre><span lang="EN-US">+ config->https_proxy = talloc_strdup(config, val);</span></pre>
<pre><span lang="EN-US">+}</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+static int load_config(struct platform *p, struct config *config)</span></pre>
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ struct platform_arm64 *platform = to_platform_arm64(p);</span></pre>
<pre><span lang="EN-US">+ int rc;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ rc = parse_nvram(platform);</span></pre>
<pre><span lang="EN-US">+ if (rc)</span></pre>
<pre><span lang="EN-US">+ pb_log("%s: Failed to parse nvram\n", __func__);</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">parse_nvram always returns zero, so fix something...</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">OK, about all the similar
problem you mentioned, I just want to<br>
keep the same form with functions in this platform-powerpc.c,<br>
will fix them in V2 patch. </span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ populate_config(platform, config);</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ get_active_consoles(config);</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ return 0;</span></pre>
<pre><span lang="EN-US">+}</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">diff --git a/lib/efi/efivar.c b/lib/efi/efivar.c</span></pre>
<pre><span lang="EN-US">new file mode 100644</span></pre>
<pre><span lang="EN-US">index 000000000000..59c2b9fe1cdf</span></pre>
<pre><span lang="EN-US">--- /dev/null</span></pre>
<pre><span lang="EN-US">+++ b/lib/efi/efivar.c</span></pre>
<pre><span lang="EN-US">@@ -0,0 +1,183 @@</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#include <errno.h></span></pre>
<pre><span lang="EN-US">+#include <fcntl.h></span></pre>
<pre><span lang="EN-US">+#include <stdio.h></span></pre>
<pre><span lang="EN-US">+#include <stdlib.h></span></pre>
<pre><span lang="EN-US">+#include <string.h></span></pre>
<pre><span lang="EN-US">+#include <sys/stat.h></span></pre>
<pre><span lang="EN-US">+#include <sys/types.h></span></pre>
<pre><span lang="EN-US">+#include <sys/ioctl.h></span></pre>
<pre><span lang="EN-US">+#include <unistd.h></span></pre>
<pre><span lang="EN-US">+#include <linux/fs.h></span></pre>
<pre><span lang="EN-US">+#include <stdbool.h></span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+#include <talloc/talloc.h></span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Could you put these includes into sorted order?</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Of course.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+#include "efivar.h"</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+char const efivarfs_path[] = "/sys/firmware/efi/efivars/";</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+static int vars_del_variable(void *ctx, char *guidstr, char *name)</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Should those be 'const char *guidstr', 'const char *name'? Please check your</span></pre>
<pre><span lang="EN-US">other routines for the same.</span></pre>
<pre><span lang="EN-US"> </span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">These will be revised in
patch v2<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ int fd, flag, errno_value;</span></pre>
<pre><span lang="EN-US">+ int rc = -1;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">It doesn't seem this initial value is ever used.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">I think if "open" or
"malloc" operation fails, this initial value will return to its
caller.
<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ char *path;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ path = talloc_asprintf(ctx, "%s%s-%s",</span></pre>
<pre><span lang="EN-US">+ efivarfs_path, name, guidstr);</span></pre>
<pre><span lang="EN-US">+ if (!path)</span></pre>
<pre><span lang="EN-US">+ return ENOMEM;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ fd = open(path, O_RDONLY|O_NONBLOCK);</span></pre>
<pre><span lang="EN-US">+ if (fd == -1)</span></pre>
<pre><span lang="EN-US">+ goto err;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ rc = ioctl (fd, FS_IOC_GETFLAGS, &flag);</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> ^</span></pre>
<pre><span lang="EN-US">Other files don't put a space after the function name, so please</span></pre>
<pre><span lang="EN-US">remove them to be consistent.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">Thanks for pointing out it.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+ if (rc == -1)</span></pre>
<pre><span lang="EN-US">+ goto err;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ flag &= ~FS_IMMUTABLE_FL;</span></pre>
<pre><span lang="EN-US">+ rc = ioctl (fd, FS_IOC_SETFLAGS, &flag);</span></pre>
<pre><span lang="EN-US">+ if (rc == -1)</span></pre>
<pre><span lang="EN-US">+ goto err;</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ close(fd);</span></pre>
<pre><span lang="EN-US">+</span></pre>
<pre><span lang="EN-US">+ rc = unlink(path);</span></pre>
<pre><span lang="EN-US">+ if (rc == -1)</span></pre>
<pre><span lang="EN-US">+ goto err;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">What's happening here? A goto to the next line...</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">These will be removed next
time...<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US">+int vars_get_variable(void *ctx, char *guidstr, char *name,</span></pre>
<pre><span lang="EN-US">+ uint8_t **data, size_t *data_size, uint32_t *attributes)</span></pre>
<pre><span lang="EN-US">+{</span></pre>
<pre><span lang="EN-US">+ int fd, errno_value;</span></pre>
<pre><span lang="EN-US">+ int rc = -1;</span></pre>
<pre><span lang="EN-US">+ void *p, *buf;</span></pre>
<pre><span lang="EN-US">+ size_t bufsize = 4096;</span></pre>
<pre><span lang="EN-US">+ size_t filesize = 0;</span></pre>
<pre><span lang="EN-US">+ ssize_t sz;</span></pre>
<pre><span lang="EN-US">+ char *path;</span></pre>
</blockquote>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">Here you use tabs, above you use spaces. With these the variables are not lined up.</span></pre>
<pre><span lang="EN-US">Maybe just put one space between the type and the name.</span></pre>
</blockquote>
<p class="MsoNormal"><span lang="EN-US">In fact "tab" was used in
this file for separation between the type and the name.<br>
May be option "expandtab" in vim changed occasionally.<br>
<br>
</span></p>
<blockquote style="margin-top:5pt;margin-bottom:5pt">
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US"> </span></pre>
<pre><span lang="EN-US">-Geoff</span></pre>
</blockquote>
</body>
</html>