On 2/27/21 5:34 AM, Roman Bogorodskiy wrote:
Implement "<os firmware='efi'>" support for bhyve driver.
As there are not really lot of options, try to find
"BHYVE_UEFI.fd" firmware which is installed by the
sysutils/uefi-edk2-bhyve FreeBSD port.
If not found, just use the first found firmware
in the firmwares directory (which is configurable via
config file).
Signed-off-by: Roman Bogorodskiy <bogorodskiy@xxxxxxxxx>
---
Not extremely happy about the LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE knob,
but not sure how to test this otherwise.
po/POTFILES.in | 1 +
src/bhyve/bhyve_domain.c | 5 +
src/bhyve/bhyve_firmware.c | 91 +++++++++++++++++++
src/bhyve/bhyve_firmware.h | 30 ++++++
src/bhyve/bhyve_process.c | 15 +++
src/bhyve/bhyve_process.h | 5 +
src/bhyve/meson.build | 1 +
.../bhyvexml2argv-firmware-efi.args | 11 +++
.../bhyvexml2argv-firmware-efi.ldargs | 1 +
.../bhyvexml2argv-firmware-efi.xml | 22 +++++
tests/bhyvexml2argvtest.c | 83 ++++++++++++++---
11 files changed, 254 insertions(+), 11 deletions(-)
create mode 100644 src/bhyve/bhyve_firmware.c
create mode 100644 src/bhyve/bhyve_firmware.h
create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.args
create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.ldargs
create mode 100644 tests/bhyvexml2argvdata/bhyvexml2argv-firmware-efi.xml
diff --git a/po/POTFILES.in b/po/POTFILES.in
index 80c5f145be..413783ee35 100644
--- a/po/POTFILES.in
+++ b/po/POTFILES.in
@@ -14,6 +14,7 @@
@SRCDIR@src/bhyve/bhyve_command.c
@SRCDIR@src/bhyve/bhyve_domain.c
@SRCDIR@src/bhyve/bhyve_driver.c
+@SRCDIR@src/bhyve/bhyve_firmware.c
@SRCDIR@src/bhyve/bhyve_monitor.c
@SRCDIR@src/bhyve/bhyve_parse_command.c
@SRCDIR@src/bhyve/bhyve_process.c
diff --git a/src/bhyve/bhyve_domain.c b/src/bhyve/bhyve_domain.c
index 8fbc554a0a..209e4d3905 100644
--- a/src/bhyve/bhyve_domain.c
+++ b/src/bhyve/bhyve_domain.c
@@ -64,6 +64,9 @@ bhyveDomainDefNeedsISAController(virDomainDefPtr def)
if (def->os.bootloader == NULL && def->os.loader)
return true;
+ if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_EFI)
+ return true;
+
if (def->nserials || def->nconsoles)
return true;
@@ -230,6 +233,8 @@ virDomainDefParserConfig virBhyveDriverDomainDefParserConfig = {
.domainPostParseCallback = bhyveDomainDefPostParse,
.assignAddressesCallback = bhyveDomainDefAssignAddresses,
.deviceValidateCallback = bhyveDomainDeviceDefValidate,
+
+ .features = VIR_DOMAIN_DEF_FEATURE_FW_AUTOSELECT,
};
static void
diff --git a/src/bhyve/bhyve_firmware.c b/src/bhyve/bhyve_firmware.c
new file mode 100644
index 0000000000..ccc3a5ffc8
--- /dev/null
+++ b/src/bhyve/bhyve_firmware.c
@@ -0,0 +1,91 @@
+/*
+ * bhyve_firmware.c: bhyve firmware management
+ *
+ * Copyright (C) 2021 Roman Bogorodskiy
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library 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
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library. If not, see
+ * <http://www.gnu.org/licenses/>.
+ *
+ */
+
+#include <config.h>
+#include <dirent.h>
+
+#include "viralloc.h"
+#include "virlog.h"
+#include "virfile.h"
+#include "bhyve_conf.h"
+#include "bhyve_firmware.h"
+
+#define VIR_FROM_THIS VIR_FROM_BHYVE
+
+VIR_LOG_INIT("bhyve.bhyve_firmware");
+
+
+#define BHYVE_DEFAULT_FIRMWARE "BHYVE_UEFI.fd"
+
+int
+bhyveFirmwareFillDomain(bhyveConnPtr driver,
+ virDomainDefPtr def,
+ unsigned int flags)
+{
+ g_autoptr(DIR) dir = NULL;
+ virBhyveDriverConfigPtr cfg = virBhyveDriverGetConfig(driver);
virBhyveDriverGetConfig() returns a reference, thus needs to be coupled
with virObjectUnref(cfg); otherwise .. [1]
+ const char *firmware_dir_cfg = cfg->firmwareDir;
+ const char *firmware_dir_env = NULL, *firmware_dir = NULL;
One variable per line, please. It turned out to be useful (although in
this specific case it's not using virXXXPtr type so doesn't matter):
https://listman.redhat.com/archives/libvir-list/2021-March/msg00542.html
+ struct dirent *entry;
+ char *matching_firmware = NULL;
+ char *first_found = NULL;
+
+ virCheckFlags(0, -1);
1: .. @cfg is leaked here .. [2]
+
+ if (def->os.firmware == VIR_DOMAIN_OS_DEF_FIRMWARE_NONE)
+ return 0;
2: .. or here. You get the idea. Alternatively, you can take inspiration
from the QEMU driver, where we defined autoptr cleanup func and then use:
g_autoptr(virQEMUDriverConfig) cfg = NULL;
+
+ if (virDirOpenIfExists(&dir, firmware_dir_cfg) > 0) {
+ while ((virDirRead(dir, &entry, firmware_dir)) > 0) {
+ if (STREQ(entry->d_name, BHYVE_DEFAULT_FIRMWARE)) {
+ matching_firmware = g_strdup(entry->d_name);
+ break;
+ }
+ if (!first_found)
+ first_found = g_strdup(entry->d_name);
+ }
+ }
+
+ if (!matching_firmware) {
+ if (!first_found) {
+ virReportError(VIR_ERR_CONFIG_UNSUPPORTED,
+ _("no firmwares found in %s"),
+ firmware_dir_cfg);
+ return -1;
+ } else {
+ matching_firmware = first_found;
3: ^^^
+ }
+ }
+
+ if (!def->os.loader)
+ def->os.loader = g_new0(virDomainLoaderDef, 1);
+
+ def->os.loader->type = VIR_DOMAIN_LOADER_TYPE_PFLASH;
+ def->os.loader->readonly = VIR_TRISTATE_BOOL_YES;
+
+ VIR_FREE(def->os.loader->path);
+
+ firmware_dir_env = g_getenv("LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE");
+ firmware_dir = firmware_dir_env ? firmware_dir_env : firmware_dir_cfg;
+ def->os.loader->path = g_build_filename(firmware_dir, matching_firmware, NULL);
I don't think that g_build_filename() consumes @matching_firmware (even
though GLib doesn't document this, looking at its source code - it sure
doesn't). Therefore, @matching_firmware should be freed in the end. The
same goes for @first_round - I can too find a path through this function
which leaks it. For [3] I think you can use g_steal_pointer() to
transfer the ownership.
+
+ return 0;
+}
diff --git a/tests/bhyvexml2argvtest.c b/tests/bhyvexml2argvtest.c
index 197334f9c4..13b8e34c2b 100644
--- a/tests/bhyvexml2argvtest.c
+++ b/tests/bhyvexml2argvtest.c
@@ -142,6 +163,35 @@ mymain(void)
if (!(driver.remotePorts = virPortAllocatorRangeNew("display", 5900, 65535)))
return EXIT_FAILURE;
+ if (!(driver.config = virBhyveDriverConfigNew()))
+ return EXIT_FAILURE;
+
+ fakefirmwaredir = g_strdup(FAKEFIRMWAREDIRTEMPLATE);
+
+ if (!g_mkdtemp(fakefirmwaredir)) {
+ fprintf(stderr, "Cannot create fakefirmwaredir");
+ abort();
+ }
+ driver.config->firmwareDir = fakefirmwaredir;
+
+ emptyfirmwaredir = g_strdup(FAKEFIRMWAREDIRTEMPLATE);
+
+ if (!g_mkdtemp(emptyfirmwaredir)) {
+ fprintf(stderr, "Cannot create emptyfirmwaredir");
+ abort();
+ }
+
+ i = 0;
+ while (firmwares[i]) {
+ g_autofree char *firmware_path = g_strdup_printf("%s/%s", fakefirmwaredir, firmwares[i++]);
for() loop would be much more readable IMO.
+
+ if (virFileTouch(firmware_path, 0600) < 0) {
+ fprintf(stderr, "Cannot create firmware file");
+ abort();
+ }
+ }
+
+ g_setenv("LIBVIRT_BHYVE_FIRMWARE_DIR_OVERRIDE", "test_firmware_dir", TRUE);
Reviewed-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
Michal
